Expire user permissions #72

mkangia · 2024-11-01T17:06:38Z

https://dimagi.atlassian.net/browse/SC-3475

Building up on work done in cs/SC-3473-user-roles-from-hq to expire user permissions in an hour to keep permissions up to date with HQ.

Review by commit 🐟
There is quite some refactor, done in separate commits for clarity

All tests ran locally successfully. They wont run here since base branch isn't master.

https://dimagi.atlassian.net/browse/QA-7205

- /api/v1/me is called frequently leading to before_request_hook getting fired unnecessarily, specifically for sync_user_domain_role leading to more than one syncs happening at once leading to db conflicts/errors

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

hq_superset/tests/base_test.py

@@ -48,3 +49,33 @@ def tearDown(self):
                    sql = "; ".join(domain_schemas) + ";"
                    connection.execute(text(sql))
        super(HQDBTestCase, self).tearDown()
+
+
+class LoginUserTestMixin(object):


Nice. Seems useful.

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

hq_superset/utils.py

@@ -415,3 +418,7 @@ def cast_data_for_table(
 def generate_secret():
    alphabet = string.ascii_letters + string.digits
    return ''.join(secrets.choice(alphabet) for __ in range(64))
+
+
+def datetime_utcnow_naive():


@mkangia
I think this function name is a misnomer. You're returning an aware datetime, are you not?.

You are right, I got that backwards, will rename this datetime_utcnow.

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

hq_superset/hq_domain.py

+    if not session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT):
+        return True
+
+    time_since_last_sync = datetime_utcnow_naive() - session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT]


@mkangia
Nit: for more robust implementation we could check if session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT] is a datetime-like object that could be operated on.

How about:

last_sync_datetime = session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT] if not isinstance(last_sync_datetime, datetime): return True time_since_last_sync = datetime_utcnow_naive() - last_sync_datetime

Cool. I can add that. This value is only always set by CCA and can't edited by the user so it would certainly be a datetime but no harm in adding safety.

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

hq_superset/hq_domain.py

+def _sync_domain_role():
+    DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(g.hq_domain)


@mkangia
What happens if the domain role cannot sync during this routine? Is it fine to keep the user logged in in the current state?

Ya, Great question.

I guess I will follow the same pattern you have followed otherwise in case sync domain role returns false and redirect user to the select domain page, though possibly with a different error message like "Your permissions have expired and the sync failed due to an unexpected error. Please select project space again or login to continue"

This turned out to be a little challenging, but I got it working like this
a195929

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

hq_superset/tests/test_views.py

@@ -134,7 +134,7 @@ def _do_assert(datasources):
        client.get('/hq_datasource/list/', follow_redirects=True)
        _do_assert(self.oauth_mock.test2_datasources)

-    @patch.object(DomainSyncUtil, "sync_domain_role", return_value=True)
+    @patch.object(DomainSyncUtil, "_get_domain_access", return_value=({"can_write": True, "can_read": True}, []))


Curious what the issue was here?

It was a flaky test that succeeded when run separately or if only this test file was run but failed when the full test suite was run.
I believe the permissions were getting set unexpectedly or getting revoked somehow, so I set them specifically instead of skipping it via the patch for sync_domain_role. I did try to debug it for a couple of hours before letting it be.

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

hq_superset/hq_domain.py

@@ -44,6 +44,7 @@ def after_request_hook(response):
    'AuthOAuthView.login',
    'AuthOAuthView.logout',
    'AuthOAuthView.oauth_authorized',
+    'CurrentUserRestApi.get_me',


Curious what this is for?

From the UI, this a call made to constantly sync the user record, I assume its getting used in javascript/UI somewhere.
It is a domain independent API call.
I added it to this list so we don't fire the before request hooks on such requests since they are not needed.

Found one more, b4eb1b3

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

@mkangia Nice! Thanks for the refactors and good tests!

…-permissions

ajeety4 · 2024-11-05T13:44:32Z

hq_superset/api.py

-from .models import DataSetChange
-from .oauth2_server import authorization, require_oauth
-from .tasks import process_dataset_change
+from hq_superset.models import DataSetChange


ajeety4

Great Refactors. The changes looks good.

Just a query related to the fix made in the commit

/api/v1/me is called frequently leading to before_request_hook getting fired unnecessarily, specifically for sync_user_domain_role leading to more than one syncs happening at once leading to db conflicts/errors

Does the db conflicts/errors happened because of API calls that are simultaneous? Wondering if the same thing can occur if a user is working with multiple tabs at once ?

…-permissions

Charl1996

Thanks for the changes! Looks good to me!

mkangia · 2024-11-06T10:54:42Z

Hey @ajeety4

Does the db conflicts/errors happened because of API calls that are simultaneous? Wondering if the same thing can occur if a user is working with multiple tabs at once ?

Spot on. That fix is where I noticed it first.
I made the following change in order to limit to only one sync at once.
It does appear to have solved the issue in most cases.

However there can be some weird edge cases where two requests come exactly at the same moment which I am letting go for now since sync will get reattempted at the next request if needed for such edge cases.
I noticed that locally but not on staging during some minor testing.

mkangia · 2024-11-06T10:55:18Z

Thanks for the reviews @Charl1996

mkangia requested a review from Charl1996 November 1, 2024 17:08

mkangia force-pushed the mk/3475-expire-user-permissions branch from dc39da7 to 12a7dc0 Compare November 3, 2024 09:54

mkangia added 11 commits November 3, 2024 20:02

refactor: setup for multiple hooks

837b5f4

refactor: extract test utils & login mixin; isort

28815f5

track role syncing timestamp

011ebda

isort

df017ea

resync domain role after expiry

3622bbf

refactor: move constants to const;isort

b437aba

refactor: expand imports;isort

764d677

fix flaky test

2d002ca

ensure logout after each test

e678353

add domain independent call

716651c

- /api/v1/me is called frequently leading to before_request_hook getting fired unnecessarily, specifically for sync_user_domain_role leading to more than one syncs happening at once leading to db conflicts/errors

refactor; remove duplicate import

c8111d4

mkangia force-pushed the mk/3475-expire-user-permissions branch from 12a7dc0 to c8111d4 Compare November 3, 2024 14:36

Charl1996 reviewed Nov 4, 2024

View reviewed changes

mkangia mentioned this pull request Nov 4, 2024

Assign HQ-defined roles to CCA user #59

Merged

mkangia added 4 commits November 4, 2024 22:48

correct function name

7ef5429

return error message if syncing fails

a195929

add log requests to domain excluded views to ignore in hooks

b4eb1b3

avoid sync conflicts

a5f2c58

mkangia force-pushed the mk/3475-expire-user-permissions branch from 59370e1 to a5f2c58 Compare November 4, 2024 22:26

Merge branch 'cs/SC-3473-user-roles-from-hq' into mk/3475-expire-user…

e92b419

…-permissions

ajeety4 reviewed Nov 5, 2024

View reviewed changes

Merge branch 'cs/SC-3473-user-roles-from-hq' into mk/3475-expire-user…

e98e66b

…-permissions

Charl1996 approved these changes Nov 6, 2024

View reviewed changes

ajeety4 approved these changes Nov 6, 2024

View reviewed changes

Base automatically changed from cs/SC-3473-user-roles-from-hq to master November 6, 2024 11:58

update error message to highlight possible access revoke

da46257

ajeety4 approved these changes Nov 8, 2024

View reviewed changes

mkangia added the QA Passed label Nov 10, 2024

mkangia merged commit b2c64d3 into master Nov 10, 2024
3 checks passed

mkangia deleted the mk/3475-expire-user-permissions branch November 10, 2024 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expire user permissions #72

Expire user permissions #72

mkangia commented Nov 1, 2024 •

edited

Loading

Charl1996 Nov 4, 2024

Charl1996 Nov 4, 2024

mkangia Nov 4, 2024

mkangia Nov 4, 2024

Charl1996 Nov 4, 2024 •

edited

Loading

mkangia Nov 4, 2024

Charl1996 Nov 4, 2024

mkangia Nov 4, 2024

mkangia Nov 4, 2024

Charl1996 Nov 4, 2024

mkangia Nov 4, 2024

Charl1996 Nov 4, 2024

mkangia Nov 4, 2024

mkangia Nov 4, 2024

Charl1996 commented Nov 4, 2024

ajeety4 Nov 5, 2024

ajeety4 left a comment

Charl1996 left a comment

mkangia commented Nov 6, 2024

mkangia commented Nov 6, 2024

		def _sync_domain_role():
		DomainSyncUtil(superset.appbuilder.sm).sync_domain_role(g.hq_domain)

Expire user permissions #72

Expire user permissions #72

Conversation

mkangia commented Nov 1, 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

Charl1996 Nov 4, 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 Nov 4, 2024

Choose a reason for hiding this comment

ajeety4 left a comment

Choose a reason for hiding this comment

Charl1996 left a comment

Choose a reason for hiding this comment

mkangia commented Nov 6, 2024

mkangia commented Nov 6, 2024

mkangia commented Nov 1, 2024 •

edited

Loading

Charl1996 Nov 4, 2024 •

edited

Loading