Assign HQ-defined roles to CCA user #59

Charl1996 · 2024-08-29T12:58:24Z

This PR expands on the DomainSyncUtil class's sync_domain_role method to now consider the roles for a particular user as defined on CommCare HQ. This PR is for returning the CCA roles for a particular user.

Note: This work will go through QA and is already on staging.

Approach

The approach is slightly different than the spec suggests in the following ways:

We now create only 1 role for a user per domain which has the correct permissions defined as per CommCare HQ. The role's name is composed here and is identifiable by domain and user.
When the user's permissions change on HQ and the user logs in again, the permissions for the role is overwritten with the new permissions.

By having only 1 role per domain user and setting the permissions for that role we won't end up having redundant roles that we'll need to clean up.

Side note

Since we're not assigning the Gamma by default to all users anymore (it's a configurable role on HQ now), it was found that the CCA user cannot log in to Superset since the privilege of having a profile on Superset is a permission the user needs. If the user does not have this permission, the user's browser will redirect endlessly resulting in a browser error.

To account for this a new standard role, namely hq_user, is assigned to all CCA users, regardless of their permissions on CommCare HQ. This role has the necessary ("baseline") permissions needed for any users to be able have a profile on Superset/ be able to log in to Superset.

This means that users will need at least 3 distinct roles if they want to be able to access a domain's data on Superset:

HQ user role (new): The role that grants the user permission to be able to have a profile on Superset.
Domain schema role (existed before this PR): This role grants (and restricts) the user's access to a particular domain.
Domain user role (new): This role contains the necessary configurable permissions as defined on CommCare HQ and governs how the user can behave on the particular domain.

In addition to the above roles the user is able to have more roles if so defined on CommCare HQ. The roles available on HQ is currently Gamma, dataset_editor and sql_lab.

Add menu permissions and do small refactor

Fix tests

mkangia · 2024-08-30T09:12:11Z

Hey @Charl1996

Can you link the related HQ PR in the description please?
dimagi/commcare-hq#34987

Charl1996 · 2024-08-30T09:15:12Z

Hey @Charl1996

Can you link the related HQ PR in the description please? dimagi/commcare-hq#34987

Done

mkangia

Beautiful PR @Charl1996

I enjoyed reviewing this. 💯

Are we taking this through QA?

mkangia · 2024-08-30T09:18:06Z

hq_superset/utils.py

@@ -163,6 +144,26 @@ def sync_domain_role(self, domain):
        self.sm.get_session.add(current_user)
        self.sm.get_session.commit()

+    @staticmethod


mkangia · 2024-08-30T09:51:19Z

hq_superset/oauth.py

@@ -56,6 +56,15 @@ def set_oauth_session(self, provider, oauth_response):
        # }
        session[SESSION_OAUTH_RESPONSE_KEY] = oauth_response

+    def set_role_permissions(self, role, permissions):


nit: its confusing that this is in the OAuth file. Isn't there a better place for this addition?

I was also thinking it feels a little out of place, but then all the permissions-related functionality sits in the class this class inherits from (methods like add_role, add_view_menu_permission, etc.), and since this is a permissions-related method I thought it's probably best putting it here. Any other suggestions are welcome.

(This class is the security manager, which handles the roles, permissions etc.)

mkangia · 2024-08-30T09:56:29Z

hq_superset/const.py

+    CAN_DELETE_PERMISSIONS,
+]
+
+READ_ONLY_MENU_PERMISSIONS = {


As I read this now, I believe at certain point we could see HQ setting read specifically for either of Chart, Dataset, Dashboard etc.
It does appear that, the way this is currently structured, that should be feasible. Right?

Correct. I played around a lot with how to best separate the read and write whilst keeping duplication to a minimum, but this current way of doing it feels like the most granular and configurable approach. You can set specific permissions for specific view/menu's.

mkangia · 2024-08-30T10:02:15Z

superset_config.example.py

-# Any other additional roles to be assigned to the user on top of the base role
-# Note: by design we cannot use AUTH_USER_REGISTRATION_ROLE to
-# specify more than one role
-AUTH_USER_ADDITIONAL_ROLES = ["sql_lab"]


Hey @Charl1996

How do we roll this change out? Is there anything specifically needed for existing users? or their permission would reset correctly when they login.
So, will this role get removed from their account automatically now, if they had it earlier?

How does platform role work along with domain role? If someone gets the role sql_lab, it would be applicable only for the project they are currently on? and when they switch project, their platform roles are fetched and updated?

or their permission would reset correctly when they login.

Yes, when they log in their permissions will reset correctly.

So, will this role get removed from their account automatically now, if they had it earlier?

Correct. My thoughts are that if a user needs access they need to now have it explicitly granted on HQ (even if it means they might not have that permissions until it's explicitly given to them on HQ if they had it before), but maybe I can check with AEs as well.

How does platform role work along with domain role? If someone gets the role sql_lab, it would be applicable only for the project they are currently on?

Ah! Brilliant question! See response below.

and when they switch project, their platform roles are fetched and updated?

More accurately, fetched and overwritten.

With the way it works in this PR, when a user logs in their user on CCA will be assigned roles/permissions based on only the domain's configuration they logged in to, so if their user on that domain on HQ does not have the Gamma role for instance, the user on CCA will not be granted that role when logging in. This is true even though the user might have Gamma on another domain; when they log into that other domain they will be given the Gamma role. So even though the platform role is technically platform wide, the user will only ever be able to have that role on the domain which gives it to the user. This is true because every time a user logs in we overwrite the user's roles completely with the appropriate roles for the domain the user logs into.

Correct. My thoughts are that if a user needs access they need to now have it explicitly granted on HQ (even if it means they might not have that permissions until it's explicitly given to them on HQ if they had it before), but maybe I can check with AEs as well.

Right. It seems if we roll this out, people would lose access, at least temporarily, till they are assigned the permission on HQ.
The good thing is that the permissions setup is already available on HQ. So we can ask projects to set them even before we release this.

Additionally, we should be explicit that the permissions are updated only at login and when switching domains so they need to re-login to see the updated permissions from HQ.

permissions are updated only at login

This is not entirely true; the permissions are updated when selecting a domain, so even though the user is "logged in" to superset, when they change domains the permissions will update (sorry, maybe I wasn't too clear on this distinction in my previous comment)

Right, that is what I meant by "only at login and when switching domains"?

may be a correction to that "they need to re-login or re-select their domain" (I don't know if they can do that, if they have only one domain, which would be for most users?)

Oh! Sorry, yeah, now I understand what you meant. You're right. Although there is a way when logged into the app to go to the "select domains" page without having to explicitly log out and re-auth.

Cool.

And we need to plan for the rollout?

I've checked with AEs and it's OK that users might loose temporary access if their HQ permissions isn't set up correctly; we just have to make sure to communicate this change when deploying the new changes.

Charl1996 · 2024-08-30T10:39:40Z

Are we taking this through QA?

Yes. I updated the description to make it more visible.

mkangia

looking forward to QA on this.

Charl1996 · 2024-09-10T10:21:24Z

hq_superset/hq_url.py

@@ -32,5 +32,5 @@ def datasource_unsubscribe(domain, datasource_id):
    )


-def user_domain_roles(username, domain):


HQ will now get the user from the request.

1. Users with 'edit' permissions gets the superset Gamma role 2. If HQ user don't have view permissions, don't allow access

Charl1996 · 2024-10-28T13:27:03Z

Please note that an update has been made which basically does the following:

If a HQ user has "edit" permissions set for CCA, assign the superset Gamma role
If a HQ user does not have "view" or "edit" permissions set for CCA, the user cannot access CCA.

hq_superset/tests/test_utils.py

mkangia · 2024-10-28T21:03:32Z

hq_superset/views.py

-        DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(hq_domain)
+        if not DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(hq_domain):
+            flash(
+                "You don't have the necessary HQ permissions to access this domain.",


nit: might be useful to add the name of the domain to the message so the user knows which domain they were trying to access?

mkangia · 2024-10-30T09:04:47Z

hq_superset/utils.py

+        additional_roles = []
+        if domain_permissions[CAN_WRITE_PERMISSION]:
+            platform_roles_names.append(
+                'gamma',


should this be the one with "G"? The one on superset is "Gamma" or "gamma"?
Extracted it to a constant would be helpful to avoid confusion

Extracted it to a constant would be helpful to avoid confusion

Good idea

mkangia · 2024-10-30T09:08:44Z

hq_superset/utils.py

+
+    @staticmethod
+    def _domain_user_role_name(domain, user):
+        return f"{domain}_user_{user.id}_read_only"


Hey @Charl1996

I wanted to check with you if its possible to add the HQ role UUID to the name?
While looking at the ticket to expire permissions after every hour, I realized we currently don't a way of linking HQ role to the CCA role, which could be useful.
If its not a big lift, would be good to do it now since we are releasing this now.

I don't think it would be that much of a big lift. I'll look into it.

Awesome, thank you!

@mkangia I had a look into this and hit a snag...the Admin role is a virtual role and is never persisted in the database, so if the user is admin we don't have a role ID that we could use.

Maybe we should rethink how we track the role linkage. I'm going to hold off on this for now.

Actually, thinking of it, why do we need to keep track of the role? We know who the user is and we know the domain, so we can just check if the user has CCA access on the domain (which is what we do on log-in to a specific domain).

So whenever it's needed to sync the role again you should just be able to call DomainSyncUtil.sync_domain_role(domain) and the rest will sort itself out.

if the user is admin we don't have a role ID that we could use.

so the id could be "admin"?

I'm going to hold off on this for now.

We don't need to block on this. Fine to go ahead without the role id if needed.

mkangia · 2024-10-31T15:45:19Z

All looks good here except this point that I believe will add redundant roles in CCA
#59 (comment)

mkangia · 2024-11-03T21:56:06Z

Hey @Charl1996

Can you confirm what happens if HQ does not respond well or say responds with a 500 when fetching roles/permissions?
I had this happening locally and for some reason CCA continued to function which was unexpected.

Charl1996 · 2024-11-04T13:04:04Z

@mkangia Here CCA simply returns empty permissions if the status is not 200, which in turn will return an empty list of additional roles. This will exit and show this message to the user.

I'm curious at what point you got an 500? If HQ returned it, you should have seen the error message, but if CCA have thrown it I think you should see the 500 status page.

mkangia · 2024-11-04T16:42:34Z

Hey @Charl1996
Due to recent api changes that broke the integration, I was getting 500 from HQ, locally, that is where I noticed the pattern.
I see how this is handled now. I need to add the same here, #72 (comment)

I think the error message is a bit misleading thought. May be we could update to tell the user that we could not sync their permission instead of saying that they don't have permission.
Thank you.

Charl1996 · 2024-11-05T07:43:51Z

If we merge this PR whilst keeping the resource on v1 I'd need to just add the update here. I'll wait till it's merged before I do it.

mkangia

Hey @Charl1996

Seems good to go ahead with this now.

Suggested some improvements, that are fine to taken as a follow up even as a separate ticket, if any of them seem reasonable. Considering the approach got simpler, which is great, code/implementation can be simplified further.

mkangia · 2024-11-06T08:51:41Z

hq_superset/const.py

+CAN_WRITE_PERMISSION = "can_write"
+CAN_EDIT_PERMISSION = "can_edit"
+CAN_ADD_PERMISSION = "can_add"
+CAN_DELETE_PERMISSIONS = "can_delete"


it seems some of these are now redundant and can be removed.

mkangia · 2024-11-06T08:51:59Z

hq_superset/hq_url.py

+
+
+def user_domain_roles(domain):
+    return f"a/{domain}/api/analytics-roles/v1"


nit: can we add the trailing slash for consistency?

mkangia · 2024-11-06T08:56:24Z

hq_superset/utils.py

+        The user gets assigned at least 3 roles in order to function on any domain:
+        1. hq_user_role: gives access to superset platform
+        2. domain_schema_role: restricts user access to specific domain schema
+        3. domain_user_role: restricts access for particular user on domain in accordance with how the permissions


I believe this needs to be updated now? we don't have a domain_user_role anymore right?

Can we add to the documentation what return value from this function means? i.e when does it return False/True?

Ah, good catch!

mkangia · 2024-11-06T09:00:59Z

hq_superset/utils.py

+        This role is the bare minimum required for a user to be able to have an account on
+        superset
+        """
+        hq_user_role = self.sm.add_role(HQ_USER_ROLE_NAME)


nit: I wonder if this is causing unnecessary saves on this role in the db since it would exist once it has been created.
Non blocking but good to check and do a follow up if needed.

I was thinking if this should instead be added by AUTH_USER_ADDITIONAL_ROLES instead though I am not sure how that would work since its a custom role and not predefined in superset.

Mmm...I'll check it out

mkangia · 2024-11-06T09:01:48Z

hq_superset/utils.py

+        ]
+        self.sm.set_role_permissions(hq_user_role, hq_user_base_permissions)
+
+        if hq_user_role not in current_user.roles:


nit: should this check be on the top so the role isn't set if its already present?

mkangia · 2024-11-06T09:06:34Z

hq_superset/utils.py

+        # Map between HQ and CCA
+        permissions = {
+            CAN_WRITE_PERMISSION: hq_permissions["can_edit"],
+            CAN_READ_PERMISSION: hq_permissions["can_view"],


nit: do we really need these mapping? we can just return permissions and that should do right?

I guess you're right. Will update (in upcoming PR)

mkangia · 2024-11-06T09:08:32Z

hq_superset/views.py

@@ -40,6 +40,8 @@


 class HQDatasourceView(BaseSupersetView):
+    class_permission_name = "ExploreFormDataRestApi"


just checking on why was this added?

This was to ensure that only users with edit permissions can import data sources.

mkangia · 2024-11-06T09:08:48Z

hq_superset/views.py

@@ -63,6 +65,8 @@ def _ucr_id_from_pk(self, datasource_pk):
            return None

    @expose("/update/<datasource_id>", methods=["GET"])
+    @has_access
+    @permission_name("write")


these checks were missing earlier?

Yes, but earlier we didn't need them. Now we need to set them to explicitly disable a read-only user from performing this action.

Charl1996 · 2024-11-06T11:48:29Z

@mkangia I'm going to merge this just to move it forward, but will be addressing your (very valid) comments in a followup PR.

Charl1996 · 2024-11-15T11:29:09Z

@mkangia

@mkangia I'm going to merge this just to move it forward, but will be addressing your (very valid) comments in a followup PR.

As promised.

Charl1996 added 17 commits August 29, 2024 14:10

Add HQ endpoint

62d687c

Reorder class methods

04a76fd

Add HQ role name mapper

db99e5c

Fetch HQ roles and domain permissions

907919a

Add ability to set role permission and add test

1e4adea

Add additional roles for domain user

c67ebfc

Only assign explicit roles to user

acbdf3b

Make distriction between domain user role and platform roles

759d312

Create default hq user role to be assigned to any hq user

e2c63e2

Add menu permissions and do small refactor

a2dad52

Add menu permissions and do small refactor

Show message to user in case no permissions found

6d5fa58

Add more permissions for edit

b244166

Split write and edit permissions

ebb1346

Restrict create/update datasource access

1c90bba

Use dict mapping for view menu permissions

49338a7

Update tests

f6e5996

Fix tests

Add comment to method

816f1f1

Charl1996 requested review from kaapstorm, mkangia, zandre-eng and ajeety4 August 29, 2024 12:58

mkangia reviewed Aug 30, 2024

View reviewed changes

Charl1996 added 2 commits September 3, 2024 13:18

Update method in accordance with HQ changes

2670e6b

Update tests

a6800b2

mkangia approved these changes Sep 3, 2024

View reviewed changes

Remove username query param

49a96a6

Charl1996 commented Sep 10, 2024

View reviewed changes

Update user write permissions

cb57fa6

1. Users with 'edit' permissions gets the superset Gamma role 2. If HQ user don't have view permissions, don't allow access

Charl1996 requested review from kaapstorm and mkangia October 28, 2024 13:27

mkangia reviewed Oct 28, 2024

View reviewed changes

hq_superset/tests/test_utils.py Show resolved Hide resolved

mkangia reviewed Oct 28, 2024

View reviewed changes

Charl1996 added 2 commits October 29, 2024 10:55

Indicate which domain access is denied in flash message

f5384d1

Update domain user role name to specify read-only

4d52e10

mkangia reviewed Oct 30, 2024

View reviewed changes

Extract role to constant

116c060

Remove redundant mapping

6cae9cb

This was referenced Nov 1, 2024

Assign gamma permissions to user domain role #71

Closed

Handle resource url according to new standard dimagi/commcare-hq#35312

Merged

Merge branch 'master' into cs/SC-3473-user-roles-from-hq

75e294d

Charl1996 added 2 commits November 4, 2024 20:56

Add read-only role const

4f3d65f

Assign universal read only role or gamma role

0c27235

Charl1996 added 2 commits November 5, 2024 17:24

Use v1 of hq resource

6928d00

Update tests

4196ab5

mkangia approved these changes Nov 6, 2024

View reviewed changes

Charl1996 merged commit fad5991 into master Nov 6, 2024
3 checks passed

Charl1996 deleted the cs/SC-3473-user-roles-from-hq branch November 6, 2024 11:58

Charl1996 mentioned this pull request Nov 15, 2024

Post PR updates #79

Merged

		@@ -32,5 +32,5 @@ def datasource_unsubscribe(domain, datasource_id):
		)


		def user_domain_roles(username, domain):



		def user_domain_roles(domain):
		return f"a/{domain}/api/analytics-roles/v1"

		@@ -40,6 +40,8 @@


		class HQDatasourceView(BaseSupersetView):
		class_permission_name = "ExploreFormDataRestApi"

Assign HQ-defined roles to CCA user #59

Assign HQ-defined roles to CCA user #59

Conversation

Charl1996 commented Aug 29, 2024 • edited Loading

Approach

Side note

mkangia commented Aug 30, 2024

Charl1996 commented Aug 30, 2024

mkangia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 commented Aug 30, 2024

mkangia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 commented Oct 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkangia commented Oct 31, 2024

mkangia commented Nov 3, 2024

Charl1996 commented Nov 4, 2024

mkangia commented Nov 4, 2024

Charl1996 commented Nov 5, 2024

mkangia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Charl1996 commented Nov 6, 2024

Charl1996 commented Nov 15, 2024 • edited Loading

Charl1996 commented Aug 29, 2024 •

edited

Loading

Charl1996 Aug 30, 2024 •

edited

Loading

Charl1996 Aug 30, 2024 •

edited

Loading

Charl1996 commented Nov 15, 2024 •

edited

Loading