-
-
Notifications
You must be signed in to change notification settings - Fork 145
Allow OIDC scopes to be configured via P_OIDC_SCOPE environment variable #1363
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
… P_OIDC_SCOPE, default to "openid profile email".
WalkthroughA new configurable Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant OIDC_Handler
participant OIDC_Provider
User->>CLI: Starts application with --oidc-scope or env var
CLI->>OIDC_Handler: Passes configured scope to handler
User->>OIDC_Handler: Initiates login
OIDC_Handler->>OIDC_Provider: Redirects with dynamic scope in auth URL
OIDC_Provider-->>User: Handles authentication with requested scope
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/handlers/http/oidc.rs (1)
80-80
: Optimize string conversion to avoid unnecessary allocation.The string conversion
PARSEABLE.options.scope.to_string().as_str()
creates an unnecessary temporary string allocation. Sincescope
is already aString
, you can pass a string slice directly.Apply this diff to optimize the string passing:
- redirect_to_oidc(query, client, PARSEABLE.options.scope.to_string().as_str()) + redirect_to_oidc(query, client, &PARSEABLE.options.scope)- redirect_to_oidc(query, oidc_client, PARSEABLE.options.scope.to_string().as_str()) + redirect_to_oidc(query, oidc_client, &PARSEABLE.options.scope)Also applies to: 116-116
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/cli.rs
(1 hunks)src/handlers/http/oidc.rs
(4 hunks)src/handlers/mod.rs
(0 hunks)
💤 Files with no reviewable changes (1)
- src/handlers/mod.rs
🧰 Additional context used
🧠 Learnings (1)
src/handlers/http/oidc.rs (1)
Learnt from: nikhilsinhaparseable
PR: parseablehq/parseable#1340
File: src/query/mod.rs:64-66
Timestamp: 2025-06-18T06:39:04.775Z
Learning: In src/query/mod.rs, QUERY_SESSION_STATE and QUERY_SESSION serve different architectural purposes: QUERY_SESSION_STATE is used for stats calculation and allows dynamic registration of individual parquet files from the staging path (files created every minute), while QUERY_SESSION is used for object store queries with the global schema provider. Session contexts with schema providers don't support registering individual tables/parquets, so both session objects are necessary for their respective use cases.
🔇 Additional comments (2)
src/cli.rs (1)
454-463
: LGTM! Well-structured CLI configuration.The OIDC scope configuration is properly implemented with appropriate CLI argument mapping, environment variable support, and maintains backward compatibility with the default value.
src/handlers/http/oidc.rs (1)
226-241
: LGTM! Clean function signature update.The function signature change to accept a configurable scope parameter is well-implemented and maintains the existing authorization URL construction logic correctly.
I have read the CLA Document and I hereby sign the CLA |
@nitisht did this PR make sense? |
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.
approved
Description
This PR adds support for customizing the OIDC scopes used during authentication by introducing a new environment variable:
By default, the OIDC scope remains unchanged, as the current implementation:
openid profile email
If P_OIDC_SCOPE is set, its value will be used instead during the OIDC authorization request.
This allows integrations (e.g., with Dex) to request additional claims like groups when needed for RBAC.
Motivation
In some OIDC providers (e.g., Dex), claims such as groups are only included in tokens if explicitly requested via the scope parameter. Since Parseable currently hardcodes the OIDC scope, there is no way to request such claims without modifying the code.
This PR enables more flexible OIDC integrations and ensures that role-based access control can work correctly with group-based identity providers.
Changes Introduced
This PR has:
Summary by CodeRabbit
New Features
Refactor