Configure DAB so custom OIDC scopes and claims are supported#957
Configure DAB so custom OIDC scopes and claims are supported#957melissalkelly wants to merge 5 commits intoansible:develfrom
Conversation
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a conditional import in the OIDC discovery URL config: it attempts to use Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/oauth2_provider/urls.py`:
- Around line 9-13: The current bare "except ImportError" around importing
ConnectDiscoveryInfoView masks errors raised inside the optional module;
instead, use importlib.util.find_spec to check for the presence of
"aap_gateway_api.views.oidc_discovery" first and only fall back when the module
is absent. Concretely, replace the try/except around the import of
ConnectDiscoveryInfoView so that you call
importlib.util.find_spec("aap_gateway_api.views.oidc_discovery") and if it
returns None assign oauth_views.ConnectDiscoveryInfoView, otherwise import the
module and import ConnectDiscoveryInfoView normally (allowing any ImportError
raised during module initialization to propagate). Ensure you reference
ConnectDiscoveryInfoView and oauth_views in the updated code.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
ansible_base/oauth2_provider/urls.py
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ansible_base/oauth2_provider/urls.py`:
- Around line 11-14: find_spec("aap_gateway_api.views.oidc_discovery") can raise
ModuleNotFoundError when the parent package is absent; wrap the
importlib.util.find_spec call in a try/except (catch
ModuleNotFoundError/ImportError) and fall back to
oauth_views.ConnectDiscoveryInfoView on exception, or alternatively check
find_spec("aap_gateway_api") first before resolving the nested module; update
the block that sets ConnectDiscoveryInfoView to handle the exception so the
pipeline doesn't fail.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
ansible_base/oauth2_provider/urls.py
rh-pre-commit.version: 2.3.2 rh-pre-commit.check-secrets: ENABLED
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
|
There was a problem hiding this comment.
Requesting changes around this import pattern, I don't think we should pursue that.
I see that the jira story suggests overriding the discovery view. But in my read of the docs django-oauth-toolkit provides mechanisms to supply scopes/claims without rewriting the views: https://django-oauth-toolkit.readthedocs.io/en/latest/oidc.html#using-oidc-scopes-to-determine-which-claims-are-returned
I have this working in a proof-of-concept branch: AAP-60507-scopes-claims-poc. Specifically, see commit 9c68a9a from Jan 25.
In summary, we should be able to achieve this by referencing a custom validator class and the scope definitions it in settings like this:
"OAUTH2_VALIDATOR_CLASS": "ansible_base.workload_identity.CustomValidator",
"SCOPES": {"some-custom-scope": "Custom Scope Description"},
In my example, I have SCOPES defined in settings, but your SCOPES_BACKEND_CLASS in the other PR will achieve the same.
Now, if that doesn't do what we need, we can still write a custom ConnectDiscoveryInfoView, but let's make sure we're not importing from gateway here.
| try: | ||
| _gateway_spec = importlib.util.find_spec("aap_gateway_api.views.oidc_discovery") | ||
| except ModuleNotFoundError: | ||
| _gateway_spec = None | ||
|
|
||
| if _gateway_spec is not None: | ||
| from aap_gateway_api.views.oidc_discovery import ConnectDiscoveryInfoView |
There was a problem hiding this comment.
Nice ingenuity here in getting his to work, but I don't think we should be importing gateway from DAB. There's a pretty clear dependency relationship already and this is not a change we should make. Let's find a better way to achieve the scopes/claims customization.
2 participants
AAP-60507
Description
What is being changed?
The OIDC discovery endpoint URL pattern now uses a custom ConnectDiscoveryInfoView (if available) instead of always using the default django-oauth-toolkit view. Falls back to the default view when custom view is not present.
Why is this change needed?
Gateway needs to extend the OIDC discovery endpoint to dynamically populate scopes_supported and claims_supported with workload identity scopes and claims. Since DAB owns the OAuth2/OIDC URL routing, the custom view must be wired up here.
How does this change address the issue?
Allows Gateway to override the discovery view to include workload identity data in the response, while maintaining backwards compatibility for standalone DAB usage via the try/except fallback.
Type of Change
Self-Review Checklist
Testing Instructions
See testing in corresponding PR for AAP-60507
Summary by CodeRabbit