FlowAuth: add token renewal endpoint (#7275)#7279
Conversation
WalkthroughThis PR introduces a token renewal endpoint ( Changes
Sequence DiagramsequenceDiagram
participant User as User
participant TokenUI as Token Component
participant TokenList as TokenList Component
participant API as API Module
participant Backend as Flask Backend
participant DB as Database
User->>TokenUI: Clicks "Renew" button
TokenUI->>TokenUI: handleRenew() triggered
TokenUI->>TokenList: onRenew(token_id)
TokenList->>TokenList: handleRenew(token_id)
TokenList->>API: renewToken(token_id, lifetime_minutes)
API->>Backend: POST /tokens/<token_id>/renew
Backend->>Backend: Validate ownership
Backend->>DB: Query TokenHistory, token_roles
DB-->>Backend: Return token and roles data
Backend->>Backend: Validate role consistency
Backend->>Backend: Compute max expiry
Backend->>Backend: Generate fresh JWT
Backend->>DB: Insert new TokenHistory entry
DB-->>Backend: Confirm insert
Backend-->>API: Return {token, history_id}
API-->>TokenList: Success response
TokenList->>TokenList: Reload token list
TokenList-->>User: Display renewed tokens
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
FlowAuth
|
||||||||||||||||||||||||||||
| Project |
FlowAuth
|
| Branch Review |
flowauth/token-renewal
|
| Run status |
|
| Run duration | 01m 04s |
| Commit |
|
| Committer | Joachim Jellinek |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
0
|
|
|
4
|
| View all changes introduced in this branch ↗︎ | |
5c75566 to
746153b
Compare
POST /tokens/tokens/<id>/renew mints a fresh JWT reusing the original token's name and roles. The original TokenHistory row is left intact — its JWT remains valid until its own exp claim passes, giving consumers a natural overlap window to switch over without downtime. Reusing the name keeps monitoring stable: tooling that creates services keyed off TokenHistory.name (e.g. CheckMK service "FlowAuth_Token_<name>") doesn't need any reconfiguration when a token is rotated. Renewal requires the caller to still hold every role the original token was minted with; if a role has been revoked since, renewal fails with 401 listing the missing roles. Optional lifetime_minutes in the body lets the user request a shorter token, validated against the current server/role caps; omitted means "max permitted". Frontend: a Renew button appears on each active token in TokenList; clicking it calls the new endpoint and refreshes the list. Expired tokens get no button (the backend would reject anyway). Depends on the tokens_with_roles association table from #7273 to recover the role set; this branch is stacked on flowauth/token-roles-visible and its base will move to master once that PR lands.
55f1139 to
2498728
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
flowauth/frontend/src/TokenList.jsx (1)
138-148:⚠️ Potential issue | 🟡 MinorMissing
keyprop on expired token list as well.Same issue applies to the expired tokens list.
🔧 Proposed fix
{expiredTokens.map((object) => ( <Token + key={object.id} id={object.id} name={object.name}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flowauth/frontend/src/TokenList.jsx` around lines 138 - 148, The expiredTokens list rendering in TokenList.jsx is missing the React key prop on each <Token> instance; update the expiredTokens.map callback to pass a stable unique key (e.g., key={object.id}) to the Token component so React can track list items properly—locate the expiredTokens.map(...) block that renders <Token id={object.id} ... /> and add key using the id (or another unique field) to each Token element.
🧹 Nitpick comments (3)
flowauth/backend/tests/test_token_generation.py (1)
172-198: Good test for ownership enforcement.Correctly verifies that an admin cannot renew another user's token.
Note: The static analysis warning about unused
uidandadmin_idvariables is valid but harmless in test fixtures. You could prefix them with underscores (_uid,_admin_id) to suppress the warnings if desired.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flowauth/backend/tests/test_token_generation.py` around lines 172 - 198, The test test_token_renewal_rejects_other_users_tokens defines unused variables uid and admin_id from fixtures which trigger static-analysis warnings; to fix, rename those to _uid and _admin_id (or prefix them with underscores) in the test function (and adjust any references like uid/admin_id if used elsewhere) so the variables remain available from the fixture but are marked as intentionally unused, leaving the test logic (auth.login, client.post, other_id retrieval, and the renew assertion) unchanged.flowauth/frontend/src/util/api.js (1)
406-416: Consider validatingparseIntresult to avoid sendingNaN.If
lifetime_minutesis a non-numeric string (e.g., user types "abc"),parseIntreturnsNaN, which would be serialised asnullin JSON and sent to the backend. While the backend validates this correctly, catching invalid input earlier provides a better user experience.♻️ Optional: Add NaN check
export async function renewToken(token_id, lifetime_minutes) { const body = {}; if (lifetime_minutes != null && lifetime_minutes !== "") { - body.lifetime_minutes = parseInt(lifetime_minutes, 10); + const parsed = parseInt(lifetime_minutes, 10); + if (!isNaN(parsed)) { + body.lifetime_minutes = parsed; + } } var dat = { method: "POST", body: JSON.stringify(body), }; return await getResponse("/tokens/tokens/" + token_id + "/renew", dat); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flowauth/frontend/src/util/api.js` around lines 406 - 416, The renewToken function currently assigns body.lifetime_minutes = parseInt(lifetime_minutes, 10) without checking for NaN; update renewToken to validate the parsed value (e.g., const mins = parseInt(lifetime_minutes, 10) and if Number.isNaN(mins) or !Number.isFinite(mins) then either omit body.lifetime_minutes or return/reject with a clear validation error) so you never stringify and send NaN to the backend; ensure you still call getResponse("/tokens/tokens/" + token_id + "/renew", dat) only with a valid numeric lifetime_minutes or without that field.flowauth/frontend/src/Token.jsx (1)
146-148: Consider updating PropTypes for new props.The component now uses
id,onRenew, androlesprops but these aren't declared inpropTypes. While this is partially a pre-existing issue, updating PropTypes would improve documentation and catch prop errors during development.♻️ Suggested PropTypes update
Token.propTypes = { classes: PropTypes.object.isRequired, + id: PropTypes.number.isRequired, + name: PropTypes.string.isRequired, + expiry: PropTypes.string.isRequired, + token: PropTypes.string.isRequired, + roles: PropTypes.arrayOf( + PropTypes.shape({ + id: PropTypes.number, + name: PropTypes.string, + }) + ), + onRenew: PropTypes.func, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flowauth/frontend/src/Token.jsx` around lines 146 - 148, Token.propTypes is missing declarations for the new props used by the component; update Token.propTypes to include id (PropTypes.oneOfType([PropTypes.string, PropTypes.number])), onRenew (PropTypes.func), and roles (PropTypes.arrayOf(PropTypes.string)) alongside the existing classes: PropTypes.object.isRequired so IDEs and React PropTypes checks catch misuse—add these keys to the Token.propTypes object and mark only classes as required unless other props must be required.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flowauth/frontend/src/TokenList.jsx`:
- Around line 107-118: The mapped Token components in TokenList.jsx are missing
the required React list key; update the JSX mapping over activeTokens to pass a
unique key prop (use object.id) to the Token component (the same id value
already passed as id) so React can reconcile properly when rendering the Token
components.
---
Outside diff comments:
In `@flowauth/frontend/src/TokenList.jsx`:
- Around line 138-148: The expiredTokens list rendering in TokenList.jsx is
missing the React key prop on each <Token> instance; update the
expiredTokens.map callback to pass a stable unique key (e.g., key={object.id})
to the Token component so React can track list items properly—locate the
expiredTokens.map(...) block that renders <Token id={object.id} ... /> and add
key using the id (or another unique field) to each Token element.
---
Nitpick comments:
In `@flowauth/backend/tests/test_token_generation.py`:
- Around line 172-198: The test test_token_renewal_rejects_other_users_tokens
defines unused variables uid and admin_id from fixtures which trigger
static-analysis warnings; to fix, rename those to _uid and _admin_id (or prefix
them with underscores) in the test function (and adjust any references like
uid/admin_id if used elsewhere) so the variables remain available from the
fixture but are marked as intentionally unused, leaving the test logic
(auth.login, client.post, other_id retrieval, and the renew assertion)
unchanged.
In `@flowauth/frontend/src/Token.jsx`:
- Around line 146-148: Token.propTypes is missing declarations for the new props
used by the component; update Token.propTypes to include id
(PropTypes.oneOfType([PropTypes.string, PropTypes.number])), onRenew
(PropTypes.func), and roles (PropTypes.arrayOf(PropTypes.string)) alongside the
existing classes: PropTypes.object.isRequired so IDEs and React PropTypes checks
catch misuse—add these keys to the Token.propTypes object and mark only classes
as required unless other props must be required.
In `@flowauth/frontend/src/util/api.js`:
- Around line 406-416: The renewToken function currently assigns
body.lifetime_minutes = parseInt(lifetime_minutes, 10) without checking for NaN;
update renewToken to validate the parsed value (e.g., const mins =
parseInt(lifetime_minutes, 10) and if Number.isNaN(mins) or
!Number.isFinite(mins) then either omit body.lifetime_minutes or return/reject
with a clear validation error) so you never stringify and send NaN to the
backend; ensure you still call getResponse("/tokens/tokens/" + token_id +
"/renew", dat) only with a valid numeric lifetime_minutes or without that field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4195b87a-3c8e-4d67-883b-b986264aebb3
📒 Files selected for processing (6)
CHANGELOG.mdflowauth/backend/flowauth/token_management.pyflowauth/backend/tests/test_token_generation.pyflowauth/frontend/src/Token.jsxflowauth/frontend/src/TokenList.jsxflowauth/frontend/src/util/api.js
| {activeTokens.map((object) => ( | ||
| <Token | ||
| id={object.id} | ||
| name={object.name} | ||
| expiry={object.expires} | ||
| token={object.token} | ||
| roles={object.roles} | ||
| classes={classes} | ||
| editAction={editAction} | ||
| onRenew={this.handleRenew} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Missing key prop on mapped Token components.
React lists require a unique key prop for efficient reconciliation. The id is now available and should be used as the key.
🔧 Proposed fix
{activeTokens.map((object) => (
<Token
+ key={object.id}
id={object.id}
name={object.name}
expiry={object.expires}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {activeTokens.map((object) => ( | |
| <Token | |
| id={object.id} | |
| name={object.name} | |
| expiry={object.expires} | |
| token={object.token} | |
| roles={object.roles} | |
| classes={classes} | |
| editAction={editAction} | |
| onRenew={this.handleRenew} | |
| /> | |
| ))} | |
| {activeTokens.map((object) => ( | |
| <Token | |
| key={object.id} | |
| id={object.id} | |
| name={object.name} | |
| expiry={object.expires} | |
| token={object.token} | |
| roles={object.roles} | |
| classes={classes} | |
| editAction={editAction} | |
| onRenew={this.handleRenew} | |
| /> | |
| ))} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flowauth/frontend/src/TokenList.jsx` around lines 107 - 118, The mapped Token
components in TokenList.jsx are missing the required React list key; update the
JSX mapping over activeTokens to pass a unique key prop (use object.id) to the
Token component (the same id value already passed as id) so React can reconcile
properly when rendering the Token components.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7279 +/- ##
==========================================
- Coverage 92.13% 92.09% -0.05%
==========================================
Files 278 278
Lines 10794 10826 +32
Branches 697 697
==========================================
+ Hits 9945 9970 +25
- Misses 696 704 +8
+ Partials 153 152 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Closes #7275.
Summary
New
POST /tokens/tokens/<token_id>/renewendpoint that mints a fresh JWT reusing the original token's name and roles. The originalTokenHistoryrow is left intact — its JWT remains valid until its ownexpclaim passes, giving consumers a natural overlap window to switch over without downtime.This is the second half of the renewal story: combined with #7274 (drop the absolute expiry cap), it eliminates the multi-step admin-bump dance currently required to rotate long-lived service tokens (e.g.
flowbot's AirflowFLOWAPI_TOKEN).Why same-name renewal matters operationally
External monitoring tooling can key off
TokenHistory.nameto create services likeFlowAuth_Token_<name>. Reusing the name on renewal keeps those alert channels stable across rotations — no reconfiguration needed.Behaviour
tokens_with_rolesassociation added in FlowAuth: surface assigned roles on the user's token list #7273. The renewal must reproduce the original role set verbatim — no role substitution.TokenHistoryrow is inserted; the old one is untouched. There is no revocation mechanism in flowauth (FlowAPI just verifies the JWT signature), so the overlap window is bounded only by the original token'sexp.lifetime_minutesin the body is honoured if≤ min(server.next_expiry(), role.next_expiry()); otherwise rejected with 400. Omitted means "max permitted" — same default as the mint endpoint.Frontend
TokenList.jsxpasses aRenewcallback through to each active token.Token.jsxshows aRenewbutton for active (non-expired) tokens; clicking it calls the new endpoint and triggers a list refresh. Errors surface inline as a small caption beneath the row. Expired tokens get no button (the backend would reject anyway).Tests
test_token_renewal_creates_new_row_with_same_name_and_roles— happy path: list goes from 1 row to 2, both with the same name and the same role set; new JWT differs from old.test_token_renewal_rejects_other_users_tokens— 401 when the calling user doesn't own the token.test_token_renewal_rejects_when_role_revoked— 401 with the missing role name in the message when a role has been removed from the user since mint.Branch base
This branch is stacked on
flowauth/token-roles-visible(the PR for #7273) because the renewal endpoint reads roles from that PR'stokens_with_rolestable. Once #7276 lands on master, I'll change this PR's base to master and rebase — the diff against master will then be just this PR's commit.Test plan
pytest flowauth/backend/tests/test_token_generation.py— three new tests pass; existing don't regress.Related
tokens_with_rolesassociation).latest_token_expirysilently caps tokens, requiring admin-bumps before every renewal #7274 / FlowAuth: drop the silent absolute-expiry cap (#7274) #7277 (drop the silent absolute-expiry cap).Summary by CodeRabbit
Release Notes
New Features
Documentation