-
Notifications
You must be signed in to change notification settings - Fork 172
Make Directory.Read.All optional for non-admin workspace listing (graceful fallback) #4682
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
base: main
Are you sure you want to change the base?
Conversation
|
@richardogundele thanks for the PR, can you please update the title and description so we know what this does. Thanks. |
Unit Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 5cb2a6d. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a graceful fallback mechanism for Microsoft Graph API failures when listing workspaces for non-admin users, allowing deployments without Directory.Read.All permissions when user management is disabled.
Key changes:
- Adds conditional error handling in
get_identity_role_assignmentsto gracefully handle Graph API unavailability - Maintains strict behavior when
USER_MANAGEMENT_ENABLED=Truebut allows fallback when disabled - Provides comprehensive test coverage for both enabled and disabled user management scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| api_app/api/routes/resource_helpers.py | Adds try-catch logic with conditional fallback based on USER_MANAGEMENT_ENABLED config |
| api_app/tests_ma/test_api/test_routes/test_resource_helpers.py | Adds comprehensive tests for both fallback and strict mode behaviors |
|
@richardogundele can you create an issue for this explaining the error seen, the PR description explains the fix but not really the error. If we look back to why this PR exists, we need the issue. Thanks. |
Title
Non-admin GET /workspaces fails without Graph Directory.Read.All when user management is disabled
Problem
When
USER_MANAGEMENT_ENABLED=False, non-admin requests toGET /api/workspacescan fail if Microsoft Graph is unavailable orDirectory.Read.Allis not granted. The failure originates from role assignment resolution and bubbles up as an error, blocking workspace listing in least-privilege deployments.Impact
WorkspaceOwner,WorkspaceResearcher,AirlockManager).Directory.Read.Allor during Graph outages.Steps to Reproduce
USER_MANAGEMENT_ENABLED=Falseand without grantingDirectory.Read.Allto the API identity.GET /api/workspaces.AuthConfigValidationErrorfrom identity role assignment lookup.Expected
GET /api/workspacessucceeds and returns the user’s accessible workspaces (or an empty list), even when Graph is unavailable, whenUSER_MANAGEMENT_ENABLED=False.Actual
AuthConfigValidationError.Root Cause
get_identity_role_assignmentserrors (Graph-dependent) are not handled in the non-admin listing path. WithUSER_MANAGEMENT_ENABLED=False, we still hard-require Graph, creating an unnecessary dependency.What
Adds a graceful fallback for user role assignment resolution so non-admin
GET /workspacesdoes not hard-require Microsoft GraphDirectory.Read.All.Why
Reduces tenant permission friction and supports least-privilege deployments without breaking the endpoint.
How
api_app/api/routes/resource_helpers.py:get_identity_role_assignmentsnow catchesAuthConfigValidationErrorand:core.config.USER_MANAGEMENT_ENABLED=True: preserve strict behavior (re-raise).False: return an empty assignment list, avoiding a hard failure.Behavior Changes
USER_MANAGEMENT_ENABLED=True: unchanged; Graph is still required.USER_MANAGEMENT_ENABLED=False: endpoint no longer errors when Graph is unavailable; response may be empty if roles can’t be resolved.Tests
api_app/tests_ma/test_api/test_routes/test_resource_helpers.py[]when disabled.unittest.mock.patchandpytest.monkeypatchfor clean config toggling.Verification
USER_MANAGEMENT_ENABLED=Falseand simulate Graph failure →GET /workspacesreturns 200 with an appropriate list (possibly empty).USER_MANAGEMENT_ENABLED=Trueand simulate Graph failure → error continues to propagate as before.Notes
Related
#4682