Conversation
barborico
left a comment
barborico
left a comment
There was a problem hiding this comment.
Reviewed schemas; will continue with the rest tomorrow!
| description=resolved_description, | ||
| ) | ||
|
|
||
| created_group = CreateGroup( |
There was a problem hiding this comment.
nit: It would be good for us to be able to tie the "group created" audit log to the corresponding request ID.
| requester = fields.Nested(lambda: OktaUserSchema) | ||
| active_requester = fields.Nested(lambda: OktaUserSchema) | ||
| resolver = fields.Nested(lambda: OktaUserSchema) | ||
| active_resolver = fields.Nested(lambda: OktaUserSchema) |
There was a problem hiding this comment.
I see this pattern is used elsewhere, but I don't really understand it. What's the purpose of having two user properties each for requester and resolver? Aren't they gonna be the same if the user isn't deleted, and one will be null otherwise?
| ) | ||
|
|
||
|
|
||
| class GroupRequestSchema(SQLAlchemyAutoSchema): |
There was a problem hiding this comment.
One thing that appears to be missing from the group request schema but is present when creating app groups directly is plugin config (a JSON blob). Seems like that should be included here, e.g. if someone is requesting to create a group for an app with a plugin configured.
| active_requester = fields.Nested(lambda: OktaUserSchema) | ||
| resolver = fields.Nested(lambda: OktaUserSchema) | ||
| active_resolver = fields.Nested(lambda: OktaUserSchema) | ||
| approved_group = fields.Nested(lambda: OktaGroup) |
There was a problem hiding this comment.
nit: created_group may be clearer
| "requested_group_type", | ||
| "requested_app_id", | ||
| "requested_group_tags", | ||
| "requested_ownership_ending_at", |
There was a problem hiding this comment.
Why not just leave the ownership indefinite, and then they can always modify ownership after the group is created? Seems like it may be confusing.
| status = fields.Enum(AccessRequestStatus, load_only=True) | ||
| requester_user_id = fields.String(load_only=True) | ||
| requested_group_name = fields.String(load_only=True) | ||
| assignee_user_id = fields.String(load_only=True) |
There was a problem hiding this comment.
Isn't the assignee the same as the requestor? Seems like this could be omitted, or replaced with the associated app.
2 participants
Description
This PR introduces the concept of 'Group Creation Requests', sometimes referred to in the code as Group Requests. This will allow users to, as the name suggests, request that a group is created.
The user provides the desired group:
This request is then created so that Access admins (and app owners if the requested group is an app group) can approve/reject it. Users may also reject their own requests.
Additional changes have been added to the plugins and audit logging to support the new request type
Notes
A few other bug fixes and circular import fixes added in as well (eg. in modify_group_tags.py)
How to review
Unfortunately, this PR is massive and not easily broken up. Probably start with models and schemas in case any changes need to be made there (which will affect the rest of the code), then maybe test cases to see if there's any edge cases missing or the behavior is unexpected, then operations, then the rest?
TODO after initial review
TODO in follow up PRs: