From 7648ed0f00a51fb53245ba905a80a385880d5d64 Mon Sep 17 00:00:00 2001 From: Nnamdi Kenneth Ojibe Date: Fri, 18 Oct 2024 14:51:34 -0400 Subject: [PATCH] [Insights][Auth] Have (`workflows` | `outliers`)_authorization.py accept feature variants set as `True` or `{}` (Recidiviz/recidiviz-data#34287) GitOrigin-RevId: f1062bf99ae757be6a9e97fa33e794ab37c0cae8 --- recidiviz/case_triage/authorization_utils.py | 44 +++++++++ .../outliers/outliers_authorization.py | 17 +--- .../workflows/workflows_authorization.py | 27 +----- .../case_triage/authorization_utils_test.py | 91 +++++++++++++++++++ .../workflows/workflows_authorization_test.py | 59 +----------- 5 files changed, 146 insertions(+), 92 deletions(-) create mode 100644 recidiviz/tests/case_triage/authorization_utils_test.py diff --git a/recidiviz/case_triage/authorization_utils.py b/recidiviz/case_triage/authorization_utils.py index ebe2853e9a..ad1dabcec3 100644 --- a/recidiviz/case_triage/authorization_utils.py +++ b/recidiviz/case_triage/authorization_utils.py @@ -18,7 +18,9 @@ This module contains a helper for authenticating users accessing product APIs hosted on the Case Triage backend. """ +import datetime import json +import logging import os from http import HTTPStatus from typing import Any, Callable, Dict, List, Optional @@ -139,3 +141,45 @@ def on_successful_authorization_requested_state( return raise AuthorizationError(code="not_authorized", description="Access denied") + + +def get_active_feature_variants( + feature_variants: dict, pseudonymized_id: Optional[str] +) -> dict: + """Get active feature variants for a user, logging an error if any + feature variant is invalid (i.e., not a dict or bool). + + Args: + feature_variants (Dict[str, Any]): Raw dictionary of possible feature variants. + pseudonymized_id (Optional[str]): ID of the user for logging purposes. + + Returns: + Dict[str, Any]: Active feature variants. + """ + + active_feature_variants, failure_feature_variants = {}, {} + + for fv, params in feature_variants.items(): + if isinstance(params, dict): + active_date: datetime.datetime | None = ( + datetime.datetime.fromisoformat(params["activeDate"]) + if "activeDate" in params + else None + ) + if active_date is None or active_date < datetime.datetime.now( + tz=active_date.tzinfo + ): + active_feature_variants[fv] = params + elif params is True: + active_feature_variants[fv] = {} + elif params not in {False, None}: + failure_feature_variants[fv] = params + + if failure_feature_variants: + logging.error( + "User with id %s has invalid feature variants: %s", + pseudonymized_id if pseudonymized_id else "unknown", + failure_feature_variants, + ) + + return active_feature_variants diff --git a/recidiviz/case_triage/outliers/outliers_authorization.py b/recidiviz/case_triage/outliers/outliers_authorization.py index edab6c267b..c4c0eab191 100644 --- a/recidiviz/case_triage/outliers/outliers_authorization.py +++ b/recidiviz/case_triage/outliers/outliers_authorization.py @@ -15,7 +15,6 @@ # along with this program. If not, see . # ============================================================================= """Implements authorization for Outliers routes""" -import datetime import os from http import HTTPStatus from typing import Any, Dict, Optional @@ -26,6 +25,7 @@ get_outliers_enabled_states, ) from recidiviz.case_triage.authorization_utils import ( + get_active_feature_variants, on_successful_authorization_requested_state, ) from recidiviz.case_triage.outliers.user_context import UserContext @@ -63,16 +63,7 @@ def on_successful_authorization( ) user_pseudonymized_id = app_metadata.get("pseudonymizedId", None) routes = app_metadata.get("routes", {}) - feature_variants = { - fv: params - for fv, params in app_metadata.get("featureVariants", {}).items() - if "activeDate" not in params - or datetime.datetime.fromisoformat(params["activeDate"]) - # Handle both naive and UTC activeDates - < datetime.datetime.now( - tz=datetime.datetime.fromisoformat(params["activeDate"]).tzinfo - ) - } + feature_variants = app_metadata.get("featureVariants", {}) if user_state_code == "RECIDIVIZ": feature_variants["supervisorHomepageWorkflows"] = {} @@ -83,7 +74,9 @@ def on_successful_authorization( can_access_all_supervisors=is_recidiviz_or_csg # TODO(Recidiviz/recidiviz-dashboards#4520): don't hard-code this string or routes.get("insights_supervision_supervisors-list", False), - feature_variants=feature_variants, + feature_variants=get_active_feature_variants( + feature_variants, user_pseudonymized_id + ), ) # If the user is a recidiviz user, skip endpoint checks diff --git a/recidiviz/case_triage/workflows/workflows_authorization.py b/recidiviz/case_triage/workflows/workflows_authorization.py index 8ac621e044..b7c93712ff 100644 --- a/recidiviz/case_triage/workflows/workflows_authorization.py +++ b/recidiviz/case_triage/workflows/workflows_authorization.py @@ -15,8 +15,6 @@ # along with this program. If not, see . # ============================================================================= """Implements user validations for workflows APIs. """ -import datetime -import logging import os from typing import Any, Dict, List @@ -26,6 +24,7 @@ get_workflows_enabled_states, ) from recidiviz.case_triage.authorization_utils import ( + get_active_feature_variants, on_successful_authorization_requested_state, ) from recidiviz.common.constants.states import StateCode @@ -66,26 +65,10 @@ def on_successful_authorization(claims: Dict[str, Any]) -> None: app_metadata = claims[f"{os.environ['AUTH0_CLAIM_NAMESPACE']}/app_metadata"] g.is_recidiviz_user = app_metadata["stateCode"].upper() == "RECIDIVIZ" - g.feature_variants = {} - for fv, params in app_metadata.get("featureVariants", {}).items(): - if isinstance(params, dict): - # Only include FVs with no date, or with a date that parses correctly & is in the past - if "activeDate" not in params or datetime.datetime.fromisoformat( - params["activeDate"] - ) < datetime.datetime.now( - tz=datetime.datetime.fromisoformat(params["activeDate"]).tzinfo - ): - g.feature_variants[fv] = params - elif params is True: - g.feature_variants[fv] = {} - elif params is not False and params is not None: - id_for_error = app_metadata.get("pseudonymizedId", "unknown") - logging.error( - "User with id %s has feature value %s with non-dict/bool value %s", - id_for_error, - fv, - params, - ) + g.feature_variants = get_active_feature_variants( + app_metadata.get("featureVariants", {}), + app_metadata.get("pseudonymizedId", None), + ) def get_workflows_external_request_enabled_states() -> List[str]: diff --git a/recidiviz/tests/case_triage/authorization_utils_test.py b/recidiviz/tests/case_triage/authorization_utils_test.py new file mode 100644 index 0000000000..d124f5a37b --- /dev/null +++ b/recidiviz/tests/case_triage/authorization_utils_test.py @@ -0,0 +1,91 @@ +# Recidiviz - a data platform for criminal justice reform +# Copyright (C) 2023 Recidiviz, Inc. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +# ============================================================================= +"""Implements tests for authorization utils.""" + +from unittest import TestCase + +import freezegun + +from recidiviz.case_triage.authorization_utils import get_active_feature_variants + +TEST_PSEUDONYMIZED_ID: str = "testestestbsapr749bwrb893b4389test" + +INPUT_FVS = { + "fvOne": {}, + "fvTwo": {"activeDate": "2022-01-01T01:01:01.000Z"}, + "fvThree": {"activeDate": "2022-01-01T01:01:01.000"}, + "fvFour": {"activeDate": "2022-01-01"}, + "fvFive": True, +} +EXPECTED_FVS = { + "fvOne": {}, + "fvTwo": {"activeDate": "2022-01-01T01:01:01.000Z"}, + "fvThree": {"activeDate": "2022-01-01T01:01:01.000"}, + "fvFour": {"activeDate": "2022-01-01"}, + "fvFive": {}, +} + + +class TestAuthorizationUtils(TestCase): + """_summary_ + Test authorization utils for case triage. + """ + + @freezegun.freeze_time("2022-12-30") + def test_feature_variant_parsing_included(self) -> None: + + self.assertDictEqual( + get_active_feature_variants(INPUT_FVS, TEST_PSEUDONYMIZED_ID), EXPECTED_FVS + ) + + @freezegun.freeze_time("2022-12-30") + def test_feature_variant_with_future_active_date(self) -> None: + input_fvs = {**INPUT_FVS, "fvSix": {"activeDate": "2023-01-01T01:01:01.000Z"}} + + self.assertDictEqual( + get_active_feature_variants(input_fvs, TEST_PSEUDONYMIZED_ID), EXPECTED_FVS + ) + + @freezegun.freeze_time("2022-12-30") + def test_feature_variant_with_bad_value(self) -> None: + # Define the additional feature variants as a separate dict + new_fvs = {"fvZero": 3, "fvSix": "dog"} + + # Merge new_fvs into a copy of INPUT_FVS + input_fvs = {**INPUT_FVS, **new_fvs} + + # Assert active feature variants match expected ones + self.assertDictEqual( + get_active_feature_variants(input_fvs, TEST_PSEUDONYMIZED_ID), EXPECTED_FVS + ) + + # Capture logs and verify content + with self.assertLogs(level="ERROR") as log: + get_active_feature_variants(input_fvs, TEST_PSEUDONYMIZED_ID) + + # Loop through new_fvs to check if each one is logged as expected + for key, value in new_fvs.items(): + self.assertTrue( + any( + all( + term in message + for term in [TEST_PSEUDONYMIZED_ID, key, str(value)] + ) + for message in log.output + ), + f"Expected {key} with value {value} in the log message, but it was not found.", + ) diff --git a/recidiviz/tests/case_triage/workflows/workflows_authorization_test.py b/recidiviz/tests/case_triage/workflows/workflows_authorization_test.py index 007daf01ba..9b5c7565d2 100644 --- a/recidiviz/tests/case_triage/workflows/workflows_authorization_test.py +++ b/recidiviz/tests/case_triage/workflows/workflows_authorization_test.py @@ -20,8 +20,7 @@ from unittest import TestCase, mock from unittest.mock import MagicMock -from flask import Flask, Response, make_response, g -import freezegun +from flask import Flask, Response, make_response from recidiviz.case_triage.workflows.workflows_authorization import ( on_successful_authorization, @@ -138,59 +137,3 @@ def test_on_successful_authorization(self, _mock_enabled_states: MagicMock) -> N "external_request/US_WY/enqueue_sms_request", user_state_code="US_WY" ) self.assertEqual(assertion.exception.code, "external_requests_not_enabled") - - @freezegun.freeze_time("2022-12-30") - def test_feature_variant_parsing_included(self) -> None: - input_fvs = { - "fvOne": {}, - "fvTwo": {"activeDate": "2022-01-01T01:01:01.000Z"}, - "fvThree": {"activeDate": "2022-01-01T01:01:01.000"}, - "fvFour": {"activeDate": "2022-01-01"}, - "fvFive": True, - } - - expected_fvs = { - "fvOne": {}, - "fvTwo": {"activeDate": "2022-01-01T01:01:01.000Z"}, - "fvThree": {"activeDate": "2022-01-01T01:01:01.000"}, - "fvFour": {"activeDate": "2022-01-01"}, - "fvFive": {}, - } - - with test_app.app_context(): # to access the Flask g object from within a test - self.assertIsNone( - self.process_claims( - "external_request/US_CA/enqueue_sms_request", - user_state_code="US_CA", - feature_variants=input_fvs, - ) - ) - fvs = g.get("feature_variants") - self.assertDictEqual(fvs, expected_fvs) - - @freezegun.freeze_time("2022-12-30") - def test_feature_variant_parsing_excluded(self) -> None: - with test_app.app_context(): # to access the Flask g object from within a test - self.assertIsNone( - self.process_claims( - "external_request/US_CA/enqueue_sms_request", - user_state_code="US_CA", - feature_variants={ - "fvShouldNotIncludeFalse": False, - "fvShouldNotIncludeNone": None, - "fvShouldNotIncludeFuture": {"activeDate": "2023-01-01"}, - }, - ) - ) - fvs = g.get("feature_variants") - self.assertDictEqual(fvs, {}) - - def test_feature_variant_parsing_incorrect_date(self) -> None: - with self.assertRaises(ValueError): - self.process_claims( - "external_request/US_CA/enqueue_sms_request", - user_state_code="US_CA", - feature_variants={ - "fvOne": {"activeDate": "garbagio, not a real datetime"}, - }, - )