feat(user_roles): Add User Roles/Permissions #276
Conversation
…hatbot into feat_appsync_newer
removed unused API resources
correctly replacing api_handler function for better code diff
|
@flamingquaks #269 has been merged |
|
I'm currently working through merge conflict resolution and related patches. I aim to have this ready within the next few days |
|
Waiting for this PR to merge.. |
We are working through some bugs that came up with the latest updates. Once those are resolved, the PR will move forward. Hoping to have them wrapped quickly! |
This comment was marked as resolved.
This comment was marked as resolved.
…roups for in-app permission management
|
This feature is now READY FOR REVIEW! Overview of major functionality changes:
It's recommended that an upgraded deployment and a fresh deployment are attempted. A full-sweep of functionality tests is recommended, including adjusting user permissions (you will need to logout/login to app after user group update) |
| response = genai_core.admin_user_management.delete_user(email) | ||
| if response: | ||
| return True | ||
| else: |
There was a problem hiding this comment.
else not needed here
| return True | ||
| else: | ||
| return ( | ||
| "The user is not disabled. To delete a user, first disable the user, then delete the user.", |
There was a problem hiding this comment.
can we safely assume that this is the case each time response is False?
| GroupName=role, | ||
| ) | ||
| return True | ||
| else: |
| UserPoolId=COGNITO_USER_POOL_ID, GroupName="chatbot_admin" | ||
| )["Users"] | ||
| users = [] | ||
| for user in response["Users"]: |
There was a problem hiding this comment.
I think you can rewrite this loop to be more dynamic and less verbose considering the following:
-
if you need to
snake_caseall attributes simply have a functionsnake_case, loop over all attributes keys and call it - instead of hardcoded if statements. -
have a list of roles sorted by importance, loop over it,
if username in ...and the first match will be the user role, also see if this function to derive role needs to be shared around.. -
let's use enums for roles from
permissionsinstead of hardcoded strings
| ) | ||
| if response["ResponseMetadata"]["HTTPStatusCode"] == 200: | ||
| user_data = {} | ||
| for attribute in response["UserAttributes"]: |
There was a problem hiding this comment.
same as above
....shared function to derive_actual_role?
| if user_details_response["Enabled"] == False: | ||
| idp.admin_delete_user(UserPoolId=COGNITO_USER_POOL_ID, Username=email) | ||
| return True | ||
| else: |
|
|
||
| def update_user_details(current_email, **kwargs): | ||
| attributes = [] | ||
| if "name" in kwargs and len(kwargs["name"]) > 0: |
There was a problem hiding this comment.
shorter version if kwargs.get("name") or if len(kwargs.get("name", '')) (same for the below ones)
| ) | ||
| if response["ResponseMetadata"]["HTTPStatusCode"] == 200: | ||
| return True | ||
| else: |
| return self.WORKSPACES_USER_ROLE | ||
| elif self.CHATBOT_USER_ROLE in user_groups: | ||
| return self.CHATBOT_USER_ROLE | ||
| else: |
|
Neat PR |
|
I tested these changes and they worked as expected, good work |
|
I've shifted this back to Draft to allow time to refactor this work to use Amazon Verified Permissions. This will allow future access controls and permissions to be added more consistently. |
|
@flamingquaks have you considered the cost implication of shifting the draft to AVP as the only option? Whats not working well with the current draft? Just some things to keep in mind |
|
Thanks @Amrib24 for raising this concern, but the cost of verified permissions is negligible compared to the running costs of an LLM based solution. Moreover, Verified Permissions provides out of the box, managed capabilities that you would need to account engineering and maintenance cost for in case you choose another solution. |
|
@flamingquaks, are you still planning on refactoring this PR to use Amazon Verified Permissions (AVP)? Please let us know if there are any blockers or challenges the team could help you with! Excited about this feature. :) |
|
@ystoneman, Apologies for the delay, it took me a bit more time to upskill on AVP than I had planned. I'm aiming to pick up steam on this by end of week and provide an update on timeline. |
This PR is for work on Issue #99 - Please see issue for detailed documentation of changes to user interface
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.