fix: admin consent screen appears again for user after admin approval#1997
Conversation
github-actions
Bot
added
frontend
|
Caution Review failedFailed to post review comments. GitHub was unavailable or timed out while CodeRabbit was posting the review. Please request a new review later if the pull request still needs one. This happened while posting 1 inline comment. Use ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
⏰ Context from checks skipped due to timeout. (1)
|
| Layer / File(s) | Summary |
|---|---|
Backend AUTH_PROMPT constants and propagation src/connectors/google_drive/oauth.py, src/connectors/onedrive/oauth.py, src/connectors/sharepoint/oauth.py, src/services/auth_service.py |
Each connector's OAuth class defines an AUTH_PROMPT constant ("consent" or "select_account"); AuthService.init_oauth reads it (default "consent") and includes prompt in the returned oauth_config. |
Frontend dynamic prompt usage frontend/app/api/mutations/useConnectConnectorMutation.ts, frontend/contexts/auth-context.tsx |
ConnectResponse.oauth_config gains an optional prompt field, and both the connect mutation and login flow build the authorization URL using result.oauth_config.prompt ?? "consent" instead of a fixed value. |
SharePoint OAuth Logging and Error-Handling Refactor
| Layer / File(s) | Summary |
|---|---|
Typing cleanup src/connectors/sharepoint/oauth.py |
Imports simplified to Any only, _current_account typed as `dict[str, Any] |
Credential loading and refresh migration src/connectors/sharepoint/oauth.py |
load_credentials() saves cache on upgrade and uses RESOURCE_SCOPES for silent acquisition; _refresh_from_json_token() builds consolidated error strings, detects specific auth failures, and simplifies exception handling. |
Authorization URL and callback handling src/connectors/sharepoint/oauth.py |
create_authorization_url() updates typing and kwargs construction; handle_authorization_callback() refactors error_msg derivation. |
Authentication and token acquisition src/connectors/sharepoint/oauth.py |
is_authenticated(), get_access_token(), and get_access_token_for_resource() gain expanded logging, fallback acquisition logic, and dedicated ValueErrors for admin-consent failures. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Sequence Diagram(s)
sequenceDiagram
participant Frontend
participant AuthService
participant ConnectorOAuthClass
Frontend->>AuthService: init_oauth(connector)
AuthService->>ConnectorOAuthClass: read AUTH_PROMPT
ConnectorOAuthClass-->>AuthService: AUTH_PROMPT value
AuthService-->>Frontend: oauth_config with prompt
Frontend->>Frontend: build authorization URL using prompt ?? "consent"
sequenceDiagram
participant Caller
participant SharePointOAuth
participant MSAL
Caller->>SharePointOAuth: get_access_token_for_resource()
SharePointOAuth->>MSAL: acquire_token_silent(account)
MSAL-->>SharePointOAuth: result or error
alt error contains AADSTS65001 or "consent"
SharePointOAuth-->>Caller: raise ValueError (admin consent required)
else other failure
SharePointOAuth-->>Caller: raise ValueError (chained)
end
Possibly related PRs
- langflow-ai/openrag#1657: Introduced the original hardcoded
prompt=consentlogic inuseConnectConnectorMutation.tsandauth-context.tsxthat this PR generalizes. - langflow-ai/openrag#1908: Overlaps with the same frontend/backend files and SharePoint OAuth adjustments for connector-specific prompt propagation.
Suggested labels: refactor
Suggested reviewers: mfortman11
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title matches the main change: fixing the OAuth flow so users are not re-prompted for admin consent after approval. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✨ Finishing Touches
📝 Generate docstrings
- Create stacked PR
- Commit on current branch
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Commit unit tests in branch
chery-pick-1908
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 help to get the list of available commands.
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
github-actions
Bot
added
bug
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/connectors/sharepoint/oauth.py (1)
348-467: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHydrate the token cache before these silent-acquisition paths.
load_credentials()is the only visible path here that deserializesself.token_fileintoself.token_cacheand restores_current_account. Bothget_access_token()andget_access_token_for_resource()now callacquire_token_silent(...)directly, so a freshly instantiated connector after process restart will fall into the failure path until some other caller has already runis_authenticated(). Please restore cache loading before these sync token getters, or move the hydration into connector initialization.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connectors/sharepoint/oauth.py` around lines 348 - 467, Both SharePoint silent token paths are using acquire_token_silent without ensuring the MSAL cache has been restored first. Update get_access_token() and get_access_token_for_resource() in SharePointOauth to hydrate state by calling the same cache-loading logic used by load_credentials() before any silent acquisition, or move that hydration into the connector’s initialization so _current_account and token_cache are always ready after restart.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/connectors/sharepoint/oauth.py`:
- Around line 3-4: The SharePoint OAuth module is still using stdlib logging
instead of the required project logger; update the imports and logger
initialization in oauth.py so it uses get_logger from utils.logging_config
rather than logging.getLogger. Remove the direct logging dependency, keep the
existing logger symbol in place for the rest of the module, and ensure any
logging calls in this file continue to use that get_logger-based logger.
---
Outside diff comments:
In `@src/connectors/sharepoint/oauth.py`:
- Around line 348-467: Both SharePoint silent token paths are using
acquire_token_silent without ensuring the MSAL cache has been restored first.
Update get_access_token() and get_access_token_for_resource() in SharePointOauth
to hydrate state by calling the same cache-loading logic used by
load_credentials() before any silent acquisition, or move that hydration into
the connector’s initialization so _current_account and token_cache are always
ready after restart.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 408905cd-c7e0-4bdd-881d-ca726629db7a
📒 Files selected for processing (6)
frontend/app/api/mutations/useConnectConnectorMutation.tsfrontend/contexts/auth-context.tsxsrc/connectors/google_drive/oauth.pysrc/connectors/onedrive/oauth.pysrc/connectors/sharepoint/oauth.pysrc/services/auth_service.py
| import os | ||
| from typing import Any |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Use get_logger here instead of stdlib logging.
This module still initializes logger via logging.getLogger(...) on Line 8, so the header cleanup leaves the file out of compliance with the src/**/*.py logging rule. Please switch this file to get_logger while you're already touching the imports. As per path instructions, src/**/*.py: Use get_logger from utils.logging_config — never import stdlib logging directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/connectors/sharepoint/oauth.py` around lines 3 - 4, The SharePoint OAuth
module is still using stdlib logging instead of the required project logger;
update the imports and logger initialization in oauth.py so it uses get_logger
from utils.logging_config rather than logging.getLogger. Remove the direct
logging dependency, keep the existing logger symbol in place for the rest of the
module, and ensure any logging calls in this file continue to use that
get_logger-based logger.
Source: Path instructions
3 participants
chore: Chery pick #1908
Summary by CodeRabbit
New Features
Bug Fixes