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

Expire user permissions #72

Merged
merged 18 commits into from
Nov 10, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
6 changes: 3 additions & 3 deletions hq_superset/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@
json_success,
)

from .models import DataSetChange
from .oauth2_server import authorization, require_oauth
from .tasks import process_dataset_change
from hq_superset.models import DataSetChange
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

from hq_superset.oauth2_server import authorization, require_oauth
from hq_superset.tasks import process_dataset_change


class OAuth(BaseApi):
Expand Down
5 changes: 5 additions & 0 deletions hq_superset/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,8 @@
"Datasets": [MENU_ACCESS_PERMISSION],
"ExploreFormDataRestApi": [CAN_READ_PERMISSION]
}

DOMAIN_PREFIX = "hqdomain_"
SESSION_USER_DOMAINS_KEY = "user_hq_domains"
SESSION_OAUTH_RESPONSE_KEY = "oauth_response"
SESSION_DOMAIN_ROLE_LAST_SYNCED_AT = "domain_role_last_synced_at"
46 changes: 44 additions & 2 deletions hq_superset/hq_domain.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,30 @@
from datetime import timedelta

import superset
from flask import flash, g, redirect, request, session, url_for
from superset.config import USER_DOMAIN_ROLE_EXPIRY

from .utils import SESSION_USER_DOMAINS_KEY
from hq_superset.const import (
SESSION_DOMAIN_ROLE_LAST_SYNCED_AT,
SESSION_USER_DOMAINS_KEY,
)
from hq_superset.utils import DomainSyncUtil, datetime_utcnow_naive


def before_request_hook():
return ensure_domain_selected()
"""
Call all hooks functions set in sequence and
if any hook returns a response,
break the chain and return that response
"""
hooks = [
ensure_domain_selected,
sync_user_domain_role
]
for _function in hooks:
response = _function()
if response:
return response


def after_request_hook(response):
Expand Down Expand Up @@ -55,6 +75,28 @@ def ensure_domain_selected():
return redirect(url_for('SelectDomainView.list', next=request.url))


def sync_user_domain_role():
if is_user_admin() or (
request.url_rule
and request.url_rule.endpoint in DOMAIN_EXCLUDED_VIEWS
):
return
if _domain_role_expired():
_sync_domain_role()


def _domain_role_expired():
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]
Copy link
Contributor

@Charl1996 Charl1996 Nov 4, 2024

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

return time_since_last_sync >= timedelta(minutes=USER_DOMAIN_ROLE_EXPIRY)


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

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Screenshot from 2024-11-05 00-58-37



def is_valid_user_domain(hq_domain):
# Admins have access to all domains
return is_user_admin() or hq_domain in user_domains()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
"""
from typing import Sequence, Union

from alembic import op
import sqlalchemy as sa

from alembic import op

# revision identifiers, used by Alembic.
revision: str = '56d0467ff6ff'
Expand Down
10 changes: 7 additions & 3 deletions hq_superset/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@
from cryptography.fernet import MultiFernet
from superset import db

from .const import OAUTH2_DATABASE_NAME
from .exceptions import TableMissing
from .utils import cast_data_for_table, get_fernet_keys, get_hq_database
from hq_superset.const import OAUTH2_DATABASE_NAME
from hq_superset.exceptions import TableMissing
from hq_superset.utils import (
cast_data_for_table,
get_fernet_keys,
get_hq_database,
)


@dataclass
Expand Down
4 changes: 2 additions & 2 deletions hq_superset/oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@
from requests.exceptions import HTTPError
from superset.security import SupersetSecurityManager

from .exceptions import OAuthSessionExpired
from .utils import (
from hq_superset.const import (
SESSION_OAUTH_RESPONSE_KEY,
SESSION_USER_DOMAINS_KEY,
)
from hq_superset.exceptions import OAuthSessionExpired

logger = logging.getLogger(__name__)

Expand Down
2 changes: 1 addition & 1 deletion hq_superset/oauth2_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
)
from authlib.oauth2.rfc6749 import grants

from .models import OAuth2Client, OAuth2Token, db
from hq_superset.models import OAuth2Client, OAuth2Token, db


def save_token(token: dict, request: FlaskOAuth2Request) -> None:
Expand Down
10 changes: 5 additions & 5 deletions hq_superset/services.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,16 @@
from superset.extensions import cache_manager
from superset.sql_parse import Table

from .exceptions import HQAPIException
from .hq_requests import HQRequest
from .hq_url import (
from hq_superset.exceptions import HQAPIException
from hq_superset.hq_requests import HQRequest
from hq_superset.hq_url import (
datasource_details,
datasource_export,
datasource_subscribe,
datasource_unsubscribe,
)
from .models import OAuth2Client
from .utils import (
from hq_superset.models import OAuth2Client
from hq_superset.utils import (
convert_to_array,
generate_secret,
get_column_dtypes,
Expand Down
6 changes: 3 additions & 3 deletions hq_superset/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

from superset.extensions import celery_app

from .exceptions import TableMissing
from .models import DataSetChange
from .services import AsyncImportHelper, refresh_hq_datasource
from hq_superset.exceptions import TableMissing
from hq_superset.models import DataSetChange
from hq_superset.services import AsyncImportHelper, refresh_hq_datasource


@celery_app.task(name='refresh_hq_datasource_task')
Expand Down
36 changes: 34 additions & 2 deletions hq_superset/tests/base_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,17 @@
"""
Base TestCase class
"""

import os
import shutil

import jwt
from flask_testing import TestCase
from sqlalchemy.sql import text
from superset.app import create_app

from hq_superset.utils import DOMAIN_PREFIX, get_hq_database
from hq_superset.const import DOMAIN_PREFIX
from hq_superset.tests.utils import OAuthMock
from hq_superset.utils import get_hq_database

superset_test_home = os.path.join(os.path.dirname(__file__), ".test_superset")
shutil.rmtree(superset_test_home, ignore_errors=True)
Expand Down Expand Up @@ -48,3 +50,33 @@ def tearDown(self):
sql = "; ".join(domain_schemas) + ";"
connection.execute(text(sql))
super(HQDBTestCase, self).tearDown()


class LoginUserTestMixin(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Seems useful.

"""
Use this mixin by calling login function with client
& then logout once done for clearing the session
"""
def login(self, client):
self._setup_user()
# bypass oauth-workflow by skipping login and oauth flow
with client.session_transaction() as session_:
session_["oauth_state"] = "mock_state"
state = jwt.encode({}, "mock_state", algorithm="HS256")
return client.get(f"/oauth-authorized/commcare?state={state}", follow_redirects=True)

def _setup_user(self):
self.app.appbuilder.add_permissions(update_perms=True)
self.app.appbuilder.sm.sync_role_definitions()

self.oauth_mock = OAuthMock()
self.app.appbuilder.sm.oauth_remotes = {"commcare": self.oauth_mock}

gamma_role = self.app.appbuilder.sm.find_role('Gamma')
self.user = self.app.appbuilder.sm.find_user(self.oauth_mock.user_json['username'])
if not self.user:
self.user = self.app.appbuilder.sm.add_user(**self.oauth_mock.user_json, role=[gamma_role])

@staticmethod
def logout(client):
return client.get("/logout/")
1 change: 1 addition & 0 deletions hq_superset/tests/config_for_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,4 @@
# CommCare Analytics extensions
FLASK_APP_MUTATOR = flask_app_mutator
CUSTOM_SECURITY_MANAGER = oauth.CommCareSecurityManager
USER_DOMAIN_ROLE_EXPIRY = 60 # minutes
13 changes: 13 additions & 0 deletions hq_superset/tests/const.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,16 @@
"id": "test1_ucr1",
"resource_uri": "/a/demo/api/v0.5/ucr_data_source/52a134da12c9b801bd85d2122901b30c/"
}

TEST_UCR_CSV_V1 = """\
doc_id,inserted_at,data_visit_date_eaece89e,data_visit_number_33d63739,data_lmp_date_5e24b993,data_visit_comment_fb984fda
a1, 2021-12-20, 2022-01-19, 100, 2022-02-20, some_text
a2, 2021-12-22, 2022-02-19, 10, 2022-03-20, some_other_text
"""

TEST_UCR_CSV_V2 = """\
doc_id,inserted_at,data_visit_date_eaece89e,data_visit_number_33d63739,data_lmp_date_5e24b993,data_visit_comment_fb984fda
a1, 2021-12-20, 2022-01-19, 100, 2022-02-20, some_text
a2, 2021-12-22, 2022-02-19, 10, 2022-03-20, some_other_text
a3, 2021-11-22, 2022-01-19, 10, 2022-03-20, some_other_text2
"""
5 changes: 2 additions & 3 deletions hq_superset/tests/test_hq_domain.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

from flask import g

from hq_superset.const import SESSION_USER_DOMAINS_KEY
from hq_superset.hq_domain import (
DOMAIN_EXCLUDED_VIEWS,
after_request_hook,
Expand All @@ -10,15 +11,13 @@
is_valid_user_domain,
user_domains,
)
from hq_superset.tests.base_test import HQDBTestCase, SupersetTestCase
from hq_superset.utils import (
SESSION_USER_DOMAINS_KEY,
DomainSyncUtil,
get_hq_database,
get_schema_name_for_domain,
)

from .base_test import HQDBTestCase, SupersetTestCase

MOCK_DOMAIN_SESSION = {
SESSION_USER_DOMAINS_KEY:[
{
Expand Down
9 changes: 4 additions & 5 deletions hq_superset/tests/test_oauth.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,13 @@

from flask import session

from hq_superset.exceptions import OAuthSessionExpired
from hq_superset.oauth import get_valid_cchq_oauth_token
from hq_superset.utils import (
from hq_superset.const import (
SESSION_OAUTH_RESPONSE_KEY,
SESSION_USER_DOMAINS_KEY,
)

from .base_test import SupersetTestCase
from hq_superset.exceptions import OAuthSessionExpired
from hq_superset.oauth import get_valid_cchq_oauth_token
from hq_superset.tests.base_test import SupersetTestCase


class MockResponse:
Expand Down
31 changes: 27 additions & 4 deletions hq_superset/tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,12 @@
import doctest
from unittest.mock import patch

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

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


def test_get_column_dtypes():
Expand All @@ -27,7 +30,7 @@ def test_doctests():
assert results.failed == 0


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

@patch.object(DomainSyncUtil, "_get_domain_access")
Expand Down Expand Up @@ -102,6 +105,26 @@ def test_permissions_change_updates_user_role(self, get_domain_access_mock, doma
additional_roles = DomainSyncUtil(security_manager)._get_additional_user_roles("test-domain")
assert not additional_roles

@patch('hq_superset.utils.datetime_utcnow_naive')
@patch.object(DomainSyncUtil, "_get_domain_access")
def test_sync_domain_role(self, get_domain_access_mock, utcnow_mock):
client = self.app.test_client()
self.login(client)

utcnow_mock_return = "2024-11-01 14:30:04.323000+00:00"
utcnow_mock.return_value = utcnow_mock_return
get_domain_access_mock.return_value = self._to_permissions_response(
can_write=False,
can_read=True,
roles=[],
)
security_manager = self.app.appbuilder.sm

self.assertIsNone(session.get(SESSION_DOMAIN_ROLE_LAST_SYNCED_AT))
DomainSyncUtil(security_manager).sync_domain_role("test-domain")
self.assertEqual(session[SESSION_DOMAIN_ROLE_LAST_SYNCED_AT], utcnow_mock_return)
self.logout(client)

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