feat: add apps management page and device authorization flow#146
Open
feat: add apps management page and device authorization flow#146
Conversation
- Created a new apps management page at /dashboard/apps to manage connected apps and discover available apps. - Added sidebar navigation link to the apps management page. - Implemented device consent page for authorizing apps with user permissions. - Added error handling page for authorization errors. - Updated integration tests to cover the new device authorization flow, including consent and revocation of app access. - Enhanced the app grant repository with necessary indexes for user and app associations. - Modified authentication service to support app-specific token management during refresh and exchange processes.
Contributor
Reviewer's GuideImplements a full app registry and device-authorization consent system, including a new dashboard Apps page, per-app device tokens and grants, CSRF-protected consent and revocation endpoints, and associated persistence, token, and test updates. Sequence diagram for device authorization consent and token exchange flowsequenceDiagram
actor User
participant ClientApp
participant Browser as Browser_UI
participant AuthRoutes as AuthRoutes_device
participant AuthService
participant AppGrantRepo
participant TokenRepo
participant TokenFactory
User->>ClientApp: Initiate sign_in
ClientApp->>Browser: Open /auth/device/login?app_id&state&redirect_uri
Browser->>AuthRoutes: GET /auth/device/login
AuthRoutes->>AuthRoutes: _get_device_app(app_id)
AuthRoutes->>AuthRoutes: _validate_redirect_uri(redirect_uri, app)
alt user_not_logged_in
AuthRoutes-->>Browser: 302 Redirect /?next=/auth/device/login...
Browser->>AuthRoutes: User logs in then retries /auth/device/login
end
AuthRoutes->>AppGrantRepo: find_active_grant(user_id, app_id)
alt grant_exists
AppGrantRepo-->>AuthRoutes: AppGrantDoc
AuthRoutes->>AuthService: get_user_profile(user_id)
AuthRoutes->>AuthService: create_device_auth_code(user_id, email, app_id)
AuthService->>TokenRepo: delete_by_user(user_id, token_type_device_auth, app_id)
AuthService->>TokenRepo: create(device_auth_token{app_id})
AuthService-->>AuthRoutes: raw_code
AuthRoutes->>AuthRoutes: _build_callback_redirect(code, state, redirect_uri, app)
AuthRoutes-->>Browser: 302 Redirect /auth/device/callback?code&state or redirect_uri
else no_grant
AppGrantRepo-->>AuthRoutes: None
AuthRoutes->>AuthService: get_user_profile(user_id)
AuthRoutes-->>Browser: Render device_consent.html with csrf_token
User->>Browser: Click Allow
Browser->>AuthRoutes: POST /auth/device/consent (app_id, state, redirect_uri, csrf_token)
AuthRoutes->>AuthRoutes: Validate_csrf_cookie
AuthRoutes->>AuthRoutes: _get_device_app(app_id)
AuthRoutes->>AuthRoutes: _validate_redirect_uri(redirect_uri, app)
AuthRoutes->>AppGrantRepo: create_or_reactivate(user_id, app_id)
AppGrantRepo-->>AuthRoutes: AppGrantDoc
AuthRoutes->>AuthService: get_user_profile(user_id)
AuthRoutes->>AuthService: create_device_auth_code(user_id, email, app_id)
AuthService->>TokenRepo: delete_by_user(user_id, token_type_device_auth, app_id)
AuthService->>TokenRepo: create(device_auth_token{app_id})
AuthService-->>AuthRoutes: raw_code
AuthRoutes->>AuthRoutes: _build_callback_redirect(code, state, redirect_uri, app)
AuthRoutes-->>Browser: 302 Redirect with code&state
end
ClientApp->>AuthRoutes: POST /auth/device/token (code)
AuthRoutes->>AuthService: exchange_device_code(code)
AuthService->>TokenRepo: find_and_consume_device_auth_token(code)
TokenRepo-->>AuthService: VerificationTokenDoc{user_id, app_id}
AuthService->>AuthService: load UserDoc
AuthService->>TokenFactory: issue_tokens(user, amr_ext, app_id)
TokenFactory-->>AuthService: access_token, refresh_token
AuthService-->>AuthRoutes: user, access_token, refresh_token, app_id
alt app_id_present
AuthRoutes->>AppGrantRepo: find_active_grant(user_id, app_id)
AppGrantRepo-->>AuthRoutes: AppGrantDoc or None
alt grant_active
AuthRoutes->>AppGrantRepo: touch_last_used(user_id, app_id)
else grant_revoked
AuthRoutes-->>ClientApp: AuthenticationError app access has been revoked
end
end
AuthRoutes-->>ClientApp: 200 DeviceTokenResponse(access_token, refresh_token, user)
Sequence diagram for app token refresh and revocation from dashboardsequenceDiagram
actor User
participant Browser as Browser_Dashboard
participant DashboardRoutes
participant AuthRoutes as AuthRoutes_device
participant AuthService
participant AppGrantRepo
participant TokenRepo
%% Dashboard Apps page
User->>Browser: Open /dashboard/apps
Browser->>DashboardRoutes: GET /dashboard/apps
DashboardRoutes->>AppGrantRepo: find_active_for_user(user_id)
AppGrantRepo-->>DashboardRoutes: list AppGrantDoc
DashboardRoutes-->>Browser: Render apps.html (connected, available, coming_soon)
%% Device refresh used by external app
participant ClientApp
ClientApp->>AuthRoutes: POST /auth/device/refresh (refresh_token)
AuthRoutes->>AuthService: refresh_token(refresh_token)
AuthService->>AuthService: verify_refresh_jwt(claims)
AuthService->>AuthService: load UserDoc
AuthService->>TokenFactory: issue_tokens(user, amr_from_claims, app_id_from_claims)
TokenFactory-->>AuthService: new_access, new_refresh
AuthService-->>AuthRoutes: user, new_access, new_refresh, app_id
alt app_id_present
AuthRoutes->>AppGrantRepo: find_active_grant(user_id, app_id)
AppGrantRepo-->>AuthRoutes: AppGrantDoc or None
alt grant_active
AuthRoutes->>AppGrantRepo: touch_last_used(user_id, app_id)
AuthRoutes-->>ClientApp: 200 DeviceRefreshResponse
else grant_revoked
AuthRoutes-->>ClientApp: AuthenticationError app access has been revoked
end
else no_app_id
AuthRoutes-->>ClientApp: 200 DeviceRefreshResponse
end
%% Revocation from dashboard
User->>Browser: Click Disconnect on app
Browser->>AuthRoutes: POST /auth/device/revoke (form app_id, header x_requested_with=fetch)
AuthRoutes->>AuthRoutes: Validate_csrf_header
AuthRoutes->>AppGrantRepo: revoke(user_id, app_id)
AppGrantRepo-->>AuthRoutes: revoked_bool
alt revoked
AuthRoutes->>AuthService: revoke_device_tokens(user_id, app_id)
AuthService->>TokenRepo: delete_by_user(user_id, token_type_device_auth, app_id)
TokenRepo-->>AuthService: deleted_count
AuthService-->>AuthRoutes: deleted_count
AuthRoutes-->>Browser: 200 {success True}
else not_found
AuthRoutes-->>Browser: 404 {error no active grant found}
end
Entity relationship diagram for app grants and device auth tokenserDiagram
UserDoc ||--o{ AppGrantDoc : has
UserDoc ||--o{ VerificationTokenDoc : has
AppGrantDoc {
string id
string user_id
string app_id
datetime granted_at
datetime last_used_at
datetime revoked_at
}
VerificationTokenDoc {
string id
string user_id
string email
string token_hash
string token_type
datetime expires_at
datetime created_at
datetime used_at
int attempts
string app_id
}
UserDoc {
string id
string email
string status
bool email_verified
}
AppRegistry ||--o{ AppGrantDoc : referenced_by
AppRegistry {
string app_id
string name
string status
string type
}
Class diagram for app registry, grants, and token handlingclassDiagram
class AppStatus {
<<enumeration>>
LIVE
COMING_SOON
}
class AppType {
<<enumeration>>
DEVICE_AUTH
}
class AppEntry {
+string name
+string icon
+string description
+bool verified
+AppStatus status
+AppType type
+list~string~ redirect_uris
+dict~string,string~ links
+list~string~ permissions
+bool is_live_device_app()
}
class AppGrantDoc {
+PyObjectId id
+PyObjectId user_id
+string app_id
+datetime granted_at
+datetime last_used_at
+datetime revoked_at
}
class AppGrantRepository {
+AppGrantRepository(collection)
+find_active_grant(user_id, app_id) AppGrantDoc
+find_active_for_user(user_id) list~AppGrantDoc~
+find_all_for_user(user_id) list~AppGrantDoc~
+create_or_reactivate(user_id, app_id) AppGrantDoc
+revoke(user_id, app_id) bool
+touch_last_used(user_id, app_id) None
}
class VerificationTokenDoc {
+PyObjectId id
+PyObjectId user_id
+string email
+string token_hash
+string token_type
+datetime expires_at
+datetime created_at
+datetime used_at
+int attempts
+string app_id
}
class TokenRepository {
+delete_by_user(user_id, token_type, app_id) int
}
class TokenFactory {
+generate_access_token(user, amr) string
+generate_refresh_token(user, amr, app_id) string
+issue_tokens(user, amr, app_id) tuple~string,string~
}
class AuthService {
+refresh_token(refresh_token_str) tuple~UserDoc,string,string,string~
+create_device_auth_code(user_id, email, app_id) string
+exchange_device_code(code) tuple~UserDoc,string,string,string~
+revoke_device_tokens(user_id, app_id) int
+get_user_profile(user_id) UserDoc
}
class AuthRoutesDevice {
+device_login(request, user, auth_service, grant_repo, app_id, redirect_uri, state)
+device_consent_approve(request, user, auth_service, grant_repo, app_id, state, csrf_token, redirect_uri)
+device_token(request, body, auth_service, grant_repo)
+device_refresh(request, body, auth_service, grant_repo)
+revoke_app(request, user, auth_service, grant_repo, app_id)
}
AppEntry <.. AppStatus
AppEntry <.. AppType
AppGrantRepository --> AppGrantDoc
AuthService --> TokenFactory
AuthService --> TokenRepository
AuthService --> AppGrantRepository
AuthRoutesDevice --> AuthService
AuthRoutesDevice --> AppGrantRepository
VerificationTokenDoc <.. TokenRepository
AppEntry <.. AuthRoutesDevice
AppGrantDoc <.. AuthRoutesDevice
VerificationTokenDoc <.. AuthService
AppGrantDoc <.. AuthService
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughLoads a validated app registry at startup and adds an app-scoped device-auth feature: new device consent/login/token/refresh/revoke endpoints, AppGrant persistence and indexes, token changes to carry Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as App Server
participant AuthSvc as AuthService
participant GrantRepo as AppGrantRepository
participant DB as Database
Client->>App: GET /auth/device/login (app_id, redirect_uri, state)
App->>App: validate app_id & redirect_uri vs registry
alt user has active grant
App->>AuthSvc: create_device_auth_code(user_id, email, app_id)
AuthSvc->>DB: insert device token (with app_id)
AuthSvc-->>App: code
App-->>Client: redirect to callback (code, state)
else no active grant
App-->>Client: render device consent page
end
sequenceDiagram
participant Client
participant App as App Server
participant GrantRepo as AppGrantRepository
participant DB as Database
Client->>App: POST /auth/device/consent (app_id, csrf_token)
App->>App: verify CSRF from secure cookie
alt CSRF valid
App->>GrantRepo: create_or_reactivate(user_id, app_id)
GrantRepo->>DB: upsert app-grant
GrantRepo-->>App: AppGrantDoc
App-->>Client: redirect to callback
else CSRF invalid
App-->>Client: render device error (403)
end
sequenceDiagram
participant Client
participant App as App Server
participant AuthSvc as AuthService
participant GrantRepo as AppGrantRepository
participant DB as Database
Client->>App: POST /auth/device/token (code)
App->>AuthSvc: exchange_device_code(code)
AuthSvc->>DB: read device token (may include app_id)
AuthSvc-->>App: (user, access, refresh, app_id)
App->>GrantRepo: find_active_grant(user_id, app_id)
alt grant active
GrantRepo->>DB: update last_used_at
GrantRepo-->>App: OK
App-->>Client: return tokens
else grant missing/revoked
App-->>Client: error (grant revoked)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 8 issues, and left some high level feedback:
- Access to
request.app.state.app_registryin multiple places assumes the registry is always initialized; consider wrapping this in a small helper (e.g.get_app_registry(request)) that defaults to an empty dict or raises a clearer error if missing to make the routes more robust and test-friendly. - The device auth error handling currently returns a generic
device_error.htmlfor various failure cases; if you expect clients to react differently to conditions like unknown app vs invalid redirect URI vs revoked grant, consider standardizing an error code in the response JSON or querystring so frontends and extensions can distinguish these states programmatically.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Access to `request.app.state.app_registry` in multiple places assumes the registry is always initialized; consider wrapping this in a small helper (e.g. `get_app_registry(request)`) that defaults to an empty dict or raises a clearer error if missing to make the routes more robust and test-friendly.
- The device auth error handling currently returns a generic `device_error.html` for various failure cases; if you expect clients to react differently to conditions like unknown app vs invalid redirect URI vs revoked grant, consider standardizing an error code in the response JSON or querystring so frontends and extensions can distinguish these states programmatically.
## Individual Comments
### Comment 1
<location path="repositories/app_grant_repository.py" line_range="81-90" />
<code_context>
+ """
+ now = datetime.now(timezone.utc)
+ try:
+ doc = await self._col.find_one_and_update(
+ {"user_id": user_id, "app_id": app_id},
+ {
+ "$set": {
+ "granted_at": now,
+ "revoked_at": None,
+ },
+ "$setOnInsert": {
+ "user_id": user_id,
+ "app_id": app_id,
+ "last_used_at": None,
+ },
+ },
+ upsert=True,
+ return_document=True,
+ )
+ return AppGrantDoc.from_mongo(doc) # type: ignore[return-value]
+ except PyMongoError as exc:
+ log.error(
</code_context>
<issue_to_address>
**issue (bug_risk):** find_one_and_update usage may not return a document as written, leading to runtime errors
`return_document=True` is not a valid PyMongo argument; it should use the `ReturnDocument` enum (e.g. `ReturnDocument.AFTER`). Depending on the driver, this may fail or return the pre-update/`None` document. Also, `doc` can still be `None`, which would break `AppGrantDoc.from_mongo(doc)`. Please switch to `ReturnDocument.AFTER` and add an explicit `doc is None` check with a clear error or fallback to avoid runtime failures.
</issue_to_address>
### Comment 2
<location path="repositories/token_repository.py" line_range="156-165" />
<code_context>
async def delete_by_user(
- self, user_id: ObjectId, token_type: str | None = None
+ self,
+ user_id: ObjectId,
+ token_type: str | None = None,
+ app_id: str | None = None,
) -> int:
"""
- Delete all tokens for a user, optionally filtered by token type.
+ Delete all tokens for a user, optionally filtered by token type and app_id.
Returns the number of documents deleted.
"""
query: dict = {"user_id": user_id}
if token_type is not None:
query["token_type"] = token_type
+ if app_id is not None:
+ query["app_id"] = app_id
try:
</code_context>
<issue_to_address>
**suggestion (performance):** Filtering tokens by app_id could benefit from an index for scalability
With `app_id` now part of the `delete_by_user` query, patterns like `{"user_id": ..., "token_type": ..., "app_id": ...}` will be frequent for per-app revocations. If the `verification-tokens` collection lacks an index including `app_id`, these `delete_many` calls may become slow as data grows. Consider adding a compound index such as `(user_id, token_type, app_id)` or at least `(user_id, app_id)` to keep these operations efficient.
</issue_to_address>
### Comment 3
<location path="tests/integration/test_device_auth.py" line_range="365-374" />
<code_context>
+ auth_svc.revoke_device_tokens.assert_awaited_once()
+
+
+def test_revoke_no_grant_returns_404(auth_svc, grant_repo, authed_user, _app_factory):
+ grant_repo.revoke.return_value = False
+
+ c = _app_factory(auth_svc, grant_repo, authed_user)
+ resp = c.post(
+ "/auth/device/revoke",
+ data={"app_id": "spoo-snap"},
+ headers={"X-Requested-With": "fetch"},
+ )
+ assert resp.status_code == 404
</code_context>
<issue_to_address>
**suggestion (testing):** Add integration coverage for the new /auth/device/refresh endpoint and revoke race conditions
This suite already covers device login/consent/token/revoke flows, but the new `/auth/device/refresh` endpoint isn’t exercised. Please add integration tests that:
- Call `POST /auth/device/refresh` with a valid refresh token and verify the `auth_service.refresh_token` result is returned/serialized correctly.
- When `auth_service.refresh_token` returns an `app_id`, assert `grant_repo.find_active_grant` is called and `grant_repo.touch_last_used` is awaited on success.
- When `find_active_grant` returns `None`, assert the endpoint responds with 401 (via `AuthenticationError`) and does not call `touch_last_used`.
These should mirror the existing `/auth/device/token` and revoke race-condition coverage for the refresh path.
Suggested implementation:
```python
resp = c.post(
"/auth/device/revoke",
data={"app_id": "spoo-snap"},
headers={"X-Requested-With": "fetch"},
)
assert resp.status_code == 404
def test_refresh_token_success(auth_svc, grant_repo, authed_user, _app_factory):
# auth_service returns a token payload that should be serialized directly
refresh_result = {
"access_token": "new-access",
"refresh_token": "new-refresh",
"expires_in": 3600,
}
auth_svc.refresh_token.return_value = refresh_result
c = _app_factory(auth_svc, grant_repo, authed_user)
resp = c.post(
"/auth/device/refresh",
data={"refresh_token": "refresh-token-123"},
headers={"X-Requested-With": "fetch"},
)
assert resp.status_code == 200
assert resp.json() == refresh_result
auth_svc.refresh_token.assert_awaited_once()
def test_refresh_token_touches_grant_when_found(
auth_svc, grant_repo, authed_user, _app_factory
):
# When refresh_token returns an app_id, we should look up and touch the grant
refresh_result = {
"access_token": "new-access",
"refresh_token": "new-refresh",
"expires_in": 3600,
"app_id": "spoo-snap",
}
auth_svc.refresh_token.return_value = refresh_result
# Simulate an active grant existing for this app_id
grant_repo.find_active_grant.return_value = object()
c = _app_factory(auth_svc, grant_repo, authed_user)
resp = c.post(
"/auth/device/refresh",
data={"refresh_token": "refresh-token-123"},
headers={"X-Requested-With": "fetch"},
)
assert resp.status_code == 200
assert resp.json() == refresh_result
auth_svc.refresh_token.assert_awaited_once()
grant_repo.find_active_grant.assert_awaited_once()
grant_repo.touch_last_used.assert_awaited_once()
def test_refresh_token_unauthorized_when_no_active_grant(
auth_svc, grant_repo, authed_user, _app_factory
):
# When refresh_token returns an app_id but there is no active grant,
# the endpoint should raise AuthenticationError and respond with 401.
refresh_result = {
"access_token": "new-access",
"refresh_token": "new-refresh",
"expires_in": 3600,
"app_id": "spoo-snap",
}
auth_svc.refresh_token.return_value = refresh_result
# No active grant found for this app_id
grant_repo.find_active_grant.return_value = None
c = _app_factory(auth_svc, grant_repo, authed_user)
resp = c.post(
"/auth/device/refresh",
data={"refresh_token": "refresh-token-123"},
headers={"X-Requested-With": "fetch"},
)
assert resp.status_code == 401
auth_svc.refresh_token.assert_awaited_once()
grant_repo.find_active_grant.assert_awaited_once()
grant_repo.touch_last_used.assert_not_awaited()
```
Depending on the existing implementation, you may need to:
1. Adjust the method name on `auth_svc` if it is not `refresh_token` (e.g. `refresh_device_token`), and update assertions accordingly.
2. Match the exact shape of the token payload that `/auth/device/refresh` returns (field names and structure) so that `refresh_result` mirrors the real service contract used elsewhere in this test file.
3. Align the arguments in `grant_repo.find_active_grant.assert_awaited_once(...)` and `grant_repo.touch_last_used.assert_awaited_once(...)` with the actual call signatures used by the production code (e.g. whether they receive `authed_user`, `app_id`, or a grant object).
</issue_to_address>
### Comment 4
<location path="tests/integration/test_device_auth.py" line_range="226-235" />
<code_context>
assert "state=xyz" in loc
+def test_device_login_without_grant_shows_consent(
+ auth_svc, grant_repo, authed_user, _app_factory
+):
+ c = _app_factory(auth_svc, grant_repo, authed_user)
+ resp = c.get("/auth/device/login?app_id=spoo-snap&state=xyz")
+ assert resp.status_code == 200
+ assert "Spoo Snap" in resp.text
+ assert "Allow" in resp.text
+ assert "Connecting as" in resp.text
+ assert "csrf_token" in resp.text
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting that the consent CSRF cookie is set when rendering the consent page
This test only checks that a CSRF token appears in the HTML, but not that the `_consent_csrf` cookie is actually set on the response, which the POST `/auth/device/consent` handler relies on. Please also assert that `"_consent_csrf"` is present in `resp.cookies` and, if feasible, that it is `httponly`, `secure`, and has the expected `samesite` attribute to better cover the CSRF mechanism end to end.
```suggestion
def test_device_login_without_grant_shows_consent(
auth_svc, grant_repo, authed_user, _app_factory
):
c = _app_factory(auth_svc, grant_repo, authed_user)
resp = c.get("/auth/device/login?app_id=spoo-snap&state=xyz")
assert resp.status_code == 200
assert "Spoo Snap" in resp.text
assert "Allow" in resp.text
assert "Connecting as" in resp.text
assert "csrf_token" in resp.text
# Ensure the consent CSRF cookie is set for the consent POST handler
assert "_consent_csrf" in resp.cookies
# Optionally, verify CSRF cookie security attributes via the Set-Cookie header
set_cookie_header = resp.headers.get("set-cookie", "")
assert "_consent_csrf=" in set_cookie_header
assert "HttpOnly" in set_cookie_header
assert "Secure" in set_cookie_header
# Adjust the expected SameSite value if the application uses a different setting
assert "SameSite=lax" in set_cookie_header
```
</issue_to_address>
### Comment 5
<location path="tests/unit/services/test_auth_service.py" line_range="324" />
<code_context>
refresh_tok = svc._generate_refresh_token(user, amr="pwd")
svc._user_repo.find_by_id.return_value = user
- result_user, new_access, new_refresh = await svc.refresh_token(refresh_tok)
+ result_user, new_access, new_refresh, app_id = await svc.refresh_token(
+ refresh_tok
+ )
</code_context>
<issue_to_address>
**suggestion (testing):** Add unit tests for app_id propagation in refresh_token and exchange_device_code and for revoke_device_tokens
The current tests only cover the classic refresh token case where `app_id` is `None`. We should also add coverage for the new app-specific behavior:
- For `refresh_token`, construct a refresh token with an `app_id` claim, call `refresh_token`, and assert that the returned `app_id` matches the claim and that `self._tokens.issue_tokens` is called with the expected `amr` and `app_id`.
- For `exchange_device_code`, set `token_doc.app_id = "spoo-snap"`, then assert that the returned `app_id` matches and that `issue_tokens` is called with `app_id="spoo-snap"`.
- For `revoke_device_tokens`, verify that with and without an `app_id`, `token_repo.delete_by_user` is called with the correct arguments and the returned count is forwarded.
These tests will protect the new app-specific token flow from regressions.
Suggested implementation:
```python
result_user, new_access, new_refresh, app_id = await svc.refresh_token(
refresh_tok
)
assert result_user is user
assert isinstance(new_access, str)
assert isinstance(new_refresh, str)
assert app_id is None
@pytest.mark.asyncio
async def test_refresh_token_app_specific_propagates_app_id_and_calls_issue_tokens(self):
# Arrange
user = make_user_doc(email_verified=True)
svc._user_repo.find_by_id.return_value = user
refresh_tok = svc._generate_refresh_token(
user,
amr="pwd",
app_id="spoo-snap",
)
# Act
result_user, new_access, new_refresh, app_id = await svc.refresh_token(
refresh_tok
)
# Assert
assert result_user is user
assert isinstance(new_access, str)
assert isinstance(new_refresh, str)
assert app_id == "spoo-snap"
svc._tokens.issue_tokens.assert_called_once()
_args, kwargs = svc._tokens.issue_tokens.call_args
# We only assert on the new behaviour we care about here
assert kwargs.get("amr") == "pwd"
assert kwargs.get("app_id") == "spoo-snap"
@pytest.mark.asyncio
async def test_refresh_token_invalid_raises(self):
svc._token_repo.consume_by_hash.return_value = token_doc
svc._user_repo.find_by_id.return_value = make_user_doc(email_verified=True)
```
Because I only see a small slice of `test_auth_service.py`, the following tests should be added near the existing tests for `exchange_device_code` and `revoke_device_tokens`. You can paste them into that file (keeping consistent style/fixtures):
```python
@pytest.mark.asyncio
async def test_exchange_device_code_app_specific_propagates_app_id_and_calls_issue_tokens(self):
# Arrange
user = make_user_doc(email_verified=True)
token_doc.user_id = user.id
token_doc.app_id = "spoo-snap"
svc._user_repo.find_by_id.return_value = user
# Adjust this to the real repo method used in exchange_device_code
svc._token_repo.consume_device_code.return_value = token_doc
# Act
result_user, access, refresh, app_id = await svc.exchange_device_code(
device_code="some-device-code"
)
# Assert
assert result_user is user
assert isinstance(access, str)
assert isinstance(refresh, str)
assert app_id == "spoo-snap"
svc._tokens.issue_tokens.assert_called_once()
_args, kwargs = svc._tokens.issue_tokens.call_args
# amr is whatever exchange_device_code uses (e.g., "device"), update if different
assert kwargs.get("app_id") == "spoo-snap"
@pytest.mark.asyncio
async def test_revoke_device_tokens_without_app_id_deletes_all_and_returns_count(self):
# Arrange
user = make_user_doc(email_verified=True)
svc._token_repo.delete_by_user.return_value = 3
# Act
count = await svc.revoke_device_tokens(user_id=user.id)
# Assert
svc._token_repo.delete_by_user.assert_called_once_with(
user_id=user.id,
app_id=None,
)
assert count == 3
@pytest.mark.asyncio
async def test_revoke_device_tokens_with_app_id_deletes_only_app_and_returns_count(self):
# Arrange
user = make_user_doc(email_verified=True)
svc._token_repo.delete_by_user.return_value = 2
# Act
count = await svc.revoke_device_tokens(
user_id=user.id,
app_id="spoo-snap",
)
# Assert
svc._token_repo.delete_by_user.assert_called_once_with(
user_id=user.id,
app_id="spoo-snap",
)
assert count == 2
```
You may need to:
1. Adjust `svc._token_repo.consume_device_code` to the actual repository method used inside `exchange_device_code` (e.g., `consume_by_device_code`, `consume_by_hash`, etc.).
2. Update the `amr` assertion in the `exchange_device_code` test if `exchange_device_code` uses a specific `amr` value (for example `"device"` or `"otp"`), and/or assert on any other arguments you care about.
3. Ensure these new tests are placed inside the same test class or module where `svc`, `token_doc`, and `make_user_doc` fixtures/helpers are already available.
</issue_to_address>
### Comment 6
<location path="tests/unit/repositories/test_indexes.py" line_range="24-33" />
<code_context>
api_keys_col = AsyncMock()
tokens_col = AsyncMock()
+ app_grants_col = AsyncMock()
+
db.__getitem__ = lambda self, name: {
</code_context>
<issue_to_address>
**suggestion (testing):** Extend index tests to cover all app-grants indexes, not just the unique (user_id, app_id)
Right now this only asserts the unique `("user_id", 1), ("app_id", 1)` index on `app-grants`. Please also assert the other two expected indexes `("user_id", 1), ("revoked_at", 1)` and `("app_id", 1), ("revoked_at", 1)` (e.g. with `assert_any_await` on the corresponding `create_index` calls) so the test fully reflects the repository’s index contract and catches regressions.
Suggested implementation:
```python
tokens_col.create_index.assert_any_await([("token", 1)], unique=True)
# app-grants indexes:
# 1) unique (user_id, app_id)
app_grants_col.create_index.assert_any_await(
[("user_id", 1), ("app_id", 1)], unique=True
)
# 2) (user_id, revoked_at)
app_grants_col.create_index.assert_any_await(
[("user_id", 1), ("revoked_at", 1)]
)
# 3) (app_id, revoked_at)
app_grants_col.create_index.assert_any_await(
[("app_id", 1), ("revoked_at", 1)]
)
```
If the `tokens_col.create_index` assertion in your file differs from
`tokens_col.create_index.assert_any_await([("token", 1)], unique=True)`,
adjust the `<<<<<<< SEARCH` block accordingly so it exactly matches the
current assertion, then keep the `REPLACE` content as-is beneath it.
Also ensure that the index definitions in your repository code for the
`"app-grants"` collection use the same key order and options as asserted
here; if the repository uses different field names, order, or options
(e.g. `sparse=True`, `background=True`), mirror those in these
`assert_any_await` calls.
</issue_to_address>
### Comment 7
<location path="tests/integration/test_auth_flow.py" line_range="184" />
<code_context>
mock_svc = AsyncMock()
mock_svc.login.return_value = (user, _ACCESS_TOKEN, _REFRESH_TOKEN)
- mock_svc.refresh_token.return_value = (user, "new.access", "new.refresh")
+ mock_svc.refresh_token.return_value = (user, "new.access", "new.refresh", None)
app = _build_test_app({get_auth_service: lambda: mock_svc})
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding an end-to-end test for refresh with an app-bound refresh token
This test only covers the case where `refresh_token` returns an app-agnostic token (`app_id=None`). To fully exercise the new app-specific refresh behavior on `/auth/device/refresh`, please add an integration test that:
- Mocks `auth_service.refresh_token` to return `(user, "new.access", "new.refresh", "spoo-snap")`.
- Calls `/auth/device/refresh` with a JSON body.
- Verifies the JSON response and, with a `grant_repo` mock, that an active grant is required and `touch_last_used` is called.
That will validate the new refresh entrypoint end-to-end over HTTP, beyond the existing unit tests.
Suggested implementation:
```python
mock_svc = AsyncMock()
mock_svc.login.return_value = (user, _ACCESS_TOKEN, _REFRESH_TOKEN)
mock_svc.refresh_token.return_value = (user, "new.access", "new.refresh", None)
app = _build_test_app({get_auth_service: lambda: mock_svc})
with TestClient(app, raise_server_exceptions=False) as client:
verified_user,
"refreshed.access",
"refreshed.refresh",
None,
)
mock_svc.get_user_profile.return_value = verified_user
mock_svc.set_password.side_effect = ValidationError("password already set")
async def test_device_refresh_with_app_bound_refresh_token(
get_auth_service,
get_grant_repo,
) -> None:
"""
End-to-end test for /auth/device/refresh when auth_service.refresh_token
returns an app-bound refresh token (non-None app_id).
"""
# Arrange
user = _build_user()
mock_auth_svc = AsyncMock()
mock_auth_svc.login.return_value = (user, _ACCESS_TOKEN, _REFRESH_TOKEN)
mock_auth_svc.refresh_token.return_value = (
user,
"new.access",
"new.refresh",
"spoo-snap",
)
mock_grant_repo = AsyncMock()
app = _build_test_app(
{
get_auth_service: lambda: mock_auth_svc,
get_grant_repo: lambda: mock_grant_repo,
}
)
# Act
with TestClient(app, raise_server_exceptions=False) as client:
response = client.post(
"/auth/device/refresh",
json={
"refresh_token": _REFRESH_TOKEN,
"app_id": "spoo-snap",
},
)
# Assert response payload
assert response.status_code == 200
body = response.json()
assert body["access_token"] == "new.access"
assert body["refresh_token"] == "new.refresh"
# Ensure the user object is present and matches the refreshed user
assert body["user"]["id"] == str(user.id)
# Assert grant_repo interaction: active grant is required and last_used is touched
mock_grant_repo.get_active_grant.assert_awaited_once()
mock_grant_repo.touch_last_used.assert_awaited_once()
```
You may need to adapt a few details to match your existing test helpers and repository API:
1. If `_build_user()` or `_ACCESS_TOKEN` / `_REFRESH_TOKEN` are named differently, swap in the appropriate helpers/constants used elsewhere in this file.
2. Replace `get_active_grant` / `touch_last_used` with the exact methods on your `grant_repo` that enforce an active grant for a given `app_id` and update the "last used" timestamp. If your repository uses a more specific signature (e.g. `get_active_grant(user_id=user.id, app_id="spoo-snap")`), update the assertions accordingly.
3. Adjust the JSON body keys (`"refresh_token"`, `"app_id"`) if your `/auth/device/refresh` endpoint expects different field names, mirroring whatever the existing app-agnostic refresh test in this file already uses.
4. If tests are synchronous in this file (regular `def` instead of `async def`), convert the new test to `def test_device_refresh_with_app_bound_refresh_token(...)` and remove `async`/`await` usage to match the style of other tests.
</issue_to_address>
### Comment 8
<location path="tests/integration/test_auth_routes.py" line_range="211" />
<code_context>
mock_svc = AsyncMock()
mock_svc.login.return_value = (user, _ACCESS_TOKEN, _REFRESH_TOKEN)
- mock_svc.refresh_token.return_value = (user, "new.access", "new.refresh")
+ mock_svc.refresh_token.return_value = (user, "new.access", "new.refresh", None)
app = _build_test_app({get_auth_service: lambda: mock_svc})
</code_context>
<issue_to_address>
**suggestion (testing):** Add a route-level test for the new /auth/device/refresh endpoint
`test_refresh_rotates_tokens` was updated for the new `refresh_token` signature, but there’s no equivalent coverage for `/auth/device/refresh`, which has its own request/response types. Please add a route-level test in this file that:
- Mocks `auth_service.refresh_token` to return `(user, "new.access", "new.refresh", "spoo-snap")`.
- Uses a mocked `grant_repo` from `get_app_grant_repo` to return an active grant and asserts `find_active_grant`/`touch_last_used` are called with the expected args.
- Issues `POST /auth/device/refresh` with a JSON `refresh_token` and asserts a 200 response matching the `DeviceRefreshResponse` schema.
This will verify the new route wiring and grant checks end-to-end.
Suggested implementation:
```python
def test_refresh_rotates_tokens():
user = _make_user_doc()
mock_svc = AsyncMock()
mock_svc.refresh_token.return_value = (user, "new.access", "new.refresh", None)
app = _build_test_app({get_auth_service: lambda: mock_svc})
with TestClient(app, raise_server_exceptions=False) as client:
# existing assertions for /auth/refresh go here
...
def test_device_refresh_route_rotates_tokens_and_updates_grant():
user = _make_user_doc()
mock_auth_service = AsyncMock()
mock_auth_service.refresh_token.return_value = (
user,
"new.access",
"new.refresh",
"spoo-snap",
)
mock_grant_repo = AsyncMock()
# Return some active grant object; tests only care that it is truthy
mock_grant_repo.find_active_grant.return_value = object()
app = _build_test_app(
{
get_auth_service: lambda: mock_auth_service,
get_app_grant_repo: lambda: mock_grant_repo,
}
)
with TestClient(app, raise_server_exceptions=False) as client:
response = client.post(
"/auth/device/refresh",
json={"refresh_token": "old.refresh"},
)
assert response.status_code == 200
body = response.json()
# The exact field names should match DeviceRefreshResponse
assert body["access_token"] == "new.access"
assert body["refresh_token"] == "new.refresh"
assert body["device_token"] == "spoo-snap"
# Ensure we wired the service and repo correctly
mock_auth_service.refresh_token.assert_awaited_once()
mock_grant_repo.find_active_grant.assert_awaited_once()
mock_grant_repo.touch_last_used.assert_awaited_once()
```
The snippet above shows only the visible tail of `test_refresh_rotates_tokens`; you should remove the placeholder `...` and keep your existing assertions for the `/auth/refresh` route.
You will also need to:
1. Adjust the assertions for `find_active_grant` and `touch_last_used` to match the actual call signatures used in your implementation (e.g., specific positional/keyword arguments such as user id, device id, grant id, or refresh token).
2. Align the response field names with your `DeviceRefreshResponse` schema (for example, if the device token field is named differently such as `device_refresh_token`, update the key used in the assertions).
3. Ensure `get_app_grant_repo` is imported or otherwise in scope in this test file, consistent with how other tests access it.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
Adds an app registry + grants system to support a new Dashboard “Apps” page and a device-authorization consent flow for external apps (extensions/desktop/CLI), including refresh/revocation paths.
Changes:
- Introduces an app registry loaded from
config/apps.yaml, surfaced in the dashboard and device-consent UI. - Implements device auth consent/grant checks (grant creation, token exchange validation, refresh, revoke) and ties refresh tokens to
app_id. - Adds MongoDB indexes and test updates for the new flows.
Reviewed changes
Copilot reviewed 28 out of 40 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/services/test_auth_service.py | Updates unit tests for new (…, app_id) return values from refresh/exchange methods. |
| tests/unit/repositories/test_indexes.py | Extends index-ensuring test coverage to include app-grants collection index creation. |
| tests/integration/test_device_auth.py | Expands integration tests for device login/consent/token exchange/revocation flows. |
| tests/integration/test_auth_routes.py | Updates mocked refresh-token return tuple shape. |
| tests/integration/test_auth_flow.py | Updates mocked refresh-token return tuple shape in auth journey tests. |
| templates/device_error.html | Adds a dedicated device-authorization error page. |
| templates/device_consent.html | Adds a device-consent UI for app authorization. |
| templates/dashboard/partials/sidebar.html | Adds sidebar navigation link to /dashboard/apps. |
| templates/dashboard/apps.html | Adds dashboard Apps management page + revoke UX. |
| static/images/apps/zapier.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/vscode.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/telegram-bot.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/spoo-snap.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/spoo-mobile.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/spoo-desktop.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/spoo-cli.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/slack-bot.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/raycast.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/n8n.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/mcp.svg | Adds icon asset for apps registry/dashboard display. |
| static/images/apps/discord-bot.svg | Adds icon asset for apps registry/dashboard display. |
| static/css/dashboard/apps.css | Adds styling for the new dashboard Apps page and revoke modal. |
| shared/app_registry.py | Adds YAML-backed app registry loader with validation/logging. |
| services/token_factory.py | Embeds optional app_id claim into refresh tokens; plumbs through token issuance. |
| services/auth_service.py | Preserves amr/app_id across refresh; binds device auth codes/tokens to app_id; adds token revocation helper. |
| schemas/models/token.py | Extends verification-token model to include optional app_id for device auth tokens. |
| schemas/models/app.py | Introduces typed app registry model/enums. |
| schemas/models/app_grant.py | Adds app-grant document model for user↔app authorization tracking. |
| schemas/dto/responses/auth.py | Adds DTO for /auth/device/refresh response. |
| schemas/dto/requests/auth.py | Adds DTO for /auth/device/refresh request. |
| routes/dashboard_routes.py | Adds /dashboard/apps handler that merges registry entries with active grants. |
| routes/auth_routes.py | Implements device login/consent/token/refresh/revoke endpoints + template rendering. |
| repositories/token_repository.py | Extends token deletion to optionally filter by app_id. |
| repositories/indexes.py | Adds app-grants collection indexes (unique + active/revoked query helpers). |
| repositories/app_grant_repository.py | Adds repository for grant CRUD, soft-revoke, and touch-last-used. |
| dependencies/services.py | Adds get_app_grant_repo dependency provider. |
| dependencies/init.py | Re-exports get_app_grant_repo. |
| config/apps.yaml | Adds initial app registry entries (live + coming soon) with metadata. |
| config.py | Removes deprecated global device-auth redirect allowlist setting. |
| app.py | Loads and stores app registry on app.state at startup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
routes/auth_routes.py (1)
223-239:⚠️ Potential issue | 🔴 CriticalReject app-scoped refresh tokens on the cookie refresh endpoint.
refresh_token()now preservesapp_id, but this handler throws it away and issues browser cookies anyway. That means a device app can present its app-scoped refresh JWT as therefresh_tokencookie to/auth/refreshand bypass the active-grant check that/auth/device/refreshenforces, so revocation is no longer effective.🔐 One safe direction
- _user, new_access, new_refresh, _app_id = await auth_service.refresh_token( + _user, new_access, new_refresh, app_id = await auth_service.refresh_token( refresh_token_str ) + if app_id is not None: + resp = JSONResponse( + { + "error": "invalid or expired refresh token", + "code": "AUTHENTICATION_ERROR", + }, + status_code=401, + ) + clear_auth_cookies(resp, jwt_cfg) + return resp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/auth_routes.py` around lines 223 - 239, The handler must reject app-scoped refresh tokens returned by auth_service.refresh_token: after calling auth_service.refresh_token(refresh_token_str) check the returned _app_id (or app_id) and if it is not None, treat it as an invalid/unsupported token for the browser endpoint — log a warning, clear cookies using clear_auth_cookies(resp, jwt_cfg), and return a 401 JSONResponse (same shape used for AuthenticationError) instead of issuing cookies; only call set_auth_cookies(resp, new_access, new_refresh, jwt_cfg) and return the RefreshResponse when _app_id is None.
🧹 Nitpick comments (1)
repositories/token_repository.py (1)
175-181: Includeapp_idin delete failure logs for triage clarity.When app-scoped deletion fails, missing
app_idin logs makes incident diagnosis harder.🧾 Small logging improvement
log.error( "token_repo_delete_by_user_failed", user_id=str(user_id), token_type=token_type, + app_id=app_id, error=str(exc), error_type=type(exc).__name__, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@repositories/token_repository.py` around lines 175 - 181, The delete failure log in token_repository currently omits app_id, hindering triage; update the log.error call that emits "token_repo_delete_by_user_failed" (in the token repository delete-by-user code path) to include app_id (e.g., app_id=str(app_id) or None-safe conversion) alongside user_id and token_type so failed app-scoped deletions record the app identifier for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@repositories/token_repository.py`:
- Around line 169-170: When building the Mongo query for app-scoped deletes,
don't always set query["app_id"] = app_id; instead, if the token type is
DEVICE_AUTH (use the DEVICE_AUTH constant/enum and the local variables query and
app_id), set query["$or"] = [{"app_id": app_id}, {"app_id": {"$exists": False}},
{"app_id": None}] so legacy device-auth documents without an app_id are matched;
for all other token types keep the exact match by setting query["app_id"] =
app_id.
In `@routes/auth_routes.py`:
- Around line 711-719: The current post-refresh grant check only ensures a
matching active grant (grant_repo.find_active_grant) but misses validating the
refresh token's issuance/version against the grant's revocation state, allowing
old tokens to become valid again after re-consent; modify the logic in the auth
flow (around auth_service.refresh_token, grant_repo.find_active_grant, and
grant_repo.touch_last_used) to obtain the token's issued_at or token_version
from auth_service.refresh_token and compare it to the grant's revocation
timestamp or current grant.version field returned by find_active_grant,
rejecting the refresh (raise AuthenticationError) if the token was issued before
revocation or its version is stale, and only call grant_repo.touch_last_used
after this additional validation succeeds.
- Around line 665-674: The touch_last_used bookkeeping call should be made
best-effort so a write failure doesn't turn a successful exchange_device_code()
into a 500; keep the current grant check using grant_repo.find_active_grant()
and raising AuthenticationError("app access has been revoked") when missing, but
wrap grant_repo.touch_last_used(user.id, app_id) in a try/except/handler that
catches exceptions, logs the failure (do not re-raise), and proceeds to return
the tokens as normal; reference exchange_device_code,
grant_repo.find_active_grant, grant_repo.touch_last_used, and
AuthenticationError when locating the code to change.
In `@routes/dashboard_routes.py`:
- Line 142: The route assumes request.app.state.app_registry always exists and
will raise AttributeError if missing; update the handler to guard access by
using getattr(request.app.state, "app_registry", {}) (and cast to dict[str,
AppEntry] if needed) or check hasattr(request.app.state, "app_registry") before
use, defaulting to an empty dict and optionally logging a warning so the route
can render safely when the registry was not populated; ensure you update any
references to app_registry in this handler to work with the empty default.
- Around line 151-154: The current order checks app.status before grant
membership, causing apps with an active grant to be classified as coming_soon;
change the logic in the loop that builds entry (reference variables app, app_id,
entry, grant_map, coming_soon) so that you first check if app_id is in grant_map
and set entry["grant"] and add it to the connected/connected_list (or the same
list used for connected apps) before falling back to the AppStatus.COMING_SOON
check; alternatively, if you must keep the status branch, add a condition to
treat COMING_SOON apps as connected when app_id is in grant_map so connected
grants are shown.
In `@shared/app_registry.py`:
- Around line 47-54: raw_apps may be present but not a mapping, causing
raw_apps.items() to raise; before iterating in the loop that builds registry
(variable registry of type dict[str, AppEntry]) add an explicit type check
(e.g., isinstance(raw_apps, dict) or collections.abc.Mapping) and if it is not a
mapping, log a warning (similar to the existing
log.warning("app_registry_empty", path=str(path)) but indicating wrong type) and
return an empty dict so the subsequent loop over raw_apps.items() is safe.
- Around line 36-41: The YAML load only catches yaml.YAMLError but misses
filesystem I/O failures; update the try/except around "with open(path) as f:
data = yaml.safe_load(f)" to also catch file I/O errors (e.g., OSError/IOError)
and log them similarly before returning {}. Specifically, add an except
OSError/IOError (or OSError as exc) branch that calls
log.error("app_registry_io_error", path=str(path), error=str(exc)) and returns
{} (keeping the existing yaml.YAMLError handler for parsing errors).
In `@templates/dashboard/apps.html`:
- Around line 84-94: The template renders an <a class="app-card app-card--link">
even when app.links is empty, producing a clickable anchor without an href;
update the template around the app-card anchor logic to conditionally render a
non-anchor disabled card (e.g., a <div> with class "app-card
app-card--disabled") or supply a safe fallback href when app.links is absent,
using the existing app.links checks (app.links, app.links|length,
'chrome'/'firefox' keys) so only when a real URL exists you output the <a ...
href="..."> and otherwise render the non-clickable disabled variant.
- Around line 51-53: The button currently injects app.app_id and app.name
directly into the onclick string which is unsafe; instead set data attributes
(e.g., data-app-id and data-app-name) on the button element (the button with
class "kebab-item kebab-item--danger") and remove the inline JS arguments, then
update the event handling to read event.currentTarget.dataset.appId and .appName
and pass those values into the existing revokeApp function (or call revokeApp
from a wrapper that reads the dataset). This avoids breaking on quotes and
prevents inline interpolation vulnerabilities while keeping the existing
revokeApp(identifier, name) usage.
In `@templates/device_consent.html`:
- Around line 186-188: Replace the inline templated string in the img onerror
handler with a data attribute and a safe JS read: add a data-fallback attribute
(populated with {{ (user.user_name or user.email)[:2]|upper }} from the
template) on the <img> that uses user.pfp.url, and change the onerror handler to
read from this.dataset.fallback (e.g., set parentElement.textContent =
this.dataset.fallback) so no templated value is injected into executable JS;
update the <img> tag where user.pfp.url is used and the onerror handler
accordingly.
---
Outside diff comments:
In `@routes/auth_routes.py`:
- Around line 223-239: The handler must reject app-scoped refresh tokens
returned by auth_service.refresh_token: after calling
auth_service.refresh_token(refresh_token_str) check the returned _app_id (or
app_id) and if it is not None, treat it as an invalid/unsupported token for the
browser endpoint — log a warning, clear cookies using clear_auth_cookies(resp,
jwt_cfg), and return a 401 JSONResponse (same shape used for
AuthenticationError) instead of issuing cookies; only call
set_auth_cookies(resp, new_access, new_refresh, jwt_cfg) and return the
RefreshResponse when _app_id is None.
---
Nitpick comments:
In `@repositories/token_repository.py`:
- Around line 175-181: The delete failure log in token_repository currently
omits app_id, hindering triage; update the log.error call that emits
"token_repo_delete_by_user_failed" (in the token repository delete-by-user code
path) to include app_id (e.g., app_id=str(app_id) or None-safe conversion)
alongside user_id and token_type so failed app-scoped deletions record the app
identifier for debugging.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0b3adef-6593-427a-a62d-9cd152d305f9
⛔ Files ignored due to path filters (12)
static/images/apps/discord-bot.svgis excluded by!**/*.svgstatic/images/apps/mcp.svgis excluded by!**/*.svgstatic/images/apps/n8n.svgis excluded by!**/*.svgstatic/images/apps/raycast.svgis excluded by!**/*.svgstatic/images/apps/slack-bot.svgis excluded by!**/*.svgstatic/images/apps/spoo-cli.svgis excluded by!**/*.svgstatic/images/apps/spoo-desktop.svgis excluded by!**/*.svgstatic/images/apps/spoo-mobile.svgis excluded by!**/*.svgstatic/images/apps/spoo-snap.svgis excluded by!**/*.svgstatic/images/apps/telegram-bot.svgis excluded by!**/*.svgstatic/images/apps/vscode.svgis excluded by!**/*.svgstatic/images/apps/zapier.svgis excluded by!**/*.svg
📒 Files selected for processing (28)
app.pyconfig.pyconfig/apps.yamldependencies/__init__.pydependencies/services.pyrepositories/app_grant_repository.pyrepositories/indexes.pyrepositories/token_repository.pyroutes/auth_routes.pyroutes/dashboard_routes.pyschemas/dto/requests/auth.pyschemas/dto/responses/auth.pyschemas/models/app.pyschemas/models/app_grant.pyschemas/models/token.pyservices/auth_service.pyservices/token_factory.pyshared/app_registry.pystatic/css/dashboard/apps.csstemplates/dashboard/apps.htmltemplates/dashboard/partials/sidebar.htmltemplates/device_consent.htmltemplates/device_error.htmltests/integration/test_auth_flow.pytests/integration/test_auth_routes.pytests/integration/test_device_auth.pytests/unit/repositories/test_indexes.pytests/unit/services/test_auth_service.py
💤 Files with no reviewable changes (1)
- config.py
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
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.
Summary by Sourcery
Introduce an app registry–backed apps management dashboard and extend the device authorization flow with consent, revocation, and app-scoped token handling.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit