Enforcing Organization and Role API Permissions #127

jacob6838 · 2024-12-05T14:48:40Z

PR Details

Description

This PR enforces organization and role permissions in the cvmanager api. This includes the following changes:

Pulling all user properties from the decoded access token (name, username, super_user, and organizations/roles
The user's info and org/role info is stored in the flask environment variable, under the key 'user'. This data is saved in the classes EnvironNoAuth, EnvironWithoutOrg, and EnvironWithOrg (saves Organization header for later use)
Non-200 response codes (400, 403, 404, 500, and 503), are now handled by throwing specific exceptions, stored in common/errors.py. common/errors.py also registers error handlers for these exceptions, and emits the expected response code when each is thrown
Authentication is enforced by decorators. This decorator, @require_permission, takes in 4 params:
- required_role: minimum role required to access method
- resource_type: Optional, 'intersection', 'rsu', 'user', or 'organization'. This enables enforcement of resource-specific permissions. When specified, the first argument in the decorated method must be the resource id (intersection_id, rsu_ip, user_email, or org_name). The decorator will use this resource ID and the user's organizations and role associations to identify whether the user has access to that resource, with at least the required_role level of access.
- checker: Optional, a PermissionChecker class, to completely customize the authentication system
- additional_check: Optional, an AdditionalCheck method, which can apply additional enforcement after the other authentication is evaluated. This can be seen in api/src/admin_intersection.py protecting modify_intersection_authorized from "organizations_to_add" and "organizations_to_remove" fields
- The decorator pulls the user information from the flask environment, so there is no need to pass this into the decorator
- For the most part, authentication and user property access is limited to this decorator. In some cases, queries can be modified to include user info, such as only filtering by organization if the user is not a super_user. In cases like this, a method can receive a PermissionRequest object from the decorator, by specifying the named parameter "permission_result". This includes the qualified organizations (if non-super user), and user info (super_user, all orgs/roles, and name/email). An example of this is in api/src/admin_intersection.py
auth_tools helper methods
- common/auth_tools.py holds many helper methods and types for authentication
Unit tests
- the src/tests/conftest.py method adds a fake user to the flask environment, to enable most tests to pass without modification. This can be easily overridden by patching the common.auth_tools.environ in an individual test

How Has This Been Tested?

This has been extensively tested by unit tests, going through the GUI, and making manual requests through postman, to test responses to malicious requests

Types of changes

Defect fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that cause existing functionality to change)

Checklist:

My changes require new environment variables:
- I have updated the docker-compose, K8s YAML, and all dependent deployment configuration files.
My changes require updates to the documentation:
- I have updated the documentation accordingly.
My changes require updates and/or additions to the unit tests:
- I have modified/added tests to cover my changes.
All existing tests pass.

drewjj

Some questions. Looks nice though.

resources/sql_scripts/CVManager_CreateTables.sql

services/api/src/main.py

services/api/src/middleware.py

services/api/src/admin_new_rsu.py

mcook42

I love the test coverage and a lot of the additions. I have some chores and open questions that I think should be addressed before I'm ready to approve this

services/api/src/admin_email_notification.py

services/common/auth_tools.py

mcook42 · 2024-12-17T21:48:50Z

services/api/src/middleware.py

+            logging.warning(f"User info: {user_info}")
+            if user_info:
+                environ[ENVIRON_USER_KEY] = EnvironWithoutOrg(user_info)
+                # environ["user_info"] = user_info


question: is there value in keeping this commented out code around, or can we remove it?

mcook42 · 2024-12-17T21:53:23Z

services/api/src/rsu_querycounts.py


-        rsu_dict = get_organization_rsus(request.environ["organization"])
-        data, code = query_rsu_counts_mongo(rsu_dict, message, start, end)
+        rsu_dict = get_organization_rsus(permission_result)


Suggested change

rsu_dict = get_organization_rsus(permission_result)

rsu_dict = get_organization_rsus(permission_result, [])

updated to pass in qualified_orgs

mcook42 · 2024-12-17T21:54:19Z

services/api/src/rsu_querymsgfwd.py

@@ -96,12 +112,11 @@ def get(self):
        logging.debug("RsuQueryMsgFwd GET requested")
        # Schema check for arguments
        schema = RsuQueryMsgFwdSchema()
+        user: EnvironWithOrg = request.environ[ENVIRON_USER_KEY]


chore: remove unused variable

Suggested change

user: EnvironWithOrg = request.environ[ENVIRON_USER_KEY]

removed, along with similar unused variables

mcook42 · 2024-12-17T21:55:21Z

services/api/src/rsu_ssm_srm.py

@@ -143,6 +143,8 @@ def options(self):
    def get(self):
        logging.debug("RsuSsmSrmData GET requested")
        data = []
+
+        # TODO: Filter by RSUs within authenticated organizations


question(non-blocking): How/when do we plan to address this TODO? Is there a work item to track it so it doesn't get forgotten?

went ahead and addressed this now!

mcook42 · 2024-12-17T22:03:40Z

services/api/src/rsuinfo.py

        logging.debug("RsuInfo GET requested")
-        return (get_rsu_data(request.environ["organization"]), 200, self.headers)
+        return (get_rsu_data(permission_result), 200, self.headers)


question: I'm seeing calls to get_rsu_data mostly without providing the positional arg of qualified_orgs. Should we be passing an empty list in for qualified_orgs everywhere, remove the qualified_orgs arg, or is there another approach that makes most sense?

get_rsu_info was updated to take in only what it needed, the user object and qualified organizations list

payneBrandon

A few questions so far

payneBrandon · 2024-12-20T22:38:02Z

services/api/src/admin_email_notification.py

@@ -87,15 +109,22 @@ def modify_notification(notification_spec):
        failed_value = failed_value.replace(")", '"')
        failed_value = failed_value.replace("=", " = ")
        logging.error(f"Exception encountered: {failed_value}")
-        return {"message": failed_value}, 500
+        raise InternalServerError(failed_value) from e
+    except InternalServerError:


If we're not doing anything with this particular error, do we need a called out "except" for it?

This is an oddity within the python try/except, while having a very broad Exception catcher at the end. Without the additional except IntersectionServerError: pass, then raise InternalServerError is caught by the last clause of the try catch. The real issue here is that the last except clause is way too broad

I just updated all of the following cases to catch a SQLAlchemyError instead of a generic Exception. This allowed me to remove the except InternalServerError: pass cases

payneBrandon · 2024-12-20T22:40:16Z

services/api/src/admin_email_notification.py

@@ -145,6 +168,7 @@ def options(self):
        # CORS support
        return ("", 204, self.options_headers)

+    @require_permission(required_role=None)


Does this just generically verify an authenticated user?

Yes, this ensures a user is authorized without requiring any organization access. This is technically unnecessary since the middleware already enforces it, but this way every single method has a decorator and it's obvious that it wasn't accidentally skipped

payneBrandon · 2024-12-20T22:55:34Z

services/api/src/admin_intersection.py

@@ -153,15 +209,23 @@ def modify_intersection(intersection_spec):
        failed_value = failed_value.replace(")", '"')
        failed_value = failed_value.replace("=", " = ")
        logging.error(f"Exception encountered: {failed_value}")
-        return {"message": failed_value}, 500
+        raise InternalServerError(failed_value) from e
+    except InternalServerError:


Do we need this InternalSererError except?

same as above -

payneBrandon · 2024-12-23T15:22:26Z

services/api/src/admin_new_intersection.py

@@ -126,20 +138,18 @@ def add_intersection(intersection_spec):
        failed_value = failed_value.replace(")", '"')
        failed_value = failed_value.replace("=", " = ")
        logging.error(f"Exception encountered: {failed_value}")
-        return {"message": failed_value}, 500
+        raise InternalServerError(failed_value) from e
+    except InternalServerError:


Is this needed?

same as above -

services/api/src/admin_new_intersection.py

payneBrandon · 2024-12-23T15:58:11Z

services/api/src/admin_new_user.py

@@ -143,6 +146,23 @@ class AdminNewUserSchema(Schema):
    )


+def enforce_add_user_org_permissions(


We've got pretty similary "enforce_xxx_permissions" in several of these admin file additions. Could we abstract to a shared location to reuse code?

jacob6838 added 30 commits November 15, 2024 08:49

Pulling org from token in api

a37135c

Merging feature flags

0019096

Adding admin endpoint org-based auth

65bc034

Enforcing organization viewing/editing permissions

454b130

updating imports

2641c00

Update auth_tools.py

6e00ef0

Working api permissions enforcement

429aab2

adding _authorized to methods which enforce permissions

84fd32c

Adding include_super_user to get_qualified_org_list

92e6b0b

Adding exception handling for 400/403/500 errors

38d48b2

Adding exceptions for admin_new_user

6b7fe15

Handling errors with exceptions

7e74215

Merge branch 'api-exception-refactoring' into organization-permissions

125d6ff

Converting unauth responses to exceptions

336b44a

Moving requirements to prevent warnings

47cf1f5

denoting authorized methods

04fcff8

Merge branch 'develop' into organization-permissions

da0bf6e

consolidating query_and_return_list, adding auth_tools tests

cfb2c32

Fixing admin email notifications tests

5593a41

Fixing authorized unit tests

dedc1e7

Fixing remaining unit tests from auth additions

52bdbba

Formatting and type checking

05ede0a

Fixing typos

fe32d7d

transitioning to decorators for auth checks

88ed6fe

Working auth decorator

eb85031

Updating tests to use decorator

ba9254a

All tests working

87b09b5

bug fixes

f4759d0

Merge branch 'feature-flags' into organization-permissions

9e19cfa

Merge branch 'feature-flags' into organization-permissions

eae7157

jacob6838 added 2 commits December 5, 2024 13:11

Moving permissions to higher levels

5a9d106

Tweaking tests and removing unused imports

56825d9

drewjj reviewed Dec 12, 2024

View reviewed changes

jacob6838 requested a review from drewjj December 16, 2024 21:11

jacob6838 added 2 commits December 16, 2024 14:11

Cleaning up code, re-naming rs-geo-query

b14950a

Merge branch 'develop' into organization-permissions

1c21fed

mcook42 requested changes Dec 17, 2024

View reviewed changes

jacob6838 added 8 commits December 17, 2024 17:00

Merge branch 'feature-flags' into organization-permissions

471c4d3

Merge branch 'pytest-warnings' into organization-permissions

80e1118

Using werkzeung exceptions, 401 -> 403, minor fixes

73a2bb9

Tweaking organizations object key

0f31619

Adding @require_permission to every rest method

b5c0aa4

cleaning up unused imports

c02472f

re-working middleware auth enforcement

824dbe4

Merge branch 'develop' into organization-permissions

dae802f

payneBrandon requested changes Dec 23, 2024

View reviewed changes

payneBrandon reviewed Dec 23, 2024

View reviewed changes

Base automatically changed from feature-flags to develop January 13, 2025 19:44

jacob6838 mentioned this pull request Jan 31, 2025

Included Additional CDOT PRs in 2025 Q1 Release Notes #142

Merged

11 tasks

jacob6838 added 8 commits February 10, 2025 16:55

Merge branch 'develop' into organization-permissions

6deedd0

Fixing MyPy errors - sqlalchemy and auth_tools

7206bec

Cleaning up middleware

c7b3d24

Merge branch 'develop' into organization-permissions

36fbf14

Fixing mypy errors

ed66aa1

Fixing unit tests

9aef7f0

Enforcing org-based auth filtering in RsuSsmSrmData

4601236

Converting SQL exception catchers to SQLAlchemyError

50e5350

jacob6838 requested review from payneBrandon and mcook42 February 26, 2025 21:54

Merge branch 'develop' into organization-permissions

875a440

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforcing Organization and Role API Permissions #127

Enforcing Organization and Role API Permissions #127

jacob6838 commented Dec 5, 2024

drewjj left a comment

mcook42 left a comment

mcook42 Dec 17, 2024

jacob6838 Feb 26, 2025

mcook42 Dec 17, 2024

jacob6838 Feb 26, 2025

mcook42 Dec 17, 2024

jacob6838 Feb 26, 2025

mcook42 Dec 17, 2024

jacob6838 Feb 26, 2025

mcook42 Dec 17, 2024

jacob6838 Feb 26, 2025

payneBrandon left a comment

payneBrandon Dec 20, 2024

jacob6838 Feb 26, 2025

jacob6838 Feb 26, 2025

payneBrandon Dec 20, 2024

jacob6838 Feb 26, 2025

payneBrandon Dec 20, 2024

jacob6838 Feb 26, 2025

payneBrandon Dec 23, 2024

jacob6838 Feb 26, 2025

payneBrandon Dec 23, 2024

	rsu_dict = get_organization_rsus(permission_result)
	rsu_dict = get_organization_rsus(permission_result, [])

		@@ -143,6 +146,23 @@ class AdminNewUserSchema(Schema):
		)


		def enforce_add_user_org_permissions(

Enforcing Organization and Role API Permissions #127

Are you sure you want to change the base?

Enforcing Organization and Role API Permissions #127

Conversation

jacob6838 commented Dec 5, 2024

PR Details

Description

How Has This Been Tested?

Types of changes

Checklist:

drewjj left a comment

Choose a reason for hiding this comment

mcook42 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

payneBrandon 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