Skip to content
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

use command separated list for envs #2157

Merged
merged 3 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions spiffworkflow-backend/src/spiffworkflow_backend/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,25 @@ def _check_extension_api_configs(app: Flask) -> None:
)


def _set_up_open_id_scopes(app: Flask) -> None:
scopes = app.config["SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES"].split(",")
if os.environ.get("SPIFFWORKFLOW_BACKEND_OPENID_SCOPE") is not None:
app.logger.warning(
"SPIFFWORKFLOW_BACKEND_OPENID_SCOPE is deprecated. "
"Please use SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES instead which expects a comma separated list like: profile,email"
)
if os.environ.get("SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES") is None:
scopes = app.config["SPIFFWORKFLOW_BACKEND_OPENID_SCOPE"].split(" ")
if (
os.environ.get("SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES") is None
and app.config["SPIFFWORKFLOW_BACKEND_OPEN_ID_IS_AUTHORITY_FOR_USER_GROUPS"]
and "groups" not in scopes
):
scopes.append("groups")

app.config["SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES"] = scopes


# see the message in the ConfigurationError below for why we are checking this.
# we really do not want this to raise when there is not a problem, so there are lots of return statements littered throughout.
def _check_for_incompatible_frontend_and_backend_urls(app: Flask) -> None:
Expand Down Expand Up @@ -271,3 +290,4 @@ def setup_config(app: Flask) -> None:
_check_for_incompatible_frontend_and_backend_urls(app)
_check_extension_api_configs(app)
_setup_cipher(app)
_set_up_open_id_scopes(app)
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

The shell script results provide sufficient information to generate the final response. The verification shows:

  1. The old config SPIFFWORKFLOW_BACKEND_OPENID_SCOPE is still present in default.py alongside the new one
  2. There's proper format conversion handling in authentication_service.py using replace(",", ' ')
  3. 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_SCOPE entry at spiffworkflow-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


# Open ID server
# use "http://localhost:7000/openid" for running with simple openid
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ def get_login_redirect_url(self, state: str, authentication_identifier: str, red
+ f"?state={state}&"
+ "response_type=code&"
+ f"client_id={self.client_id(authentication_identifier)}&"
+ f"scope={current_app.config['SPIFFWORKFLOW_BACKEND_OPENID_SCOPE']}&"
+ f"scope={' '.join(current_app.config['SPIFFWORKFLOW_BACKEND_OPEN_ID_SCOPES'])}&"
+ f"redirect_uri={redirect_url_to_use}"
)
return login_redirect_url
Expand Down