Finish implementation of stubbed roles APIs#4
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a users domain (schemas, repository, routes) for workspace role management, migrates token cache to user-UUID keyed caching with jti rotation and eviction, removes workspace-level user-role types/methods, updates workspace/team endpoints to use the new users repo, registers the users router, and adds a ConflictException (409). ChangesUsers & Auth refactor
Sequence DiagramsequenceDiagram
participant Client
participant API
participant Auth as ValidateToken
participant UserRepo as UserRepository
participant DB as Database
Client->>API: PUT /workspaces/{id}/users/{user_id}/role
API->>Auth: validate_token()
Auth->>Auth: decode JWT, extract sub (user_uuid) and jti
Auth->>Auth: check _user_info_cache[user_uuid]
alt cache miss or jti mismatch
Auth->>DB: fetch user info / roles by user_uuid
DB-->>Auth: user info (includes token_jti)
Auth->>Auth: store _user_info_cache[user_uuid]
end
Auth-->>API: return UserInfo
API->>API: check current_user.isWorkspaceLead(workspace_id)
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-->>API: evicted
API-->>Client: 204 No Content
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
api/core/exceptions.py (1)
18-22: Consider documenting the distinction fromAlreadyExistsException.Both
ConflictExceptionandAlreadyExistsExceptionuse HTTP 409. Consider updating the docstrings to clarify when each should be used (e.g.,AlreadyExistsExceptionfor duplicate creation attempts vs.ConflictExceptionfor 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 unusedcurrent_userparameters.The
current_userparameter 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
📒 Files selected for processing (11)
api/core/exceptions.pyapi/core/security.pyapi/main.pyapi/src/teams/repository.pyapi/src/teams/routes.pyapi/src/users/repository.pyapi/src/users/routes.pyapi/src/users/schemas.pyapi/src/workspaces/repository.pyapi/src/workspaces/routes.pyapi/src/workspaces/schemas.py
cyrossignol
force-pushed
the
roles
branch
2 times, most recently
from
f22fba5 to
ba419c7
Compare
There was a problem hiding this comment.
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
📒 Files selected for processing (11)
api/core/exceptions.pyapi/core/security.pyapi/main.pyapi/src/teams/repository.pyapi/src/teams/routes.pyapi/src/users/repository.pyapi/src/users/routes.pyapi/src/users/schemas.pyapi/src/workspaces/repository.pyapi/src/workspaces/routes.pyapi/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
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
api/core/security.py (1)
142-147: Verifyeffective_rolereturn type consistency withWorkspaceUserRoleType.The method returns string literals (
"lead","validator","contributor") rather thanWorkspaceUserRoleTypeenum values. This works becauseWorkspaceUserRoleTypeis aStrEnumwith 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: prefernotoveris False.Line 34 uses
not current_user.isWorkspaceContributor(...)while lines 52 and 75 use... is False. Usingnotis 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 withassign_member_role.The
assign_member_roleendpoint validates workspace existence (lines 58-62), butremove_member_roledoes 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
📒 Files selected for processing (11)
api/core/exceptions.pyapi/core/security.pyapi/main.pyapi/src/teams/repository.pyapi/src/teams/routes.pyapi/src/users/repository.pyapi/src/users/routes.pyapi/src/users/schemas.pyapi/src/workspaces/repository.pyapi/src/workspaces/routes.pyapi/src/workspaces/schemas.py
🚧 Files skipped from review as they are similar to previous changes (2)
- api/main.py
- api/src/teams/repository.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
api/src/workspaces/routes.py (2)
152-156:⚠️ Potential issue | 🟠 MajorEvict 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 usesmember.auth_uidfor eviction on Lines 206-207. Usingcurrent_user.user_uuidhere 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 fromUserInfothat 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 | 🟠 MajorInvalidate every removed member, not only the privileged subset.
remove_all_member_roles()deletes allWorkspaceUserRolerows (api/src/users/repository.py:103-109), but the eviction list comes fromget_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 checksisWorkspaceContributor(), 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
📒 Files selected for processing (11)
api/core/exceptions.pyapi/core/security.pyapi/main.pyapi/src/teams/repository.pyapi/src/teams/routes.pyapi/src/users/repository.pyapi/src/users/routes.pyapi/src/users/schemas.pyapi/src/workspaces/repository.pyapi/src/workspaces/routes.pyapi/src/workspaces/schemas.py
🚧 Files skipped from review as they are similar to previous changes (2)
- api/src/teams/repository.py
- api/main.py
jeffmaki
previously requested changes
Mar 19, 2026
cyrossignol
dismissed
jeffmaki’s stale review
Changes addressed--merging for Mahesh
cyrossignol
added a commit
to TaskarCenterAtUW/workspaces-frontend
that referenced
this pull request
May 19, 2026
This sets up the UI for role management and integrates access controls into the Workspaces frontend. Backend PR: TaskarCenterAtUW/workspaces-backend#4 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Members page in workspace settings for viewing/managing project-group admins, data generators, and workspace members; leads can change member roles and assign/remove roles via Workspaces API. * Permission-aware export flow to TDEI with selectable eligible project groups and project-group selection UI integration. * New composable useWorkspaceRole() exposing role, isLead, and isValidator to gate UI and actions across the app. * New Workspaces API methods: getWorkspaceMembers(id), assignRole(id, userUuid, role), removeRole(id, userUuid). * New TDEI user APIs: getMyProjectGroups(), getMyRolesForProjectGroup(projectGroupId, pgName), getProjectGroupUsers(projectGroupId). * **UI/UX Improvements / Access Controls** * Site-wide role-aware UI gating: many settings panels (General, Imagery, Apps, Delete, Teams) and actions (create team, delete workspace, rename, save settings) now show informational alerts and disable inputs/buttons for non-leads. * Dashboard and workspace lists show a new "My Role" row and role badges (Owner/Lead, Validator, Member, POC, Data Generator) in DetailsTable, WorkspaceItem, and related components. * Review toolbar and feedback controls gated by validator/lead role logic. * Settings navigation updated to include a "Members" item; team items and team dialogs respect lead gating. * **Types & Utilities** * New types: WorkspaceRole ('lead' | 'validator' | 'contributor'), WorkspaceMember, TdeiProjectGroup, TdeiUserItem, TdeiRoleAssignment. * New util ROLE_LABELS mapping WorkspaceRole to display labels. * ProjectGroupPicker updated to use tdei_project_group_id keys and accept options prop; ProjectGroup types and compare sorting used. * **Service & Backend Integration** * services/workspaces.ts migrated various endpoints to a new API wrapper and added workspace member management methods. * services/tdei.ts expanded TDEI user client surface to return typed project groups and users and added role-query helpers. * Frontend pages/components updated to fetch and use TDEI project groups and roles for eligibility and UI defaults (export flow, dashboard grouping). * **Notable Component Changes** * New pages/workspace/[id]/settings/members.vue added (major new file). * pages/dashboard.vue: reworked to model currentProjectGroup/currentWorkspace with typed bindings and pass currentWorkspaceTdeiRoles to details table. * pages/workspace/[id]/export/tdei.vue: adds project group selection, eligibility checks, and permission messaging. * Many components now consume useWorkspaceRole() and conditionalize UI and actions on isLead/isValidator. Overall, this PR implements the initial permissions/roles system end-to-end: types and service methods, a composable for role derivation, UI wiring across dashboard and workspace settings, a members management page, and permission-aware flows for TDEI export and settings management. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
2 participants
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.
This fleshes-out the role management API and access controls and includes a mechanism for evicting
UserInfoobjects from the cache when permissions change.Frontend PR: TaskarCenterAtUW/workspaces-frontend#44
New Features
Access control / behavior changes
Caching & authentication
Repository changes
Error handling
Other