FlowAuth: surface assigned roles on token list (#7273)#7276
Merged
jakejellinek merged 2 commits intomasterfrom Apr 29, 2026
Merged
FlowAuth: surface assigned roles on token list (#7273)#7276jakejellinek merged 2 commits intomasterfrom
jakejellinek merged 2 commits intomasterfrom
Conversation
Contributor
🚥 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-roles-visible
|
| Run status |
|
| Run duration | 00m 52s |
| Commit |
|
| Committer | Joachim Jellinek |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
4
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/backend/flowauth/token_management.py (1)
153-160:⚠️ Potential issue | 🔴 CriticalScope role selection to the current server/user before persisting.
roles=rolesnow stores the resolvedRolerows, but current resolution is name-only. If two servers share a role name, the wrong role record can be selected and persisted/encoded.🔧 Suggested fix
- roles = [] + user_roles_by_name = {ur.name: ur for ur in user_roles} + roles = [] for requested_role in json["roles"]: - if requested_role["name"] not in [ur.name for ur in user_roles]: + role_name = requested_role["name"] + if role_name not in user_roles_by_name: raise Unauthorized( - f"Role '{requested_role['name']}' is not permitted for the current user" + f"Role '{role_name}' is not permitted for the current user" ) - roles.append(Role.query.filter(Role.name == requested_role["name"]).first()) + roles.append(user_roles_by_name[role_name])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flowauth/backend/flowauth/token_management.py` around lines 153 - 160, The TokenHistory entry is persisting resolved Role rows via roles=roles but the current resolution uses name-only and can pick the wrong Role if multiple servers share a role name; update the role resolution that builds the roles variable (used when creating TokenHistory) to filter by the current server (and/or current_user if applicable) — e.g. query Role where Role.name IN provided_names AND Role.server_id == server.id (or Role.user_id == current_user.id if roles are user-scoped), then use those scoped Role objects when constructing TokenHistory and when encoding the token so the persisted/encoded roles reference the correct server-specific role records.
🤖 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/backend/tests/test_token_generation.py`:
- Line 99: The tuple unpacking from test_user_with_roles assigns uid, uname,
upass but uid is never used; update the unpack to avoid the unused variable
(e.g., unpack only uname and upass or rename uid to _uid) in the test function
where test_user_with_roles is consumed so the Ruff warning is resolved while
preserving uname and upass usage.
---
Outside diff comments:
In `@flowauth/backend/flowauth/token_management.py`:
- Around line 153-160: The TokenHistory entry is persisting resolved Role rows
via roles=roles but the current resolution uses name-only and can pick the wrong
Role if multiple servers share a role name; update the role resolution that
builds the roles variable (used when creating TokenHistory) to filter by the
current server (and/or current_user if applicable) — e.g. query Role where
Role.name IN provided_names AND Role.server_id == server.id (or Role.user_id ==
current_user.id if roles are user-scoped), then use those scoped Role objects
when constructing TokenHistory and when encoding the token so the
persisted/encoded roles reference the correct server-specific role records.
🪄 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: d596d44d-0a0d-49e9-ae96-a6cd0f432c58
📒 Files selected for processing (7)
CHANGELOG.mdflowauth/backend/flowauth/migrations/versions/a8c5e1d3f7b2_add_tokens_with_roles_assoc.pyflowauth/backend/flowauth/models.pyflowauth/backend/flowauth/token_management.pyflowauth/backend/tests/test_token_generation.pyflowauth/frontend/src/Token.jsxflowauth/frontend/src/TokenList.jsx
| ): | ||
| """The token list endpoint exposes the roles that were assigned at mint time.""" | ||
| with app.app_context(): | ||
| uid, uname, upass = test_user_with_roles |
Contributor
There was a problem hiding this comment.
Remove unused unpacked variable.
Line 99 assigns uid but never uses it, which triggers the current Ruff warning set.
✏️ Suggested tidy-up
- uid, uname, upass = test_user_with_roles
+ _uid, uname, upass = test_user_with_roles📝 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.
Suggested change
| uid, uname, upass = test_user_with_roles | |
| _uid, uname, upass = test_user_with_roles |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 99-99: Unpacked variable uid is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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` at line 99, The tuple
unpacking from test_user_with_roles assigns uid, uname, upass but uid is never
used; update the unpack to avoid the unused variable (e.g., unpack only uname
and upass or rename uid to _uid) in the test function where test_user_with_roles
is consumed so the Ruff warning is resolved while preserving uname and upass
usage.
This was referenced Apr 28, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
flowauth/backend/flowauth/migrations/versions/a8c5e1d3f7b2_add_tokens_with_roles_assoc.py (1)
40-53: Add a standalone index forrole_idon the association table.The composite PK helps
token_idlookups, but notrole_id-only paths. Addingrole_idindex avoids avoidable scans on larger datasets.♻️ Proposed migration diff
def upgrade(): @@ op.create_table( "tokens_with_roles", sa.Column("token_id", sa.Integer(), nullable=False), sa.Column("role_id", sa.Integer(), nullable=False), @@ sa.PrimaryKeyConstraint("token_id", "role_id"), ) + op.create_index( + "ix_tokens_with_roles_role_id", + "tokens_with_roles", + ["role_id"], + unique=False, + ) @@ def downgrade(): @@ + op.drop_index("ix_tokens_with_roles_role_id", table_name="tokens_with_roles") op.drop_table("tokens_with_roles")Also applies to: 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flowauth/backend/flowauth/migrations/versions/a8c5e1d3f7b2_add_tokens_with_roles_assoc.py` around lines 40 - 53, Add a standalone index on role_id for the association table to speed up role-only lookups: after the op.create_table("tokens_with_roles", ...) call that defines sa.Column("role_id", ...), create an index (e.g. via op.create_index) on the "tokens_with_roles" table for the "role_id" column (name it something like "ix_tokens_with_roles_role_id"); this uses the existing tokens_with_roles table and role_id column defined in the migration and avoids changing the primary key definition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@flowauth/backend/flowauth/migrations/versions/a8c5e1d3f7b2_add_tokens_with_roles_assoc.py`:
- Around line 40-53: Add a standalone index on role_id for the association table
to speed up role-only lookups: after the op.create_table("tokens_with_roles",
...) call that defines sa.Column("role_id", ...), create an index (e.g. via
op.create_index) on the "tokens_with_roles" table for the "role_id" column (name
it something like "ix_tokens_with_roles_role_id"); this uses the existing
tokens_with_roles table and role_id column defined in the migration and avoids
changing the primary key definition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2627e2a0-5383-4351-a65f-924a29c403fe
📒 Files selected for processing (1)
flowauth/backend/flowauth/migrations/versions/a8c5e1d3f7b2_add_tokens_with_roles_assoc.py
Roles granted to a token at mint time were only ever stored inside the gzipped JWT user_claims payload, so users had no practical way to inspect what permissions an existing token carried. Add a tokens_with_roles association table populated in add_token, expose the roles on the GET /tokens/tokens/<server_id> response, and render them under the nickname in the token list. Backwards compatible: pre-existing token_history rows have no associated roles and the UI simply omits the line.
…rade Bumps docstring coverage on the PR above CodeRabbit's 80% threshold. Existing migrations (73ea696e203d, 976c731ff30f) lack docstrings, but they're not in this PR's diff so they don't count against the score.
jakejellinek
force-pushed
the
flowauth/token-roles-visible
branch
from
5c75566 to
746153b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
flowauth/backend/tests/test_token_generation.py (1)
99-99:⚠️ Potential issue | 🟡 MinorUnused unpacked variable at Line 99.
uidis assigned but never used, matching the existing RuffRUF059warning previously raised on this PR.Suggested tidy-up
- uid, uname, upass = test_user_with_roles + _uid, uname, upass = test_user_with_roles🤖 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` at line 99, The test unpacks test_user_with_roles into uid, uname, upass but never uses uid, causing an unused-variable warning; to fix it, change the unpack to ignore the first value (e.g. use _ or _uid) so only uname and upass are treated as used, updating the unpack expression in test_token_generation.py where test_user_with_roles is destructured.
🤖 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/backend/tests/test_token_generation.py`:
- Around line 107-110: The test asserts the wrong status code for token minting:
update the expectation in test_token_generation.py so the response from the POST
to "/tokens/tokens/1" (stored in mint_response) is asserted to be 201 (the code
returned by flowauth.backend.flowauth.token_management's token creation path)
instead of 200; locate the mint_response assertion and change it to assert
mint_response.status_code == 201 to match token_management.py behavior.
---
Duplicate comments:
In `@flowauth/backend/tests/test_token_generation.py`:
- Line 99: The test unpacks test_user_with_roles into uid, uname, upass but
never uses uid, causing an unused-variable warning; to fix it, change the unpack
to ignore the first value (e.g. use _ or _uid) so only uname and upass are
treated as used, updating the unpack expression in test_token_generation.py
where test_user_with_roles is destructured.
🪄 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: 29638041-6619-4270-88d3-b9266732cc79
📒 Files selected for processing (7)
CHANGELOG.mdflowauth/backend/flowauth/migrations/versions/a8c5e1d3f7b2_add_tokens_with_roles_assoc.pyflowauth/backend/flowauth/models.pyflowauth/backend/flowauth/token_management.pyflowauth/backend/tests/test_token_generation.pyflowauth/frontend/src/Token.jsxflowauth/frontend/src/TokenList.jsx
✅ Files skipped from review due to trivial changes (2)
- flowauth/frontend/src/TokenList.jsx
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- flowauth/backend/flowauth/token_management.py
- flowauth/frontend/src/Token.jsx
- flowauth/backend/flowauth/models.py
Comment on lines
+107
to
+110
| mint_response = client.post( | ||
| "/tokens/tokens/1", headers={"X-CSRF-Token": csrf_cookie}, json=token_req | ||
| ) | ||
| assert mint_response.status_code == 200 |
Contributor
There was a problem hiding this comment.
Status-code assertion appears incorrect for token minting.
Line 110 expects 200, but flowauth/backend/flowauth/token_management.py:160 returns 201 for successful token creation. This can make the new test fail while the API is behaving correctly.
Proposed fix
- assert mint_response.status_code == 200
+ assert mint_response.status_code == 201🤖 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 107 - 110, The
test asserts the wrong status code for token minting: update the expectation in
test_token_generation.py so the response from the POST to "/tokens/tokens/1"
(stored in mint_response) is asserted to be 201 (the code returned by
flowauth.backend.flowauth.token_management's token creation path) instead of
200; locate the mint_response assertion and change it to assert
mint_response.status_code == 201 to match token_management.py behavior.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7276 +/- ##
==========================================
+ Coverage 92.08% 92.14% +0.05%
==========================================
Files 277 278 +1
Lines 10778 10794 +16
Branches 697 697
==========================================
+ Hits 9925 9946 +21
+ Misses 700 696 -4
+ Partials 153 152 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #7273.
Summary
When a user views their tokens in FlowAuth, the response and UI show
id, name, token, expires, server_name, username— but not the roles that were granted at mint time. The role information was only present inside the gzippeduser_claimspayload of the encrypted JWT, so there was no practical way to look at an existing token and see what permissions it carried.Changes
models.py— newtokens_with_rolesassociation table (mirroring the existingusers_with_rolespattern) plus arolesrelationship onTokenHistory.token_management.py—add_tokenpopulates the new association at mint time;list_my_tokensincludes arolesfield ([{id, name}, …]) in its response.a8c5e1d3f7b2_add_tokens_with_roles_assoc.pycreates the new table. Down-revision is the current head (976c731ff30f).TokenList.jsxpassesrolesthrough toToken.jsx, which renders a compactRoles: …caption under the nickname when present. Layout grid is otherwise unchanged.test_token_list_includes_assigned_rolesmints a token and asserts the list endpoint returns the expected role names.[Unreleased] / Added.Backwards compatibility
Purely additive on the wire. The JWT format is unchanged, so FlowAPI is unaffected. Existing
token_historyrows have no associated roles, so the UI simply omits the caption for them — those tokens get the new info on next reissue.Test plan
pytest flowauth/backend/tests/test_token_generation.py::test_token_list_includes_assigned_rolespasses.flowauth/backendtest suite passes (no regressions).flask db upgradethenflask db downgraderound-trips cleanly on a populated database.Related work
This is the first of three planned PRs against FlowAuth. The other two are tracked under #7274 (drop the silent absolute-expiry cap so renewals don't require admin-bumps) and #7275 (add a real token renewal endpoint, which depends on this PR's
tokens_with_rolestable).Summary by CodeRabbit
New Features
Tests