Fixed an issue where OAuth2 authentication fails with 'object has no attribute' if OAUTH2_AUTO_CREATE_USER is False. #9279#9691
Open
khushboovashi wants to merge 2 commits intopgadmin-org:masterfrom
Open
Conversation
…attribute' if OAUTH2_AUTO_CREATE_USER is False. pgadmin-org#9279
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughReplaced hard-coded strings with gettext-wrapped messages across the OAuth2 authentication flow and claim validation. Modified __auto_create_user to check for an existing user before creating; if not found and auto-creation is disabled, return a localized failure message. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/pgadmin/authenticate/oauth2.py`:
- Around line 699-700: The failure message returned by the "return False,
gettext(...)" line in oauth2.py is misleading for the branch where the user
isn't provisioned and auto-create is disabled; update that gettext string to
clearly state that the user exists in the identity provider but is not
provisioned in pgAdmin and automatic user creation is disabled (e.g., "User not
provisioned in pgAdmin and automatic user creation is disabled. Please contact
your administrator."), keeping it wrapped in gettext to preserve localization
and leaving the return tuple signature intact.
- Around line 710-712: The current check uses additional_claims.keys() is None
which is unreachable and causes empty dicts to be treated as failing; change the
logic to treat None or empty mappings as "no checks" and authorize: replace the
condition with a truthiness check (e.g., if not additional_claims:) and return
success (True, None) so an empty {} does not cause unauthorized; remove the
keys() is None check and ensure the rest of the additional_claims validation
only runs when additional_claims is non-empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b0245443-03fe-4cb3-9428-dda69e9bbcda
📒 Files selected for processing (1)
web/pgadmin/authenticate/oauth2.py
Comment on lines
+699
to
+700
| return False, gettext('No Email/Username found.' | ||
| ' Please contact your administrator.') |
There was a problem hiding this comment.
Failure message is inaccurate for this branch.
This branch means “user not provisioned and auto-create disabled,” not “no email/username found,” which can mislead admins during triage.
Suggested message tweak
- return False, gettext('No Email/Username found.'
- ' Please contact your administrator.')
+ return False, gettext(
+ 'User is not provisioned in pgAdmin and automatic '
+ 'user creation is disabled. Please contact your '
+ 'administrator.'
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/pgadmin/authenticate/oauth2.py` around lines 699 - 700, The failure
message returned by the "return False, gettext(...)" line in oauth2.py is
misleading for the branch where the user isn't provisioned and auto-create is
disabled; update that gettext string to clearly state that the user exists in
the identity provider but is not provisioned in pgAdmin and automatic user
creation is disabled (e.g., "User not provisioned in pgAdmin and automatic user
creation is disabled. Please contact your administrator."), keeping it wrapped
in gettext to preserve localization and leaving the return tuple signature
intact.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Localization
Bug Fixes