Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assign HQ-defined roles to CCA user #59

Merged
merged 33 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
62d687c
Add HQ endpoint
Charl1996 Aug 19, 2024
04a76fd
Reorder class methods
Charl1996 Aug 20, 2024
db99e5c
Add HQ role name mapper
Charl1996 Aug 20, 2024
907919a
Fetch HQ roles and domain permissions
Charl1996 Aug 20, 2024
1e4adea
Add ability to set role permission and add test
Charl1996 Aug 21, 2024
c67ebfc
Add additional roles for domain user
Charl1996 Aug 22, 2024
acbdf3b
Only assign explicit roles to user
Charl1996 Aug 26, 2024
759d312
Make distriction between domain user role and platform roles
Charl1996 Aug 26, 2024
e2c63e2
Create default hq user role to be assigned to any hq user
Charl1996 Aug 27, 2024
a2dad52
Add menu permissions and do small refactor
Charl1996 Aug 26, 2024
6d5fa58
Show message to user in case no permissions found
Charl1996 Aug 27, 2024
b244166
Add more permissions for edit
Charl1996 Aug 27, 2024
ebb1346
Split write and edit permissions
Charl1996 Aug 28, 2024
1c90bba
Restrict create/update datasource access
Charl1996 Aug 28, 2024
49338a7
Use dict mapping for view menu permissions
Charl1996 Aug 28, 2024
f6e5996
Update tests
Charl1996 Aug 28, 2024
816f1f1
Add comment to method
Charl1996 Aug 29, 2024
2670e6b
Update method in accordance with HQ changes
Charl1996 Sep 3, 2024
a6800b2
Update tests
Charl1996 Sep 3, 2024
49a96a6
Remove username query param
Charl1996 Sep 10, 2024
d42e09b
Fix tests
Charl1996 Sep 10, 2024
caec3aa
Update: parse detail response
Charl1996 Oct 21, 2024
5efceb6
Fix invalid test response
Charl1996 Oct 21, 2024
cb57fa6
Update user write permissions
Charl1996 Oct 28, 2024
f5384d1
Indicate which domain access is denied in flash message
Charl1996 Oct 29, 2024
4d52e10
Update domain user role name to specify read-only
Charl1996 Oct 29, 2024
116c060
Extract role to constant
Charl1996 Oct 30, 2024
6cae9cb
Remove redundant mapping
Charl1996 Nov 1, 2024
75e294d
Merge branch 'master' into cs/SC-3473-user-roles-from-hq
mkangia Nov 3, 2024
4f3d65f
Add read-only role const
Charl1996 Nov 4, 2024
0c27235
Assign universal read only role or gamma role
Charl1996 Nov 4, 2024
6928d00
Use v1 of hq resource
Charl1996 Nov 5, 2024
4196ab5
Update tests
Charl1996 Nov 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions hq_superset/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,53 @@
HQ_DATABASE_NAME = "HQ Data"

OAUTH2_DATABASE_NAME = "oauth2-server-data"

HQ_USER_ROLE_NAME = "hq_user"

HQ_ROLE_NAME_MAPPING = {
"gamma": "Gamma",
"dataset_editor": "dataset_editor",
"sql_lab": "sql_lab",
}

# Permissions
SCHEMA_ACCESS_PERMISSION = "schema_access"
MENU_ACCESS_PERMISSION = "menu_access"

CAN_SHOW_PERMISSION = "can_show"
CAN_LIST_PERMISSION = "can_list"
CAN_GET_PERMISSION = "can_get"
CAN_EXTERNAL_METADATA_PERMISSION = "can_external_metadata"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this permission allow? Reading external metadata, or setting external metadata, or something else?

CAN_EXTERNAL_METADATA_BY_NAME_PERMISSION = "can_external_metadata_by_name"
CAN_READ_PERMISSION = "can_read"

READ_ONLY_PERMISSIONS = [
CAN_SHOW_PERMISSION,
CAN_LIST_PERMISSION,
CAN_GET_PERMISSION,
CAN_EXTERNAL_METADATA_PERMISSION,
CAN_EXTERNAL_METADATA_BY_NAME_PERMISSION,
CAN_READ_PERMISSION,
]

CAN_WRITE_PERMISSION = "can_write"
CAN_EDIT_PERMISSION = "can_edit"
CAN_ADD_PERMISSION = "can_add"
CAN_DELETE_PERMISSIONS = "can_delete"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



READ_ONLY_MENU_PERMISSIONS = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@Charl1996 Charl1996 Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"Chart": READ_ONLY_PERMISSIONS,
"Dataset": READ_ONLY_PERMISSIONS,
"Dashboard": READ_ONLY_PERMISSIONS,
"Datasource": READ_ONLY_PERMISSIONS,
"OpenApi": READ_ONLY_PERMISSIONS,
"Explore": READ_ONLY_PERMISSIONS,
"Select a Domain": [MENU_ACCESS_PERMISSION],
"Home": [MENU_ACCESS_PERMISSION],
"Data": [MENU_ACCESS_PERMISSION],
"Dashboards": [MENU_ACCESS_PERMISSION],
"Charts": [MENU_ACCESS_PERMISSION],
"Datasets": [MENU_ACCESS_PERMISSION],
"ExploreFormDataRestApi": [CAN_READ_PERMISSION]
}
4 changes: 4 additions & 0 deletions hq_superset/hq_url.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,7 @@ def datasource_unsubscribe(domain, datasource_id):
f"a/{domain}/configurable_reports/data_sources/unsubscribe/"
f"{datasource_id}/"
)


def user_domain_roles(domain):
return f"a/{domain}/api/v0.5/analytics-roles/"
9 changes: 9 additions & 0 deletions hq_superset/oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@Charl1996 Charl1996 Aug 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

"""
This method sets the permissions on a role by overwriting the existing
permissions
"""
role.permissions = []
for permission in permissions:
self.add_permission_role(role, permission)


def get_valid_cchq_oauth_token():
# Returns a valid working oauth access_token and also saves it on session
Expand Down
5 changes: 0 additions & 5 deletions hq_superset/tests/config_for_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@
'0fXurIGyQM4HQYoe7feuwV8c1Kz_88BdmCNutLKiO38=', # Don't reuse this!
]

# 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"]

HQ_DATABASE_URI = "postgresql://commcarehq:commcarehq@localhost:5432/test_superset_hq"

AUTH_TYPE = AUTH_OAUTH
Expand Down
25 changes: 25 additions & 0 deletions hq_superset/tests/test_oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ def test_oauth_user_info(self):
oauth_mock.domain_json["objects"]
)


class TestGetOAuthTokenGetter(SupersetTestCase):

def tearDown(self):
Expand Down Expand Up @@ -110,3 +111,27 @@ def test_tries_token_refresh_if_expired(self):
set_mock.assert_called_once_with(
"commcare", {"access_token": "new key"}
)


class TestSetRolePermissions(SupersetTestCase):

def tearDown(self):
session.clear()

def test_set_role_permissions(self):
appbuilder = self.app.appbuilder
role_name = "test_role"

role = appbuilder.sm.add_role(role_name)
permissions = [
appbuilder.sm.add_permission_view_menu("can_edit", "Chart"),
appbuilder.sm.add_permission_view_menu("can_edit", "Dashboard"),
]

appbuilder.sm.set_role_permissions(role, permissions)
role = appbuilder.sm.find_role(role_name)
assert role.permissions == permissions

appbuilder.sm.set_role_permissions(role, [])
role = appbuilder.sm.find_role(role_name)
assert role.permissions == []
92 changes: 90 additions & 2 deletions hq_superset/tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import doctest
from unittest.mock import patch

from hq_superset.utils import get_column_dtypes

from hq_superset.utils import get_column_dtypes, DomainSyncUtil
from .base_test import SupersetTestCase
from .const import TEST_DATASOURCE


Expand All @@ -24,3 +25,90 @@ def test_doctests():
import hq_superset.utils
results = doctest.testmod(hq_superset.utils)
assert results.failed == 0


class TestDomainSyncUtil(SupersetTestCase):
PLATFORM_ROLE_NAMES = ["Gamma", "sql_lab", "dataset_editor"]

@patch.object(DomainSyncUtil, "_get_domain_access")
def test_gamma_role_assigned_for_edit_permissions(self, get_domain_access_mock):
security_manager = self.app.appbuilder.sm
self._ensure_platform_roles_exist(security_manager)

get_domain_access_mock.return_value = self._to_permissions_response(
can_write=True,
can_read=True,
roles=[],
)
additional_roles = DomainSyncUtil(security_manager)._get_additional_user_roles("test-domain")
assert len(additional_roles) == 1
assert additional_roles[0].name == "Gamma"
mkangia marked this conversation as resolved.
Show resolved Hide resolved

@patch.object(DomainSyncUtil, "_get_domain_access")
def test_no_roles_assigned_without_at_least_read_permission(self, get_domain_access_mock):
security_manager = self.app.appbuilder.sm
self._ensure_platform_roles_exist(security_manager)

get_domain_access_mock.return_value = self._to_permissions_response(
can_write=False,
can_read=False,
roles=["sql_lab", "dataset_editor"],
)
additional_roles = DomainSyncUtil(security_manager)._get_additional_user_roles("test-domain")
assert not additional_roles

@patch.object(DomainSyncUtil, "_domain_user_role_name")
@patch.object(DomainSyncUtil, "_get_domain_access")
def test_read_permission_gives_custom_domain_role(self, get_domain_access_mock, domain_user_role_name_mock):
domain_user_role_name = "test-domain_user_1_read_only"
domain_user_role_name_mock.return_value = domain_user_role_name

security_manager = self.app.appbuilder.sm
self._ensure_platform_roles_exist(security_manager)

get_domain_access_mock.return_value = self._to_permissions_response(
can_write=False,
can_read=True,
roles=[],
)
additional_roles = DomainSyncUtil(security_manager)._get_additional_user_roles("test-domain")
assert len(additional_roles) == 1
assert additional_roles[0].name == domain_user_role_name

@patch.object(DomainSyncUtil, "_domain_user_role_name")
@patch.object(DomainSyncUtil, "_get_domain_access")
def test_permissions_change_updates_user_role(self, get_domain_access_mock, domain_user_role_name_mock):
domain_user_role_name = "test-domain_user_1_read_only"
domain_user_role_name_mock.return_value = domain_user_role_name

security_manager = self.app.appbuilder.sm
self._ensure_platform_roles_exist(security_manager)

# user has maximum access
get_domain_access_mock.return_value = self._to_permissions_response(
can_write=False,
can_read=True,
roles=[],
)
additional_roles = DomainSyncUtil(security_manager)._get_additional_user_roles("test-domain")
assert additional_roles[0].name == domain_user_role_name

# user has no access
get_domain_access_mock.return_value = self._to_permissions_response(
can_write=False,
can_read=False,
roles=[],
)
additional_roles = DomainSyncUtil(security_manager)._get_additional_user_roles("test-domain")
assert not additional_roles

def _ensure_platform_roles_exist(self, sm):
for role_name in self.PLATFORM_ROLE_NAMES:
sm.add_role(role_name)

@staticmethod
def _to_permissions_response(can_write, can_read, roles):
return {
"can_write": can_write,
"can_read": can_read,
}, roles
15 changes: 13 additions & 2 deletions hq_superset/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
from hq_superset.utils import (
SESSION_USER_DOMAINS_KEY,
get_schema_name_for_domain,
DomainSyncUtil,
)

from .base_test import HQDBTestCase
Expand Down Expand Up @@ -80,6 +81,10 @@ def __init__(self):
]
}
self.api_base_url = "https://cchq.org/"
self.user_domain_roles = {
"permissions": {"can_view": True, "can_edit": True},
"roles": ["Gamma", "sql_lab"],
}

def authorize_access_token(self):
return {"access_token": "some-key"}
Expand All @@ -92,6 +97,8 @@ def get(self, url, token):
'a/test2/api/v0.5/ucr_data_source/': MockResponse(self.test2_datasources, 200),
'a/test1/api/v0.5/ucr_data_source/test1_ucr1/': MockResponse(TEST_DATASOURCE, 200),
'a/test1/configurable_reports/data_sources/export/test1_ucr1/?format=csv': MockResponse(TEST_UCR_CSV_V1, 200),
'a/test1/api/v0.5/analytics-roles/': MockResponse(self.user_domain_roles, 200),
'a/test2/api/v0.5/analytics-roles/': MockResponse(self.user_domain_roles, 200),
}[url]


Expand Down Expand Up @@ -171,7 +178,8 @@ def test_redirects_to_domain_select_after_login(self):
)
self.logout(client)

def test_domain_select_works(self):
@patch.object(DomainSyncUtil, "_get_domain_access", return_value=({"can_write": True, "can_read": True}, []))
def test_domain_select_works(self, *args):
client = self.app.test_client()
self.login(client)

Expand Down Expand Up @@ -201,6 +209,7 @@ def test_non_user_domain_cant_be_selected(self):
self.logout(client)

@patch('hq_superset.hq_requests.get_valid_cchq_oauth_token', return_value={})
@patch.object(DomainSyncUtil, "sync_domain_role", return_value=True)
def test_datasource_list(self, *args):
def _do_assert(datasources):
self.assert_template_used("hq_datasource_list.html")
Expand All @@ -218,6 +227,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)
def test_datasource_upload(self, *args):
client = self.app.test_client()
self.login(client)
Expand All @@ -232,7 +242,8 @@ def test_datasource_upload(self, *args):
'ds1'
)

def test_trigger_datasource_refresh_with_api_exception(self):
@patch.object(DomainSyncUtil, "sync_domain_role", return_value=True)
def test_trigger_datasource_refresh_with_api_exception(self, *args):
with patch("hq_superset.views.download_and_subscribe_to_datasource", side_effect=HQAPIException('mocked error')):
client = self.app.test_client()
self.login(client)
Expand Down
Loading
Loading