Skip to content

feat(api): add scoped persistent access token infrastructure - #790

Merged
DavidMStraub merged 8 commits into
gramps-project:masterfrom
elig-45:anniversary-ics-api
Jun 28, 2026
Merged

feat(api): add scoped persistent access token infrastructure#790
DavidMStraub merged 8 commits into
gramps-project:masterfrom
elig-45:anniversary-ics-api

Conversation

@elig-45

@elig-45 elig-45 commented Mar 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Implement the first atomic backend step: generic long-lived scoped access token infrastructure.

What’s included

  • Add a new persistent access_tokens table (Alembic migration), instead of a feature-specific column on users.
  • Add auth helpers for token scope validation and lifecycle:
    • get current token
    • create/rotate token
    • revoke token
    • resolve user by token+scope
  • Add authenticated user endpoint for scope-based token lifecycle:
    • GET /api/users/-/access-tokens/<scope>/
    • POST /api/users/-/access-tokens/<scope>/
    • DELETE /api/users/-/access-tokens/<scope>/
  • Add endpoint tests for JWT requirement, scope validation, and lifecycle behavior.

Not in this PR

  • No /api/anniversaries.ics endpoint yet.
  • No anniversaries business logic/UI changes.

Related

@DavidMStraub

Copy link
Copy Markdown
Member

Thanks! But let's go step by step please - atomic PRs, even if it belong to a single feature. Let's first just implement the persistent token feature without the new anniversaries endpoint. And we need to think more long term - having a dedicated database migration just for a single feature without any extensibility doesn't make sense. Long lived access tokens with limited scope, anniversaries being one (the first) example.

@elig-45 elig-45 closed this Mar 31, 2026
@elig-45 elig-45 reopened this Mar 31, 2026
@elig-45
elig-45 marked this pull request as draft March 31, 2026 00:05
@elig-45
elig-45 force-pushed the anniversary-ics-api branch from 073fab6 to 4eacd13 Compare March 31, 2026 00:12
@elig-45 elig-45 changed the title feat(api): add anniversary ICS feed and token lifecycle feat(api): add scoped persistent access token infrastructure Mar 31, 2026
@elig-45

elig-45 commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

Updated as requested and split to the first atomic step.

This PR now contains only the generic persistent scoped token infrastructure (migration + helpers + scoped token lifecycle endpoint + tests).

I removed the anniversaries ICS endpoint from this PR. I will open it as a follow-up PR on top of this once this part is reviewed.

@DavidMStraub

Copy link
Copy Markdown
Member

Hi,

thanks for refactoring! I'll review when tests pass.

@elig-45

elig-45 commented Apr 3, 2026

Copy link
Copy Markdown
Contributor Author

Done ;)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a first-pass backend implementation for long-lived, scope-bound “persistent access tokens” (intended to support future features like an anniversaries ICS feed), including storage, auth helpers, endpoints, and endpoint tests.

Changes:

  • Introduces an access_tokens persistence model + Alembic migration for user-scoped tokens.
  • Adds auth-layer helpers to validate scopes and manage token lifecycle (get/rotate/revoke/resolve user).
  • Adds /api/users/-/access-tokens/<scope>/ GET/POST/DELETE endpoints plus endpoint tests.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/test_endpoints/test_access_tokens.py Adds endpoint-level lifecycle and validation tests for the new access-token endpoints.
gramps_webapi/auth/const.py Defines allowed persistent access-token scopes.
gramps_webapi/auth/__init__.py Adds AccessToken model and helper functions for token scope validation + lifecycle.
gramps_webapi/api/resources/access_tokens.py Implements authenticated per-scope token management endpoints for the current user.
gramps_webapi/api/__init__.py Registers the new access-token endpoint route.
alembic_users/versions/6d8f3cb50b71_add_persistent_access_tokens_table.py Creates the new access_tokens table and indexes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread gramps_webapi/auth/__init__.py Outdated
Comment thread gramps_webapi/auth/__init__.py
Comment thread gramps_webapi/api/resources/access_tokens.py Outdated
Comment thread alembic_users/versions/6d8f3cb50b71_add_persistent_access_tokens_table.py Outdated
@DavidMStraub

Copy link
Copy Markdown
Member

@elig-45 please hold back your agent until I've completed the review, thanks!

I appreciate your proactiveness but I can't review if you keep changing things.

@DavidMStraub

DavidMStraub commented Apr 5, 2026

Copy link
Copy Markdown
Member

Also - it's ok if you use AI of course, but it's NOT OK if you just tell copilot to implement maintainer review comments, especially if they are security relevant. I expect you take full responsibility of all commits.

@elig-45

elig-45 commented Apr 5, 2026

Copy link
Copy Markdown
Contributor Author

Ok I understand, I wanted to try this functionnality here
I'll remove it

@DavidMStraub

Copy link
Copy Markdown
Member

I looked into this in detail and thought about it for a while, but I realized I need to think deeper about the implications (security and design) before it's worth (for you) investing more work into it. In the case of OIDC authentication, I was a bit too quick merging the contribution without fully realizing the maintenance burden.

Let's put this on hold for a few days and I'll come back with more useful feedback.

@elig-45

elig-45 commented Apr 5, 2026

Copy link
Copy Markdown
Contributor Author

np, keep me updated!
Happy Easter !

@DavidMStraub

Copy link
Copy Markdown
Member

Sorry for the delay. I think this is the right way foward. If you can rebase on current on current main and address the comments, I think we can merge this in the near future.

Please make sure the tokens are not stored in the database (only an appropriate hash) and that GET doesn't return the token value (only whether it's active/valid).

@elig-45
elig-45 force-pushed the anniversary-ics-api branch from 3f35b6e to f1d3eba Compare May 30, 2026 14:42
@elig-45

elig-45 commented May 30, 2026

Copy link
Copy Markdown
Contributor Author

I rebased this branch on current master and addressed the requested security changes:

  • persistent tokens are no longer stored in plaintext (only token_hash is stored)
  • GET /api/users/-/access-tokens/<scope>/ now returns only token status (active) and never the token value
  • token validation now checks non-revoked rows (revoked_at IS NULL) with non-null hash

I also fixed the Alembic chain so there is a single migration head after rebase.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Comment thread gramps_webapi/auth/__init__.py
@DavidMStraub

Copy link
Copy Markdown
Member

Please review copilot's last comment and fix or reject. Apart from that, LGTM!

…level unique/index flags

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@elig-45

elig-45 commented Jun 4, 2026

Copy link
Copy Markdown
Contributor Author

Done ✅

@DavidMStraub
DavidMStraub merged commit 0fbd229 into gramps-project:master Jun 28, 2026
3 checks passed
@DavidMStraub

Copy link
Copy Markdown
Member

Sorry for the delay.

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.

3 participants