Skip to content

feat(auth): API authentication – MVP#24

Open
snabo1988 wants to merge 3 commits intomainfrom
feature/19-api-auth-mvp
Open

feat(auth): API authentication – MVP#24
snabo1988 wants to merge 3 commits intomainfrom
feature/19-api-auth-mvp

Conversation

@snabo1988
Copy link
Copy Markdown
Contributor

Summary

Introduces MVP-level API authentication for CSDS.

This PR adds a simple token-based authentication layer to secure API endpoints
required for MVP scope.

Scope

  • HTTP Bearer token authentication
  • Authorization dependency for protected endpoints
  • Minimal scope validation (sessions:read)
  • Aligned with MVP requirements (no user management yet)

Out of scope

  • User accounts
  • Token rotation
  • Role-based access control (RBAC)

Related

Notes

Authentication was implemented before automated tests as a prerequisite
for validating protected API endpoints.

@snabo1988 snabo1988 requested a review from knowack1 January 30, 2026 23:26
Copy link
Copy Markdown
Member

@knowack1 knowack1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title says this is Authentication but this is rather Authorization


router = APIRouter()

@router.get("/sessions", dependencies=[Depends(require_scopes({"sessions:read"}))])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why second session endpoint? How it works with the other conflicting one ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — this was an intermediate MVP artifact.

There is no intention to expose two /sessions endpoints. The duplicate route was introduced while testing scope-based authorization and was not meant to coexist long-term.

The implementation has now been consolidated into a single /sessions endpoint, which:

enforces sessions:read authorization

returns faulted sessions via the defined response model

removes any routing conflict or ambiguity

Authentication is handled earlier in the pipeline; this endpoint only performs authorization and data access.

Thanks for flagging this — resolved in the latest commit.

csds/main.py Outdated
def configure_logging():
logging.basicConfig(
format="%(name)s - %(levelname)s - %(message)s", level=logging.INFO
format="%(name)s - %(levelname)s - %(message)s",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not modify unrelated code nor introduce the unrelevant changes to the task. If you you like to introduce not related changes please do this in dedicated PR or at least in dedicated commit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right — the logging-related changes are not directly related to the scope of this PR.
I have reverted those changes and limited this PR strictly to the authentication/authorization-related logic and required wiring.

Any logging or other cross-cutting improvements will be proposed separately in a dedicated PR (or at least a dedicated commit), to keep the scope clean and reviewable.

csds-ui/.DS_Store
csds-ui/.env
csds-ui/node_modules/
# =========================
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same. Why to touch this file?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right — this change is unrelated. I will revert the .gitignore
modification to keep the PR focused.

@@ -0,0 +1,21 @@
[build-system]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes in this file are not related to Authorization.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change still present.



def hash_api_key(raw_key: str) -> str:
return hashlib.sha256(raw_key.encode("utf-8")).hexdigest()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How we assure that given scopes comes from trusted source?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scopes are not taken from the client. They are assigned server-side based
on trusted configuration for a given API key. This is a deliberate MVP
trade-off.

- Requires discipline to avoid redefining schemas in routes

## Notes
This ADR was introduced while implementing automated tests for
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true. Please remove this ADR files. I haven't seen practice to push such files into the repo. No one will ever read this files, hence I think there is no reason to add them. Better place for such description would be the GH test/issue itself rather the code/repo.

except Exception as e:
logging.error(f"Unexpected error: {e}", exc_info=True)
exit(1)
exit(1) No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please restore new line at the end of file.

from uec_csds.db.database import Database

db = Database()
app = create_app(db)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of this change ? db and app are already created in csds.py.

@knowack1
Copy link
Copy Markdown
Member

knowack1 commented Feb 3, 2026

@snabo1988 I think more important would be to focus on #23. Currently verifying changes is very cumbersome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

API Authorization and Authentication

2 participants