use command separated list for envs #2157
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new function Changes
Sequence Diagram(s)sequenceDiagram
participant App as Flask App
participant Config as Config Module
participant Auth as Authentication Service
App->>Config: Initialize Configuration
Config->>Config: _set_up_open_id_scopes()
Config->>Config: Log warning if deprecated variable is set
Config->>Config: Populate SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES
App->>Auth: Request Login Redirect
Auth->>Config: Get OpenID Scopes
Config->>Auth: Return SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES
Auth->>Auth: Format scopes for URL
Auth->>App: Redirect to Login URL
Possibly related PRs
Warning Rate limit exceeded@jasquat has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 38 seconds before requesting another review. ⌛ 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. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
spiffworkflow-backend/src/spiffworkflow_backend/config/default.py (1)
Line range hint
234-234: Add deprecation warning comment for the old scope config.Since
SPIFFWORKFLOW_BACKEND_OPENID_SCOPEis being deprecated in favor ofSPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES, add a comment to warn users.-config_from_env("SPIFFWORKFLOW_BACKEND_OPENID_SCOPE", default="openid profile email") +# DEPRECATED: Use SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES instead. +# This config will be removed in a future release. +config_from_env("SPIFFWORKFLOW_BACKEND_OPENID_SCOPE", default="openid profile email")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
spiffworkflow-backend/src/spiffworkflow_backend/config/__init__.py(1 hunks)spiffworkflow-backend/src/spiffworkflow_backend/config/default.py(1 hunks)spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py(1 hunks)
🔇 Additional comments (2)
spiffworkflow-backend/src/spiffworkflow_backend/config/default.py (1)
126-126: LGTM! New OpenID scopes config follows consistent naming pattern.
The new configuration SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES is well-structured:
- Uses consistent naming pattern with other OpenID configs
- Default value maintains backward compatibility
spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py (1)
299-299: LGTM! The change correctly handles OpenID Connect scopes format.
The modification properly converts comma-separated scopes to space-separated format as required by the OAuth 2.0 specification for the scope parameter.
Let's verify the config usage across the codebase:
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check the transition handling:
The initial results show both old and new config variables are present, with what appears to be a transition mechanism. I need to verify the exact implementation of this transition and ensure it's handled correctly across the authentication flow.
Based on the comprehensive results, I can now generate the final response:
The change to use SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES is properly implemented and handled.
The codebase shows a well-managed transition:
- The new config variable is properly defined in
default.pywith correct default value "openid,profile,email" - A transition mechanism in
config/__init__.pyhandles backward compatibility - The scope format conversion from comma-separated to space-separated in the authentication URL follows OAuth 2.0 spec
- No unhandled references to the old config variable exist in authentication flows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the transition from old to new config variable
# Expected results:
# 1. No remaining usage of old config variable
# 2. New config variable is properly defined
# 3. All authentication-related code properly uses the new variable
# Check for any remaining usage of old config
echo "Checking for old config variable usage..."
rg "SPIFFWORKFLOW_BACKEND_OPENID_SCOPE"
# Check for new config variable definition and usage
echo "Checking new config variable definition and usage..."
rg "SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES"
# Check for any scope-related patterns in authentication code
echo "Checking scope-related patterns in authentication code..."
fd -e py -e ini -e cfg -e conf | xargs rg -l "scope.*=|=.*scope"
Length of output: 2233
Script:
#!/bin/bash
# Check the transition handling in config/__init__.py
echo "Checking config initialization..."
cat spiffworkflow-backend/src/spiffworkflow_backend/config/__init__.py
# Check the default configuration
echo -e "\nChecking default configuration..."
cat spiffworkflow-backend/src/spiffworkflow_backend/config/default.py
# Check usage in authentication-related files
echo -e "\nChecking authentication service usage..."
cat spiffworkflow-backend/src/spiffworkflow_backend/services/authentication_service.py
echo -e "\nChecking OpenID blueprint usage..."
cat spiffworkflow-backend/src/spiffworkflow_backend/routes/openid_blueprint/openid_blueprint.py
Length of output: 58538
spiffworkflow-backend/src/spiffworkflow_backend/config/__init__.py
Outdated
Show resolved
Hide resolved
spiffworkflow-backend/src/spiffworkflow_backend/config/__init__.py
Outdated
Show resolved
Hide resolved
spiffworkflow-backend/src/spiffworkflow_backend/config/__init__.py
Outdated
Show resolved
Hide resolved
| @@ -123,6 +123,7 @@ def config_from_env(variable_name: str, *, default: str | bool | int | None = No | |||
| config_from_env("SPIFFWORKFLOW_BACKEND_OPEN_ID_VERIFY_AZP", default=True) | |||
| config_from_env("SPIFFWORKFLOW_BACKEND_OPEN_ID_LEEWAY", default=5) | |||
| config_from_env("SPIFFWORKFLOW_BACKEND_OPEN_ID_INTERNAL_URL_IS_VALID_ISSUER", default=False) | |||
| config_from_env("SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES", default="openid,profile,email") | |||
There was a problem hiding this comment.
💡 Codebase verification
The shell script results provide sufficient information to generate the final response. The verification shows:
- The old config
SPIFFWORKFLOW_BACKEND_OPENID_SCOPEis still present indefault.pyalongside the new one - There's proper format conversion handling in
authentication_service.pyusingreplace(",", ' ') - There's a deprecation warning and conversion logic in
config/__init__.py
Remove the duplicate deprecated config entry
The format conversion is properly implemented, but there's a redundant configuration:
- Remove the deprecated
SPIFFWORKFLOW_BACKEND_OPENID_SCOPEentry atspiffworkflow-backend/src/spiffworkflow_backend/config/default.py
🔗 Analysis chain
Verify the scope format conversion implementation.
Let's verify the implementation that converts between comma-separated and space-separated formats, and check for any remaining usages of the old config.
Also applies to: 234-234
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of scope format conversion and usage of old config
# Check for occurrences of both old and new config variables
echo "Checking config usage..."
rg "SPIFFWORKFLOW_BACKEND_OPENID_SCOPE|SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES" -A 3
# Look for the scope format conversion implementation
echo -e "\nChecking scope format handling..."
rg -A 5 "scope.*=.*replace.*,.*\s" || rg -A 5 "scope.*=.*join.*\s"
Length of output: 3661
This deprecrates old
SPIFFWORKFLOW_BACKEND_OPENID_SCOPEenv var in favor ofSPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPESwhich is a comma separated list of space separated so it's safer to define as an env var.Summary by CodeRabbit
New Features
Bug Fixes