Add role owner view for expiring roles page #300
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -157,10 +158,16 @@ export default function NavItems(props: NavItemsProps) { | |||
| <List disablePadding> | |||
| <ListItemLink | |||
| to="/expiring-roles?owner_id=@me" | |||
| displayText="Owned by Me" | |||
| displayText="Owned Groups" | |||
There was a problem hiding this comment.
I believe the difference is:
/expiring-groups?owner_id=@meis for group owners with individuals with expiring ownership/membership/expiring-roles?owner_id=@meis for role owners with groups with expiring ownership/membership
There was a problem hiding this comment.
ah so something like "groups owned by a role I own"? I don't want to block on this but thanks for clarifying
There was a problem hiding this comment.
I had a little trouble figuring out how to explain it in like less than 15ish characters (so it fit in the menu bar) but essentially :
/expiring-groups?owner_id=@me shows a group owner when individuals' access to groups they own are expiring.
/expiring-roles?owner_id=@me shows a group owner when roles' access to groups they own are expiring.
/expiring-roles?role_owner_id=@me shows a role owner_ when roles they own will be losing access to a group soon
There was a problem hiding this comment.
Changing 'Owned by Me' under Expiring Roles to 'Owned Groups' and 'Owned Roles' was meant to differentiate the two cases since both involve ownership. I left 'Owned by Me' under Expiring Groups alone since that seemed clear enough there
1b725c0
a1f886e to
1b725c0
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a view and API support for role owners to see and request renewal of expiring role access.
- Introduces a new
role_owner_idquery parameter throughout frontend and backend. - Adds a “Owned Roles” nav link and integrates
CreateRoleRequestbuttons in the Expiring Roles page. - Extends the audit endpoint, schema, and Swagger spec to filter by
role_owner_id.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pages/roles/Expiring.tsx | Added role_owner_id state, query handling, and request button |
| src/pages/role_requests/Create.tsx | Refined permission logic and updated group prop types |
| src/components/NavItems.tsx | Added "Owned Roles" navigation item |
| src/api/apiComponents.ts | Added role_owner_id to GetGroupRoleAuditsQueryParams |
| api/views/schemas/pagination.py | Included role_owner_id in pagination request schema |
| api/views/resources/audit.py | Implemented role_owner_id filtering in audit resource |
| api/swagger.json | Added role_owner_id parameter to Swagger spec |
Comments suppressed due to low confidence (1)
src/pages/role_requests/Create.tsx:522
- The new logic enforces role-based permission but removed the original group-manager check, which means group requests may now render for unauthorized users. Restore a branch for
(props.group != null && canManageGroup(...))to keep hiding requests for groups the user manages.
(props.role != null && !canManageGroup(props.currentUser, props.role)) ||
| @@ -402,6 +406,16 @@ export default function ExpiringRoless() { | |||
| rereview={row.should_expire} | |||
| /> | |||
| </TableCell> | |||
| ) : roleOwnerId || canManageGroup(currentUser, row.role_group) ? ( | |||
There was a problem hiding this comment.
The conditional shows the CreateRoleRequest button whenever any role_owner_id is present or the user can manage the group, rather than checking if the current row’s role is actually owned by the user. Update it to roleOwnerId && row.is_owner (or similar) to ensure the button only appears for roles the user owns.
| ) : roleOwnerId || canManageGroup(currentUser, row.role_group) ? ( | |
| ) : (roleOwnerId && row.is_owner) || canManageGroup(currentUser, row.role_group) ? ( |
Copilot uses AI. Check for mistakes.
| # https://stackoverflow.com/questions/4186062/sqlalchemy-order-by-descending#comment52902932_9964966 | ||
| query = query.filter( | ||
| RoleGroupMap.role_group_id.in_( | ||
| [o.group_id for o in owner_role_ownerships.with_entities(OktaUserGroupMember.group_id).all()] |
There was a problem hiding this comment.
Loading all owner role IDs into Python and then filtering can be inefficient for large datasets. Consider using a subquery—e.g., owner_role_ownerships.with_entities(...).subquery()—and pass that directly into .in_() to let the database handle filtering.
| [o.group_id for o in owner_role_ownerships.with_entities(OktaUserGroupMember.group_id).all()] | |
| owner_role_ownerships.with_entities(OktaUserGroupMember.group_id).subquery() |
Copilot uses AI. Check for mistakes.
Prior to having role requests, it didn't make much sense to have a view under Expiring Roles that would show role owners the access expiring soon for roles they own because we could not make that knowledge actionable.
This new view shows role owners the roles they own with access expiring soon along with the ability to make a role request for that expiring access from that page (similar to how the 'My Access' tab under Expiring Groups allows users to make access requests for their expiring access).
Related to #299 (adding role owner expiring access notifications)