chore: add configurable expiry for management API tokens#403
Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurable expiry for management API tokens by introducing expiry-option records in Postgres, enforcing expiry during API-token auth, and exposing expiry selection in the UI.
Changes:
- Add DB/API schema support for
expires_at+expiry_option, plus anapi_token_expiry_optionstable and a new/auth/token/expiry-optionsendpoint. - Enforce token expiry in
get_current_user_from_authand expand token-related API responses. - Update the frontend token page to allow selecting an expiry option; add tests for expiry behaviors.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_postgres.py | Minor test formatting cleanup. |
| tests/test_api_token_expiry.py | Adds API token expiry option + expiry enforcement tests. |
| frontend/src/app/auth/token/page.tsx | Adds expiry option dropdown and displays token expiry in the list. |
| app/schemas/models.py | Adds Pydantic schemas/fields for expiry options and token expiry metadata. |
| app/migrations/versions/20260408_125946_daf5bf0b03c2_add_api_token_expiry_options.py | Creates expiry options table and adds token expiry columns. |
| app/db/models.py | Adds SQLAlchemy model for expiry options and expiry fields on API tokens. |
| app/db/init_db.py | Adds DB seeding helper for expiry options (and calls it from init_db). |
| app/core/security.py | Blocks expired API tokens and updates last_used_at for valid ones. |
| app/api/auth.py | Adds expiry-options endpoint and applies expiry selection during token creation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/amazeeio/amazee.ai/sessions/a93a3f19-57ef-42a9-abdf-1b76aee1a8b0 Co-authored-by: dan2k3k4 <158704+dan2k3k4@users.noreply.github.com>
Agent-Logs-Url: https://github.com/amazeeio/amazee.ai/sessions/a93a3f19-57ef-42a9-abdf-1b76aee1a8b0 Co-authored-by: dan2k3k4 <158704+dan2k3k4@users.noreply.github.com>
Applied all changes from the review thread (commits
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
alagoa
left a comment
There was a problem hiding this comment.
Overall looks good, nice work!
Just some comments and another question - do we want to have a cron to clear expired tokens after some days, or do we want to keep a log of them in the database?
|
@copilot address the comments
|
Agent-Logs-Url: https://github.com/amazeeio/amazee.ai/sessions/023151ab-b191-4dc3-8bb1-9312ca45aae0 Co-authored-by: dan2k3k4 <158704+dan2k3k4@users.noreply.github.com>
Fixed in commit |
dspachos
left a comment
There was a problem hiding this comment.
- Why do we need this? Is it a product decision?
Other than that:
1. Merge conflicts — mergeable: CONFLICTING
The PR currently has merge conflicts with dev. These need to be resolved before merging.
2. expiry_option default on existing tokens (Migration)
sa.Column("expiry_option", sa.String(), nullable=False, server_default="forever"),This is correct for new/existing rows, but existing tokens that were created before this PR won't have an expires_at value — they'll be NULL, meaning they never expire. This is the right default behavior, but it's worth documenting as a conscious decision.
3. conftest.py seeds expiry options globally
# conftest.py
init_api_token_expiry_options(db=db)This seeds expiry options for every test. If some tests don't need them, this adds unnecessary overhead. Not a blocker, but worth noting.
4. Open question from @alagoa: cleanup of expired tokens
"do we want to have a cron to clear expired tokens after some days, or do we want to keep a log of them in the database?"
This is unanswered. Currently expired tokens remain in the DB indefinitely — they're just rejected at auth time. A follow-up PR might want to add a cleanup job or a "revoked" status.
5. No loading/error state for expiry options in frontend
The <Select> renders the fallback "forever" item when options are empty/undefined, but there's no spinner or error message while the expiry options query is loading or has failed. If the API is slow/down, the user sees a dropdown with just "forever" and no indication something is wrong.
6. test_api_token_expiry.py seed_expiry_options fixture is redundant with conftest.py
Both conftest.py (line 37) and test_api_token_expiry.py (line 33) seed expiry options. The conftest.py version calls init_api_token_expiry_options(db=db) directly, while the test file uses _SessionProxy + monkeypatch. The autouse fixture in test_api_token_expiry.py runs after the conftest seeding, so it's seeding twice. One of these should be removed.
7. Missing init_api_token_expiry_options() call on app startup
The init_db() function calls init_api_token_expiry_options(), but if the app doesn't call init_db() on startup (e.g., in production where Alembic handles migrations), new expiry options added in future won't be auto-seeded. The migration handles the initial seed, but there's no runtime check. This is fine for now since the options are static, but something to keep in mind if options ever change.
| class DBAPITokenExpiryOption(Base): | ||
| __tablename__ = "api_token_expiry_options" | ||
|
|
||
| id = Column(Integer, primary_key=True, index=True) | ||
| name = Column(String, nullable=False) | ||
| slug = Column(String, unique=True, index=True, nullable=False) | ||
| days = Column(Integer, nullable=True) # None for forever | ||
| is_active = Column(Boolean, default=True) |
There was a problem hiding this comment.
The
is_active column is missing nullable=False in the SQLAlchemy model, while the Alembic migration correctly declares it as nullable=False, server_default=sa.true(). When the schema is created via Base.metadata.create_all() (as in the test suite's conftest.py), the column will be nullable with only a Python-side default rather than a DB-level constraint. This divergence means raw-SQL inserts or ORM usage outside the test harness could produce NULL rows that the filter(DBAPITokenExpiryOption.is_active) query would silently exclude.
| class DBAPITokenExpiryOption(Base): | |
| __tablename__ = "api_token_expiry_options" | |
| id = Column(Integer, primary_key=True, index=True) | |
| name = Column(String, nullable=False) | |
| slug = Column(String, unique=True, index=True, nullable=False) | |
| days = Column(Integer, nullable=True) # None for forever | |
| is_active = Column(Boolean, default=True) | |
| class DBAPITokenExpiryOption(Base): | |
| __tablename__ = "api_token_expiry_options" | |
| id = Column(Integer, primary_key=True, index=True) | |
| name = Column(String, nullable=False) | |
| slug = Column(String, unique=True, index=True, nullable=False) | |
| days = Column(Integer, nullable=True) # None for forever | |
| is_active = Column(Boolean, nullable=False, default=True) |
| def seed_expiry_options(db, monkeypatch): | ||
| session_proxy = _SessionProxy(db) | ||
| monkeypatch.setattr(init_db, "SessionLocal", lambda: session_proxy) | ||
| init_db.init_api_token_expiry_options() | ||
|
|
||
|
|
There was a problem hiding this comment.
conftest.py was updated in this same PR to call init_api_token_expiry_options(db=db) directly inside the db fixture, so the 15 default options are already present by the time any test runs. The seed_expiry_options autouse fixture here monkeypatches SessionLocal and calls init_api_token_expiry_options() again, but because all slugs already exist it finds nothing new to insert and commits an empty transaction — the _SessionProxy machinery never does meaningful work. Consider removing this fixture (and the _SessionProxy class) to reduce indirection, or remove the init_api_token_expiry_options call from conftest.py and keep only this fixture for tests that actually need it.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| # Create a new session for the test | ||
| db = TestingSessionLocal() | ||
|
|
||
| # Initialize default data |
There was a problem hiding this comment.
Expiry-option seeding added to the shared
db fixture
init_api_token_expiry_options(db=db) is now called for every test that pulls the db fixture, not just the new expiry tests. Tests completely unrelated to token expiry incur the extra query loop on every run. If the options table grows or the initialization logic becomes heavier, this becomes a broader test-suite slowdown. Consider either gating the call behind a fixture that only expiry tests depend on, or keeping it here but accepting the tradeoff explicitly.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Management API tokens had no expiry mechanism. This adds a configurable expiry system backed by a new
api_token_expiry_optionstable, enforced at auth time, and exposed in the UI.Migration
api_token_expiry_optionstable withis_activeasnullable=False, server_default=sa.true()op.bulk_insert— functional immediately afteralembic upgradewithout needinginit_dbdowngrade()uses inspector-based existence checks before droppingexpires_at/expiry_optioncolumns (mirrors upgrade pattern)API
POST /auth/token: resolves expiry slug against DB, filtersis_active=True; returns 400 for unknown or inactive slugsGET /auth/token/expiry-options: now requires authentication (consistent with other token endpoints)Auth
get_current_user_from_authlogger.exception()instead of silently swallowedInit
init_api_token_expiry_optionsuses structured logging; re-raises after rollback so startup failures surfaceFrontend
<Select>fallback renders "forever" when options areundefinedor empty (length === 0)tokensstate,fetchTokenscallback, anduseEffect; React Query is the single source of truthTests
seed_expiry_optionsfixture uses_SessionProxy+monkeypatchto redirectSessionLocalto the test-managed session rather than opening a separate DB connectiontest_create_token_with_expiryasserts HTTP 200, validates returned options list, and verifies expected slugs are presentGreptile Summary
This PR adds a configurable expiry system for management API tokens: a new
api_token_expiry_optionstable is seeded with 15 defaults (1 day → forever) via an Alembic migration, expiry is enforced at auth time with a 401, and the frontend gains an expiry selector.get_current_user_from_auth; previously silent exceptions during API token validation are now logged vialogger.exception()before falling through to JWT validation.POST /auth/tokenresolves theexpiryslug against active DB rows and calculatesexpires_at;GET /auth/token/expiry-optionsis a new auth-gated endpoint listing active options.useEffect) is removed; React Query is now the single source of truth, and an expiry<Select>with a "forever" fallback is added to the create-token form.Confidence Score: 4/5
Safe to merge; the expiry enforcement, migration, and frontend changes are all correct and well-tested.
The auth path, migration, and UI are solid. The only gap is that the DBAPITokenExpiryOption.is_active column lacks nullable=False in the SQLAlchemy model while the migration correctly enforces it — a divergence that doesn't bite in production (Alembic path) but creates a subtle inconsistency in environments using create_all(). The test fixture redundancy is cosmetic. No correctness or data-safety issues on the changed paths.
app/db/models.py — the is_active column definition should be brought in line with the migration. tests/conftest.py and tests/test_api_token_expiry.py both seed the same expiry-options data; one path is redundant.
Important Files Changed
Reviews (1): Last reviewed commit: "Merge branch 'dev' into add-expiry-for-a..." | Re-trigger Greptile