Skip to content

Finish implementation of stubbed roles APIs#4

Open
cyrossignol wants to merge 2 commits intomainfrom
roles
Open

Finish implementation of stubbed roles APIs#4
cyrossignol wants to merge 2 commits intomainfrom
roles

Conversation

@cyrossignol
Copy link
Collaborator

@cyrossignol cyrossignol commented Mar 7, 2026

This fleshes-out the role management API and access controls and includes a mechanism for evicting UserInfo objects from the cache when permissions change.

Frontend PR: TaskarCenterAtUW/workspaces-frontend#44

Summary by CodeRabbit

  • New Features

    • Workspace user management API: list members, assign roles, remove members; only workspace Leads can manage roles.
    • Workspace responses now include a user's effective role; creator is assigned Lead on workspace creation.
  • Performance

    • Authentication cache switched to user-ID keyed with explicit eviction on role changes and token rotation.
  • Bug Fixes

    • Added HTTP 409 Conflict error type.

@coderabbitai
Copy link

coderabbitai bot commented Mar 7, 2026

📝 Walkthrough

Walkthrough

Adds a users module for workspace role management, moves user-role logic out of workspaces, introduces UserRepository and user routes, switches token cache to a user-UUID keyed cache with eviction, enforces workspace-lead guards for team mutations, and adds a ConflictException (409).

Changes

Cohort / File(s) Summary
Core exceptions
api/core/exceptions.py
Added ConflictException (409) with docstring "Base exception for conflict errors."
Auth & cache
api/core/security.py
Replaced token-keyed TTL cache with a user-UUID keyed cache (_user_info_cache), added evict_user_from_cache(auth_uid: UUID), stored token_jti on cached UserInfo, added UserInfo.effective_role(workspaceId: int), and updated token validation to evict on jti mismatch.
Application router
api/main.py
Registered new users_router from api/src/users/routes.py under /api/v1.
New users domain
api/src/users/schemas.py, api/src/users/repository.py, api/src/users/routes.py
Added workspace user models/enums (WorkspaceUserRoleType, WorkspaceUserRole, SetRoleRequest, WorkspaceUserRoleItem, User), UserRepository (queries and role assign/remove methods), and new endpoints: GET workspace users, PUT set role, DELETE remove role; endpoints evict user cache on mutations and add dependencies for task/workspace validation.
Teams adjustments
api/src/teams/repository.py, api/src/teams/routes.py
Switched imports to api.src.users.schemas, replaced get_osm_repoget_user_repo (returns UserRepository), updated dependency wiring and calls to get_current_user, and enforced workspace-lead checks on team create/update/delete and member add/remove operations.
Workspaces refactor
api/src/workspaces/repository.py, api/src/workspaces/routes.py, api/src/workspaces/schemas.py
Removed legacy user-role methods/types from workspaces repository and schemas, introduced WorkspaceResponse model, updated workspace endpoints to use UserRepository (assign creator as LEAD, evict caches), return WorkspaceResponse including effective role, and added helper _to_response.

Sequence Diagram

sequenceDiagram
    participant Client
    participant API
    participant Auth as Auth/Cache
    participant UserRepo as UserRepository
    participant DB as Database

    Client->>API: PUT /workspaces/{id}/users/{user_id}/role
    API->>Auth: validate_token()
    Auth->>Auth: verify JWT and jti
    Auth->>Auth: check _user_info_cache[user_uuid]
    alt cache miss or jti mismatch
        Auth->>DB: fetch user info (TDEI/DB)
        DB-->>Auth: user info (includes token_jti)
        Auth->>Auth: store _user_info_cache[user_uuid]
    end
    Auth-->>API: return UserInfo
    API->>Auth: isWorkspaceLead(UserInfo, workspace_id)
    Auth-->>API: authorized
    API->>UserRepo: assign_member_role(workspace_id, user_id, role)
    UserRepo->>DB: insert/update WorkspaceUserRole
    DB-->>UserRepo: confirm
    UserRepo-->>API: success
    API->>Auth: evict_user_from_cache(user_id)
    Auth->>Auth: remove _user_info_cache[user_id]
    Auth-->>API: evicted
    API-->>Client: 204 No Content
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • jeffmaki

Poem

🐰
I hop where routers meet the role,
Cache by user keeps control,
Leads assigned with nimble paws,
Evictions tidy up the laws,
A gentle 409 — hooray, bravo!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.32% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main objective of the changeset, which is implementing previously stubbed role management APIs with cache eviction mechanisms.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch roles

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (2)
api/core/exceptions.py (1)

18-22: Consider documenting the distinction from AlreadyExistsException.

Both ConflictException and AlreadyExistsException use HTTP 409. Consider updating the docstrings to clarify when each should be used (e.g., AlreadyExistsException for duplicate creation attempts vs. ConflictException for other conflict scenarios like concurrent modifications).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/core/exceptions.py` around lines 18 - 22, Update the docstring for
ConflictException to clarify its intended use versus AlreadyExistsException:
state that ConflictException (class ConflictException) represents generic 409
conflict scenarios (e.g., concurrent modification, resource state mismatches)
while AlreadyExistsException should be used specifically for duplicate/creation
conflicts (attempts to create a resource that already exists). Edit the
docstring on ConflictException and also add or adjust the docstring on
AlreadyExistsException to make the distinction explicit so reviewers can choose
the correct exception.
api/src/users/repository.py (1)

51-57: Consider documenting intent for unused current_user parameters.

The current_user parameter is unused in both methods. If it's reserved for future audit logging or authorization checks, a brief comment would clarify the intent.

Also applies to: 88-93

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/src/users/repository.py` around lines 51 - 57, The parameter current_user
is unused in addUserToWorkspaceWithRole (and the analogous method later in the
file); either remove it from the signature and update all call sites/tests, or
explicitly document and mark it as intentionally reserved for future
audit/authorization (e.g., add a one-line comment above
addUserToWorkspaceWithRole stating "current_user reserved for future
audit/authorization; intentionally unused" and optionally rename to
_current_user or add a linter ignore) so reviewers and linters understand the
intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/src/users/repository.py`:
- Around line 108-113: The deleteRolesForWorkspace function issues the DELETE
via self.session.execute but never commits, so the removal may not be persisted;
after the delete(WorkspaceUserRole)... await call add an await
self.session.commit() (mirroring
addUserToWorkspaceWithRole/removeUserFromWorkspace) to persist the transaction
and keep the method signature/return as None.

In `@api/src/workspaces/routes.py`:
- Around line 140-156: The workspace is committed by repository_ws.create before
calling repository_users.addUserToWorkspaceWithRole, so if the role assignment
fails the workspace is left inconsistent; wrap the addUserToWorkspaceWithRole
call in a try/except that catches NotFoundException (and other relevant
exceptions) and, on failure, perform a compensating delete of the created
workspace using the repository_ws delete/remove method (or other existing
cleanup API) and re-raise or return an appropriate error; ensure
evict_user_from_cache is only called after the role assignment succeeds (move it
inside the try) so cache is not evicted for a workspace that was rolled back.
- Around line 203-204: deleteRolesForWorkspace currently issues the DELETE but
never commits on its separate OSM session, so role deletions are rolled back;
fix by adding an explicit commit in the repository method (recommended) — update
users.repository.deleteRolesForWorkspace to call await self.session.commit() (or
the equivalent commit helper used by get_osm_session) after executing the
DELETE, otherwise if you prefer to keep repo unchanged call an explicit commit
on the OSM session from the route after await
repository_users.deleteRolesForWorkspace(workspace_id) (ensure you reference the
same session/commit API the repository uses) so deletions persist before calling
await repository_ws.delete(current_user, workspace_id).

In `@api/src/workspaces/schemas.py`:
- Around line 113-117: Docstring in the WorkspaceResponse class has a
typographical error ("requuest"); update the class docstring in schemas.py
(class WorkspaceResponse) to correct "requuest" to "request" so the description
reads "user making the request." Ensure only the docstring text is changed and
nothing else in the class definition or imports is modified.

---

Outside diff comments:
In `@api/src/teams/routes.py`:
- Line 1: The module uses HTTPException in several access-control checks
(occurring around lines referencing its use), but it isn't imported, causing a
NameError at runtime; fix this by adding HTTPException to the FastAPI imports at
the top (alongside APIRouter, Depends, status) so the symbol HTTPException is
available for the access-check code paths.

---

Nitpick comments:
In `@api/core/exceptions.py`:
- Around line 18-22: Update the docstring for ConflictException to clarify its
intended use versus AlreadyExistsException: state that ConflictException (class
ConflictException) represents generic 409 conflict scenarios (e.g., concurrent
modification, resource state mismatches) while AlreadyExistsException should be
used specifically for duplicate/creation conflicts (attempts to create a
resource that already exists). Edit the docstring on ConflictException and also
add or adjust the docstring on AlreadyExistsException to make the distinction
explicit so reviewers can choose the correct exception.

In `@api/src/users/repository.py`:
- Around line 51-57: The parameter current_user is unused in
addUserToWorkspaceWithRole (and the analogous method later in the file); either
remove it from the signature and update all call sites/tests, or explicitly
document and mark it as intentionally reserved for future audit/authorization
(e.g., add a one-line comment above addUserToWorkspaceWithRole stating
"current_user reserved for future audit/authorization; intentionally unused" and
optionally rename to _current_user or add a linter ignore) so reviewers and
linters understand the intent.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0c3573f7-969f-4c61-bbf9-a5defb44943d

📥 Commits

Reviewing files that changed from the base of the PR and between 3a38839 and d89b362.

📒 Files selected for processing (11)
  • api/core/exceptions.py
  • api/core/security.py
  • api/main.py
  • api/src/teams/repository.py
  • api/src/teams/routes.py
  • api/src/users/repository.py
  • api/src/users/routes.py
  • api/src/users/schemas.py
  • api/src/workspaces/repository.py
  • api/src/workspaces/routes.py
  • api/src/workspaces/schemas.py

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/core/security.py`:
- Around line 20-25: The cache currently keys only by `sub` (_user_info_cache)
so TDEI permission changes remain stale; change the cache key to include the
token issuance time/unique token identifier (e.g., `iat` or `jti`) so a newly
minted token bypasses the stale `UserInfo`. Update the lookup/insert logic that
currently references `_user_info_cache[sub]` to use a composite key like `(sub,
token_iat)` (or `(sub, token_jti)`) and ensure `_validate_token_uncached()`
continues to populate the cache under that composite key; adjust any callers
that fetch from the cache to pass the token claims used in the new key. If your
codebase uses a helper to build cache keys, update it and add comments noting
the reasoning.

In `@api/src/users/repository.py`:
- Around line 69-88: The current update-then-insert using WorkspaceUserRole with
self.session.execute(update(...)) then add/commit is racy; replace it with a
single atomic upsert using the DB's upsert facility (e.g. use
sqlalchemy.dialects.postgresql.insert(WorkspaceUserRole).on_conflict_do_update(...)
to set role=role for the (user_auth_uid, workspace_id) unique key) and execute
that via self.session.execute, or alternatively catch IntegrityError on commit
and retry the update inside the same transaction; target the existing
update(...) block and the WorkspaceUserRole insert path so the operation becomes
atomic and idempotent.

In `@api/src/users/routes.py`:
- Around line 36-51: The handler assign_member_role currently authorizes via
current_user.isWorkspaceLead(workspace_id) but does not verify the workspace
exists in the task DB; before calling user_repo.assign_member_role(...) call the
task/workspace repository's getById (the same guard used in the team routes) to
fetch the workspace by workspace_id and raise HTTPException(status_code=404,
detail="Workspace not found") if it returns None, then proceed to call
user_repo.assign_member_role(...) and evict_user_from_cache; reference
validate_token for auth, assign_member_role on UserRepository for the write, and
the task repo's getById used by team routes to locate the correct place to add
the guard.

In `@api/src/users/schemas.py`:
- Around line 42-43: SetRoleRequest currently allows assigning the "contributor"
role which should be read-only; update SetRoleRequest (the class and its role
field) to disallow "contributor" by either using a narrowed type that excludes
contributor or adding a runtime validation on SetRoleRequest.role (e.g., in a
validator or __post_init__) that raises an error when role ==
WorkspaceUserRoleType.CONTRIBUTOR, so clients cannot persist a no-op override;
reference the class name SetRoleRequest and the role attribute for the change.

In `@api/src/workspaces/routes.py`:
- Around line 202-203: The current sequence calls
repository_users.remove_all_member_roles(workspace_id) before
repository_ws.delete(current_user, workspace_id), which commits OSM role
removals before the workspace DB delete and can leave a live workspace with no
admins if the latter fails; to fix, either swap the calls so
repository_ws.delete(...) runs first and only then remove_all_member_roles(...),
or wrap both operations in a single transactional flow/compensating action
(e.g., capture the removed roles and restore them on delete failure) so that
failure of repository_ws.delete does not leave orphaned workspaces without
explicit admin roles; refer to repository_users.remove_all_member_roles and
repository_ws.delete to find the code to change.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 32070e07-a63f-461f-97e9-0662aeccde52

📥 Commits

Reviewing files that changed from the base of the PR and between d89b362 and ba419c7.

📒 Files selected for processing (11)
  • api/core/exceptions.py
  • api/core/security.py
  • api/main.py
  • api/src/teams/repository.py
  • api/src/teams/routes.py
  • api/src/users/repository.py
  • api/src/users/routes.py
  • api/src/users/schemas.py
  • api/src/workspaces/repository.py
  • api/src/workspaces/routes.py
  • api/src/workspaces/schemas.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • api/src/teams/repository.py
  • api/src/workspaces/repository.py
  • api/main.py

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
api/core/security.py (1)

142-147: Verify effective_role return type consistency with WorkspaceUserRoleType.

The method returns string literals ("lead", "validator", "contributor") rather than WorkspaceUserRoleType enum values. This works because WorkspaceUserRoleType is a StrEnum with matching values, but using the enum directly would provide type safety.

♻️ Optional refactor for type consistency
-    def effective_role(self, workspaceId: int) -> str:
+    def effective_role(self, workspaceId: int) -> WorkspaceUserRoleType:
         if self.isWorkspaceLead(workspaceId):
-            return "lead"
+            return WorkspaceUserRoleType.LEAD
         if self.isWorkspaceValidator(workspaceId):
-            return "validator"
-        return "contributor"
+            return WorkspaceUserRoleType.VALIDATOR
+        return WorkspaceUserRoleType.CONTRIBUTOR
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/core/security.py` around lines 142 - 147, The effective_role method
returns raw strings instead of WorkspaceUserRoleType enum members; update
effective_role to return WorkspaceUserRoleType values (e.g.,
WorkspaceUserRoleType.LEAD / .VALIDATOR / .CONTRIBUTOR) so callers get
enum-typed results and stronger type safety, ensuring WorkspaceUserRoleType is
imported where effective_role is defined and adjusting any call sites expecting
strings if needed.
api/src/users/routes.py (2)

52-56: Minor style inconsistency: prefer not over is False.

Line 34 uses not current_user.isWorkspaceContributor(...) while lines 52 and 75 use ... is False. Using not is more Pythonic and consistent.

♻️ Optional style fix
-    if current_user.isWorkspaceLead(workspace_id) is False:
+    if not current_user.isWorkspaceLead(workspace_id):
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/src/users/routes.py` around lines 52 - 56, Replace the non-idiomatic
boolean comparisons using "is False" with the Pythonic "not" form to keep style
consistent: change checks like current_user.isWorkspaceLead(workspace_id) is
False to not current_user.isWorkspaceLead(workspace_id) (and similarly update
any other occurrences such as current_user.isWorkspaceContributor(...) is False)
so the code uses "not" consistently.

68-82: Consider adding workspace existence check for consistency with assign_member_role.

The assign_member_role endpoint validates workspace existence (lines 58-62), but remove_member_role does not. While deleting orphaned role records is less harmful than creating them, adding the same guard would provide consistent behavior and clearer error messages.

♻️ Proposed fix for consistency
 `@router.delete`("/{user_id}", status_code=status.HTTP_204_NO_CONTENT)
 async def remove_member_role(
     workspace_id: int,
     user_id: UUID,
     current_user: UserInfo = Depends(validate_token),
     user_repo: UserRepository = Depends(get_user_repo),
+    workspace_repo: WorkspaceRepository = Depends(get_workspace_repo),
 ):
     if current_user.isWorkspaceLead(workspace_id) is False:
         raise HTTPException(
             status_code=status.HTTP_403_FORBIDDEN,
             detail="Must be a workspace owner to remove roles",
         )
 
+    await workspace_repo.getById(current_user, workspace_id)
+
     await user_repo.remove_member_role(workspace_id, user_id)
     evict_user_from_cache(str(user_id))
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/src/users/routes.py` around lines 68 - 82, Add the same workspace
existence guard used in assign_member_role: inject the workspace repo dependency
(e.g., WorkspaceRepository = Depends(get_workspace_repo)) into
remove_member_role, call the repo method used by assign_member_role to
fetch/verify the workspace (e.g., workspace_repo.get_workspace or
workspace_repo.exists_workspace(workspace_id)), and if not found raise
HTTPException(status_code=404, detail="Workspace not found"); then proceed with
current_user.isWorkspaceLead check and the call to user_repo.remove_member_role
and evict_user_from_cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@api/src/workspaces/routes.py`:
- Around line 202-203: Before calling repository_users.remove_all_member_roles
after await repository_ws.delete(current_user, workspace_id), first query and
capture the list of affected user IDs (the members whose roles will be removed)
so you can evict their caches; then, after the DB commit where roles were
deleted succeeds, call evict_user_from_cache(user_id) for each captured user to
invalidate their cached UserInfo and clear stale workspace permissions. Ensure
you reference repository_ws.delete and repository_users.remove_all_member_roles
to locate the deletion flow and use evict_user_from_cache from the security
module to perform the per-user eviction.
- Around line 145-155: The UserInfo cache key mismatch occurs because eviction
uses str(current_user.user_uuid) while the cache is keyed by the raw JWT "sub";
fix by adding a user_id: str field to the UserInfo dataclass and populate it
from the incoming token, then replace
evict_user_from_cache(str(current_user.user_uuid)) with
evict_user_from_cache(current_user.user_id) so eviction uses the exact cache
key; update any constructors/creators of UserInfo and code paths that build
UserInfo to set user_id accordingly (refer to UserInfo, current_user, and
evict_user_from_cache).

---

Nitpick comments:
In `@api/core/security.py`:
- Around line 142-147: The effective_role method returns raw strings instead of
WorkspaceUserRoleType enum members; update effective_role to return
WorkspaceUserRoleType values (e.g., WorkspaceUserRoleType.LEAD / .VALIDATOR /
.CONTRIBUTOR) so callers get enum-typed results and stronger type safety,
ensuring WorkspaceUserRoleType is imported where effective_role is defined and
adjusting any call sites expecting strings if needed.

In `@api/src/users/routes.py`:
- Around line 52-56: Replace the non-idiomatic boolean comparisons using "is
False" with the Pythonic "not" form to keep style consistent: change checks like
current_user.isWorkspaceLead(workspace_id) is False to not
current_user.isWorkspaceLead(workspace_id) (and similarly update any other
occurrences such as current_user.isWorkspaceContributor(...) is False) so the
code uses "not" consistently.
- Around line 68-82: Add the same workspace existence guard used in
assign_member_role: inject the workspace repo dependency (e.g.,
WorkspaceRepository = Depends(get_workspace_repo)) into remove_member_role, call
the repo method used by assign_member_role to fetch/verify the workspace (e.g.,
workspace_repo.get_workspace or workspace_repo.exists_workspace(workspace_id)),
and if not found raise HTTPException(status_code=404, detail="Workspace not
found"); then proceed with current_user.isWorkspaceLead check and the call to
user_repo.remove_member_role and evict_user_from_cache.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4425b0af-4edd-4513-994c-8dc4e2ffb20a

📥 Commits

Reviewing files that changed from the base of the PR and between ba419c7 and 2a0e712.

📒 Files selected for processing (11)
  • api/core/exceptions.py
  • api/core/security.py
  • api/main.py
  • api/src/teams/repository.py
  • api/src/teams/routes.py
  • api/src/users/repository.py
  • api/src/users/routes.py
  • api/src/users/schemas.py
  • api/src/workspaces/repository.py
  • api/src/workspaces/routes.py
  • api/src/workspaces/schemas.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • api/main.py
  • api/src/teams/repository.py

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (2)
api/src/workspaces/routes.py (2)

152-156: ⚠️ Potential issue | 🟠 Major

Evict the creator by auth UID, not user_uuid.

evict_user_from_cache() is keyed by auth UID (api/core/security.py:46-54), and this file already uses member.auth_uid for eviction on Lines 206-207. Using current_user.user_uuid here will miss the cache entry whenever the auth identifier and OSM user UUID differ, so the creator can keep stale permissions after workspace creation. Use the same auth-UID field from UserInfo that backs the cache key here as well.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/src/workspaces/routes.py` around lines 152 - 156, The eviction call uses
the wrong identifier: change the evict_user_from_cache call to pass the
creator's auth UID (the same field used elsewhere as member.auth_uid) instead of
current_user.user_uuid so it matches the cache key; update the call in the
workspace creation flow (evict_user_from_cache) to use current_user.auth_uid
(UserInfo.auth_uid) to ensure the creator's cache entry is removed.

203-207: ⚠️ Potential issue | 🟠 Major

Invalidate every removed member, not only the privileged subset.

remove_all_member_roles() deletes all WorkspaceUserRole rows (api/src/users/repository.py:103-109), but the eviction list comes from get_privileged_workspace_members(...). Any non-privileged users with explicit workspace roles can keep stale cached access until the cache TTL expires after the workspace is deleted. Capture all affected auth UIDs before the delete and evict that full set after the OSM commit succeeds.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/src/workspaces/routes.py` around lines 203 - 207, Capture the full set of
workspace members before deleting the workspace and removing roles instead of
only calling get_privileged_workspace_members; i.e., call the repository method
that returns all workspace members (e.g., repository_users.get_workspace_members
or get_all_workspace_members) to collect auth_uids, then perform
repository_ws.delete(current_user, workspace_id) and
repository_users.remove_all_member_roles(workspace_id), and only after those
operations succeed iterate the previously captured members and call
evict_user_from_cache(UUID(member.auth_uid)) for each to ensure all removed
members are evicted.
🧹 Nitpick comments (1)
api/src/users/routes.py (1)

28-40: Consider adding workspace existence check for consistency.

The PUT and DELETE endpoints call workspace_repo.getById() to verify the workspace exists before proceeding. This GET endpoint only checks isWorkspaceContributor(), which could theoretically succeed for a deleted workspace if cached user permissions haven't been evicted yet.

For consistency with the mutation endpoints, consider adding the same workspace validation. Though this is lower risk since it's a read operation.

♻️ Proposed fix
 `@router.get`("", response_model=list[WorkspaceUserRoleItem])
 async def get_privileged_workspace_members(
     workspace_id: int,
     current_user: UserInfo = Depends(validate_token),
     user_repo: UserRepository = Depends(get_user_repo),
+    workspace_repo: WorkspaceRepository = Depends(get_workspace_repo),
 ):
     if not current_user.isWorkspaceContributor(workspace_id):
         raise HTTPException(
             status_code=status.HTTP_403_FORBIDDEN,
             detail="Project group membership required to view members",
         )
 
+    # Ensure the workspace exists
+    await workspace_repo.getById(current_user, workspace_id)
+
     return await user_repo.get_privileged_workspace_members(workspace_id)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@api/src/users/routes.py` around lines 28 - 40, Add a workspace existence
check to get_privileged_workspace_members similar to the PUT/DELETE endpoints:
inject WorkspaceRepository via a dependency (e.g. workspace_repo:
WorkspaceRepository = Depends(get_workspace_repo)), call await
workspace_repo.getById(workspace_id) at the start of
get_privileged_workspace_members, and if it returns None raise
HTTPException(status_code=404, detail="Workspace not found") before performing
current_user.isWorkspaceContributor and returning
user_repo.get_privileged_workspace_members(workspace_id).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@api/src/workspaces/routes.py`:
- Around line 152-156: The eviction call uses the wrong identifier: change the
evict_user_from_cache call to pass the creator's auth UID (the same field used
elsewhere as member.auth_uid) instead of current_user.user_uuid so it matches
the cache key; update the call in the workspace creation flow
(evict_user_from_cache) to use current_user.auth_uid (UserInfo.auth_uid) to
ensure the creator's cache entry is removed.
- Around line 203-207: Capture the full set of workspace members before deleting
the workspace and removing roles instead of only calling
get_privileged_workspace_members; i.e., call the repository method that returns
all workspace members (e.g., repository_users.get_workspace_members or
get_all_workspace_members) to collect auth_uids, then perform
repository_ws.delete(current_user, workspace_id) and
repository_users.remove_all_member_roles(workspace_id), and only after those
operations succeed iterate the previously captured members and call
evict_user_from_cache(UUID(member.auth_uid)) for each to ensure all removed
members are evicted.

---

Nitpick comments:
In `@api/src/users/routes.py`:
- Around line 28-40: Add a workspace existence check to
get_privileged_workspace_members similar to the PUT/DELETE endpoints: inject
WorkspaceRepository via a dependency (e.g. workspace_repo: WorkspaceRepository =
Depends(get_workspace_repo)), call await workspace_repo.getById(workspace_id) at
the start of get_privileged_workspace_members, and if it returns None raise
HTTPException(status_code=404, detail="Workspace not found") before performing
current_user.isWorkspaceContributor and returning
user_repo.get_privileged_workspace_members(workspace_id).

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5bc62fd5-8237-470b-8e1a-098c0b4656eb

📥 Commits

Reviewing files that changed from the base of the PR and between 2a0e712 and 583ad8d.

📒 Files selected for processing (11)
  • api/core/exceptions.py
  • api/core/security.py
  • api/main.py
  • api/src/teams/repository.py
  • api/src/teams/routes.py
  • api/src/users/repository.py
  • api/src/users/routes.py
  • api/src/users/schemas.py
  • api/src/workspaces/repository.py
  • api/src/workspaces/routes.py
  • api/src/workspaces/schemas.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • api/src/teams/repository.py
  • api/main.py

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant