refactor(jans-saml): shibboleth Identity Provider integration#13124
Merged
refactor(jans-saml): shibboleth Identity Provider integration#13124
Conversation
• Update installer to use Shibboleth for SAML authentication • Integrate Shibboleth Identity Provider into the all-in-one • Add Shibboleth identity provider to the all-in-one image • Saved progress at the end of the loop • Add shibboleth to docker image build workflow os • Add integration tests and validate parent chart dependency • Add comprehensive testing and documentation for Shibboleth IDP integration • Add OpenAPI specification and validation for Shibboleth plugin • Add Shibboleth Identity Provider documentation and navigation links • Add comprehensive documentation for Shibboleth IDP integration • Add Shibboleth IDP integration for SAML SSO authentication Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
moabu
requested review from
devrimyatar,
iromli,
pujavs,
yurem,
yuriyz and
yuriyzz
as code owners
Member
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Contributor
|
Important Review skippedToo many files! This PR contains 247 files, which is 97 over the limit of 150. You can disable this status message by setting the
📝 WalkthroughWalkthroughThis PR executes a comprehensive migration from Keycloak-based SAML to Shibboleth Identity Provider integration across the Janssen project, removing all Keycloak components while adding complete Shibboleth IDP support including Docker image, Helm charts, Config API plugin, Linux installers, Terraform resources, and documentation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
mo-auto
added
area-CI
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 88
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
docker-jans-config-api/scripts/upgrade.py (1)
116-130: 🧹 Nitpick | 🔵 TrivialConsider cleaning up stale "jans-keycloak-link" references during upgrade.
The change correctly stops adding "jans-keycloak-link" to new configurations. However, existing installations that already have "jans-keycloak-link" in their
jansServiceModulelist will retain this stale reference after the upgrade.While this is low-risk (the orphaned reference won't break functionality), consider adding cleanup logic for migration completeness:
♻️ Optional: Add cleanup for stale Keycloak references
match dir_mapping["directory"]: # add missing service module for `/opt/jans/jetty/%s/custom/libs` dir mapping case "/opt/jans/jetty/%s/custom/libs": + # remove deprecated keycloak module + if "jans-keycloak-link" in dir_mapping["jansServiceModule"]: + conf["assetMgtConfiguration"]["assetDirMapping"][idx]["jansServiceModule"].remove("jans-keycloak-link") + should_update = True + for svc_module in ["jans-lock", "jans-link"]: if svc_module in dir_mapping["jansServiceModule"]: continue conf["assetMgtConfiguration"]["assetDirMapping"][idx]["jansServiceModule"].append(svc_module) should_update = Truejans-cli-tui/setup.py (1)
29-34:⚠️ Potential issue | 🔴 CriticalKeycloak plugin removal is correct, but shibboleth-plugin swagger file needs to be added to plugins/docs for CLI-TUI access.
The removal of
kc-saml-plugin-swagger.yamlandkc-link-plugin-swagger.yamlaligns with the PR objectives to remove Keycloak components.However, the shibboleth-plugin swagger file exists at
jans-config-api/plugins/shibboleth-plugin/docs/shibboleth-plugin-swagger.yamlbut is not in the centralizedjans-config-api/plugins/docs/directory where setup.py downloads from. All other plugin swagger files are located inplugins/docs/. To enable CLI-TUI access to shibboleth plugin endpoints, either copyshibboleth-plugin-swagger.yamltojans-config-api/plugins/docs/and add'shibboleth-plugin-swagger.yaml'to the tuple in line 29, or adjust the download path to reference the plugin's own docs directory.jans-config-api/server/src/test/resources/json/openid/clients/openid-client-get.json (1)
635-695:⚠️ Potential issue | 🟡 MinorInconsistency: Description still references Keycloak.
The
clientName(line 635) anddisplayName(line 695) were updated tosaml_openid, but thedescriptionat line 694 still reads"Keycloak OpenID client used for SAML authentication".In the corresponding example file (
openid-clients-get-all.json), the description was properly updated to"OpenID client used for SAML authentication". This test fixture should be updated to match.🔧 Proposed fix
- "description": "Keycloak OpenID client used for SAML authentication", + "description": "OpenID client used for SAML authentication",jans-linux-setup/jans_setup/setup_app/installers/jans.py (1)
641-653:⚠️ Potential issue | 🔴 CriticalUpdate kc.service template to use new Shibboleth order variable or remove stale dependency.
The
order_services()method generatesorder_jans_shibboleth_idp_servicefor the renamed Shibboleth service, butkc.servicereferences%(order_saml_service)swhich is never set. This template variable will remain unresolved during rendering, breaking Keycloak's service startup ordering dependency.Additionally, there is no
jans-shibboleth-idp.servicesystemd unit file. If Shibboleth should be systemd-managed (not just a Jetty app), a service unit with proper ordering must be created.charts/janssen-all-in-one/README.md (1)
311-334: 🧹 Nitpick | 🔵 TrivialConsider removing deprecated kc-scheduler documentation.
The
kc-schedulersection remains in the README withenabled: false. Since this PR removes Keycloak components, consider removing this documentation entirely or adding a deprecation notice. Keeping it may cause confusion for users.docker-jans-all-in-one/Dockerfile (2)
193-224:⚠️ Potential issue | 🟠 MajorInclude Shibboleth in default CN_AIO_COMPONENTS.
The default list omits
jans-shibboleth, so the new component won’t start unless users override the env var.✅ Suggested fix
- CN_AIO_COMPONENTS="configurator,persistence-loader,jans-auth,jans-config-api,jans-fido2,jans-scim,jans-casa" \ + CN_AIO_COMPONENTS="configurator,persistence-loader,jans-auth,jans-config-api,jans-fido2,jans-scim,jans-casa,jans-shibboleth" \
253-327:⚠️ Potential issue | 🟠 MajorUpdate ownership paths to match the Shibboleth migration.
chown/chmodstill reference/opt/idpand/opt/keycloak/*(now removed), which can fail the build. Also, the new/opt/shibboleth-idpand/opt/shibboleth-jettypaths aren’t owned by uid 1000, which can break runtime writes for the non-root user.🧹 Suggested fix
RUN chown -R 1000:1000 ${JETTY_HOME}/temp \ @@ - /opt/idp \ - /opt/keycloak/logs \ - /opt/keycloak/conf + /opt/shibboleth-idp \ + /opt/shibboleth-jetty @@ RUN chmod -R g=u ${JETTY_HOME}/temp \ @@ - /opt/idp \ - /opt/keycloak/logs \ - /opt/keycloak/conf + /opt/shibboleth-idp \ + /opt/shibboleth-jetty
🤖 Fix all issues with AI agents
In `@charts/janssen-all-in-one/values.yaml`:
- Around line 538-544: YAML lint fails because empty mapping values use spaced
braces ("{ }"); change the two fields shibbolethLabels and
shibbolethAdditionalAnnotations to use compact empty mapping syntax without
spaces (shibbolethLabels: {} and shibbolethAdditionalAnnotations: {}) so the
values.yaml passes linting while preserving the same empty-map semantics.
In `@charts/janssen/charts/shibboleth-idp/templates/hpa.yaml`:
- Around line 16-27: The metrics block currently always emits "metrics:" even
when empty; change the template so the entire "metrics:" key is rendered only
when at least one metric is configured by wrapping the whole block in a single
conditional that checks {{ .Values.hpa.targetCPUUtilizationPercentage }} OR {{
.Values.hpa.metrics }} (i.e., render metrics: plus the CPU Resource entry and/or
toYaml .Values.hpa.metrics only when one of those values is set) — update the
hpa.yaml template accordingly to avoid emitting an empty spec.metrics list.
In `@charts/janssen/charts/shibboleth-idp/values.yaml`:
- Around line 80-96: The livenessProbe and readinessProbe use very high
failureThreshold (20) which delays failure detection; update the livenessProbe
and readinessProbe settings to lower thresholds and, if appropriate, reduce
periodSeconds so failures are detected faster—specifically change
livenessProbe.failureThreshold from 20 to 3 and livenessProbe.periodSeconds from
30 to 10, and change readinessProbe.failureThreshold from 20 to 3 (and
optionally readinessProbe.periodSeconds to 5–10) to ensure quicker restarts and
faster traffic cutover for unhealthy pods; adjust initialDelaySeconds only if
needed for startup timing.
In `@charts/janssen/README.md`:
- Line 411: The README entry for global.config-api.plugins currently lists
supported plugins in the description but omits Shibboleth; update the
description for the "global.config-api.plugins" table row so the supported
plugins list includes "shibboleth" (if the chart actually gates Shibboleth
endpoints by this flag) and adjust the example default value string (e.g., add
"shibboleth" to `"fido2,scim,user-mgt"`) to accurately reflect the real
supported plugin names and avoid misleading operators.
In `@docker-jans-config-api/Dockerfile`:
- Around line 60-64: The Dockerfile loop that downloads plugin jars (the for
plugin in ... loop referencing JETTY_BASE and CN_VERSION) removed kc-saml but
never added the new Shibboleth plugin; update that plugin list to include the
Shibboleth plugin name (e.g., add "shibboleth" to the space-separated plugins in
the for loop) so the wget command will fetch the corresponding
${plugin}-plugin-${CN_VERSION}-distribution.jar into
${JETTY_BASE}/jans-config-api/_plugins/.
In `@docker-jans-config-api/README.md`:
- Line 63: The documented plugin list for CN_CONFIG_API_PLUGINS is missing the
new "shibboleth" plugin; update the README entry that enumerates available
plugins (the line listing `admin-ui`, `scim`, `fido2`, `user-mgt`, `lock`,
`jans-link`) to include `shibboleth` so that CN_CONFIG_API_PLUGINS documentation
reflects all supported plugin names and clarifies that unknown names are
ignored.
In `@docker-jans-shibboleth/Dockerfile`:
- Around line 124-131: The image currently runs as root (no non-root user
defined) — add a dedicated low-privilege user and group (e.g., shib or
shibboleth) and switch to it before finalizing the image by adding a USER
instruction; ensure all writable directories used by ENTRYPOINT (entrypoint.py),
runtime services, and any config or cache paths are chowned/chmod'ed to that
user (update ownership for those paths referenced in scripts/resources) and
preserve required exec bits; keep ENTRYPOINT and CMD (ENTRYPOINT ["tini", ...]
and CMD ["python3", "/app/scripts/entrypoint.py"]) but run them as the new user
so the container no longer runs as root.
- Around line 61-67: Add a build arg to pin the Jans pycloudlib source to a
specific tag/commit and use that arg in the wget URL instead of hardcoding
refs/heads/main; e.g., introduce ARG JANS_PYCLOUDLIB_REF (default to a stable
tag or commit hash) and change the download URL to use
/archive/${JANS_PYCLOUDLIB_REF}.tar.gz so builds are reproducible and
supply-chain risk is reduced. Update the RUN block that currently references
refs/heads/main to interpolate the new ARG and ensure documentation or build
scripts pass the desired tag/commit when building the image.
- Around line 55-60: The Dockerfile currently masks download failures for the
IdP WAR by appending "|| echo", so the image can build without
${SHIBBOLETH_HOME}/war/idp.war; update the RUN that fetches CN_SOURCE_URL (the
line that uses wget and writes idp.war) to make the download mandatory: remove
the "|| echo" fallback and instead ensure the command exits non‑zero on failure
(e.g., use wget/curl options that fail on HTTP errors and then assert the file
exists/has nonzero size), so any failed download of the WAR causes the build to
fail and surfaces the error immediately.
In `@docker-jans-shibboleth/README.md`:
- Around line 13-23: The README's Environment Variables table is incomplete and
includes variables that may not exist in code; update it to reflect the actual
runtime variables used by the codebase and remove or clarify unused ones: add
CN_JAVA_OPTIONS (referenced in entrypoint.py), CN_MAX_RAM_PERCENTAGE
(entrypoint.sh), CN_HEALTH_CHECK_INTERVAL and CN_SHIBBOLETH_PORT (both in
healthcheck.py), and CN_PYCLOUDLIB_LOG_LEVEL (settings.py) — for each provide a
short description and a sensible default if applicable; verify whether
CN_CONFIG_ADAPTER and CN_SECRET_ADAPTER are actually referenced anywhere and
either remove them from the README or add a note/link explaining where they are
used and why; ensure each table row cites the source file (e.g., entrypoint.py,
entrypoint.sh, healthcheck.py, settings.py) so maintainers can trace usage.
In `@docker-jans-shibboleth/requirements.txt`:
- Around line 1-4: Update the cryptography requirement to a patched minimum:
change the version specifier for the package name "cryptography" in the
requirements list from "cryptography>=3.0" to "cryptography>=44.0.1" so the
dependency cannot resolve to vulnerable releases (42.0.0–44.0.0) and ensure any
CI/builds/use of pip install will pick a safe patched release.
In `@docker-jans-shibboleth/scripts/bootstrap.py`:
- Around line 132-135: The default value passed to os.environ.get for
JANS_SHIBBOLETH_WAR uses an unnecessary f-string; update the assignment to
war_src so the default string "/tmp/shibboleth-idp-src/war/idp.war" is a plain
string literal (remove the leading f from the quoted default), leaving the
os.environ.get call and the JANS_SHIBBOLETH_WAR environment key unchanged.
- Around line 57-78: The generate_sealer_key function currently generates a
random sealer password only when the keystore is created but does not persist it
or reuse it later; update generate_sealer_key to (1) when creating a new
sealer.jks, persist the chosen password (from IDP_SEALER_PASSWORD or generated
via get_random_chars) into your configuration store (e.g., write into
idp.properties or another stable file) immediately after keystore creation; (2)
when sealer.jks already exists and IDP_SEALER_PASSWORD is not set, read and
return the persisted password from idp.properties instead of generating a new
one; and (3) if sealer.jks exists and no persisted password is found, fail fast
with a clear error instructing the operator to set IDP_SEALER_PASSWORD so the
keystore and password remain consistent. Use the symbols generate_sealer_key,
sealer_file, IDP_SEALER_PASSWORD, and idp.properties to locate where to add
persistence/read logic.
- Around line 104-127: The setup_credentials method writes private key files
with default permissions; after writing the signing key and encryption key files
(idp-signing.key and idp-encryption.key in the credentials directory created by
setup_credentials), change their permissions to 0o600 to restrict access (use
os.chmod) and ensure os is imported at top of the module; apply the chmod call
immediately after each private key write in setup_credentials so keys are never
left world-readable.
In `@docker-jans-shibboleth/scripts/entrypoint.py`:
- Around line 28-35: The current logic appends "couchbase" to deps when
persistence_type is "couchbase" or "hybrid", but jans.pycloudlib.wait.wait_for
does not support a "couchbase" key so this is a no-op and can let services start
before Couchbase is ready; remove adding "couchbase" to the deps list and
instead implement an explicit Couchbase readiness check (e.g., a new
wait_for_couchbase or call into a Couchbase-specific readiness helper) that is
invoked when persistence_type == "couchbase" or "hybrid", or alternatively
extend jans.pycloudlib.wait.wait_for to accept a "couchbase" callback and
register that callback so couchbase readiness is actually awaited (update the
code paths around the deps variable and any wait_for calls to use the new
explicit check or the extended wait_for).
- Around line 61-66: In the os.execl call in entrypoint.py, remove the
unnecessary f-string prefix from the "-jar" argument (currently written as
f"-jar") so it becomes a plain string "-jar"; update the argument list passed to
os.execl (the call that includes "/usr/bin/java", "java",
f"-Didp.home={SHIBBOLETH_HOME}", "-Djava.io.tmpdir=/tmp", "-jar",
f"{jetty_home}/start.jar") to eliminate the redundant f-prefix while keeping
SHIBBOLETH_HOME and jetty_home f-strings intact.
In `@docker-jans-shibboleth/scripts/entrypoint.sh`:
- Line 24: The -XX:MaxRAMPercentage JVM argument uses an unquoted environment
variable CN_MAX_RAM_PERCENTAGE which can cause word-splitting and an invalid
argument if unset; update the entrypoint handling of -XX:MaxRAMPercentage to
quote the variable and provide a sensible default (e.g., use
"${CN_MAX_RAM_PERCENTAGE:-75}" or another default percent) so the command
becomes -XX:MaxRAMPercentage="..."; ensure the change targets the line that
constructs the JVM args containing -XX:MaxRAMPercentage and uses the
CN_MAX_RAM_PERCENTAGE variable.
In `@docker-jans-shibboleth/scripts/healthcheck.py`:
- Around line 28-39: The main() loop currently casts CN_HEALTH_CHECK_INTERVAL
with int() without validation and uses f-strings for logging; update main() to
catch ValueError (and TypeError) when reading os.environ, validate the parsed
interval is a positive integer (fallback to a safe default like 30 if invalid or
<=0), and use lazy %-style logging for messages (e.g., logger.info("Starting
health check loop with %ds interval", interval) and logger.debug("Health check
passed") / logger.warning("Health check failed") with %-style where parameters
exist); keep check_health() usage unchanged but ensure the loop uses the
validated interval variable.
- Around line 14-25: In check_health(), validate CN_SHIBBOLETH_PORT before
calling int() to avoid ValueError (e.g., treat non-numeric values as missing and
fall back to default 8080 or use try/except around int conversion and log a
warning), and change the logger.debug call in the exception handler to use lazy
%-style formatting (e.g., logger.debug("Health check failed: %s", e)) so the
exception string is computed only if the debug level is enabled; reference
function check_health, env var CN_SHIBBOLETH_PORT, and logger.debug to locate
the changes.
In `@docker-jans-shibboleth/scripts/settings.py`:
- Around line 18-23: The shibboleth logger configuration currently defines both
"handlers": ["console"] and "propagate": True which causes duplicate console
output; update the "shibboleth" logger entry (the block for the "shibboleth"
logger) to either remove the explicit "console" handler or set "propagate" to
False (matching the jans.pycloudlib logger) so logs are emitted only once, and
ensure the level remains set via os.environ.get("CN_SHIBBOLETH_LOG_LEVEL",
"INFO").
In `@docker-jans-shibboleth/scripts/shib_setup.py`:
- Around line 66-90: The configure_credentials method writes private keys and
secret files with default filesystem permissions; after writing each sensitive
file (e.g., idp-signing.key, idp-encryption.key, sealer.jks and any other secret
outputs like jans.properties referenced elsewhere) update their mode to a
restrictive permission (use os.chmod(path, 0o600) or stricter) so only owner can
read/write; locate file writes in configure_credentials and the other block
mentioned (around lines 220-235) and apply the same chmod call immediately after
each Path(...).write_text/...write_bytes operation to enforce 0o600.
- Around line 97-113: The properties block currently hardcodes
idp.sealer.storePassword and idp.sealer.keyPassword to "changeit"; instead read
these values from configuration/secrets (e.g., environment variables or a
secrets manager) and interpolate them into the props string (reference the props
template and the idp.sealer.storePassword / idp.sealer.keyPassword entries) so
production secrets are used and only a development default is applied when
explicitly in a dev mode; also ensure the provided sealer.jks password matches
the sourced secret and fail fast with a clear error if the secret is missing in
non-dev environments.
- Around line 51-59: The f-strings are used for static multi-line literals
(e.g., start_ini and the other static string variables in shib_setup.py) but
contain no interpolation; remove the unnecessary leading f from each of these
string assignments so they become plain string literals, i.e., replace
f"""...""" with """...""" for start_ini and the other four similar variables to
satisfy Ruff F541.
- Around line 15-18: The __init__ method lacks the explicit return type required
by ANN204; update the method signature for the initializer (the __init__ method
that accepts manager and sets self.manager, self.hostname, self.jans_auth_url)
to include "-> None" as its return annotation so the signature becomes def
__init__(self, manager) -> None: (keep parameter names and body unchanged).
In `@docker-jans-shibboleth/scripts/wait.py`:
- Line 22: The variable sleep_duration is assigned from CN_WAIT_SLEEP_DURATION
but never used; remove the unused variable assignment (sleep_duration) and
either use the environment value where needed (e.g., replace hardcoded sleep
calls with int(os.environ.get("CN_WAIT_SLEEP_DURATION", 10))) or simply delete
the sleep_duration line so no unused symbol remains; update any references to
rely on the environment variable name CN_WAIT_SLEEP_DURATION or the existing
time.sleep usage instead of the removed variable.
- Around line 42-47: The calls to wait_for_config, wait_for_secret, and
wait_for_persistence pass remaining_time as a second positional arg which will
raise TypeError; update the calls so wait_for_config(manager,
timeout=remaining_time) and wait_for_secret(manager, timeout=remaining_time) use
keyword arguments (or the exact kwarg name expected by pycloudlib if different)
and remove any extra argument for wait_for_persistence(manager) (it accepts only
manager). If you actually need timeout behavior and the library does not support
it, implement a local wrapper around wait_for_config/wait_for_secret that
enforces timeout using time.time() checks and retries, or add your own timeout
logic before calling wait_for_persistence.
In `@docs/janssen-server/shibboleth-idp/helm-deployment.md`:
- Around line 32-38: The helm install example uses the wrong flag; replace the
feature toggle `--set shibboleth-idp.enabled=true` with the global toggle `--set
global.shibboleth-idp.enabled=true` so it follows the Janssen Helm chart
convention (update the helm install snippet that currently sets
shibboleth-idp.enabled and ensure it uses global.shibboleth-idp.enabled
instead).
- Around line 255-273: The fenced code blocks under the headings "### Pod Not
Starting", "### Configuration Issues", and "### Authentication Failures" lack
blank lines separating the surrounding text from the ```bash``` blocks; update
the Markdown so there is a blank line before each opening triple-backtick and a
blank line after each closing triple-backtick (i.e., ensure an empty line
between the heading/paragraph and the ```bash``` block, and an empty line after
the closing ```), leaving the commands themselves unchanged.
- Around line 13-19: Update the "Prerequisites" section to note the
ingress-nginx project retirement status and recommend alternatives: modify the
bullet "Ingress controller (nginx, traefik, etc.)" under the "Prerequisites"
heading to mention that ingress-nginx (kubernetes/ingress-nginx) is in
maintenance and will not receive new releases after March 2026, and suggest
using Gateway API implementations or maintained controllers such as Traefik,
Contour, or service meshes (e.g., Istio) for new deployments; keep the note
concise and include a short guidance line for new installs to prefer Gateway API
or a maintained controller.
In `@docs/janssen-server/shibboleth-idp/installation.md`:
- Around line 36-45: Replace the hardcoded Docker image tag
'janssenproject/shibboleth:5.1.6_dev' in the shown run command with a
placeholder (e.g., '<version>') or a reference to a maintained variable so the
documentation does not require manual updates; locate the image string
'janssenproject/shibboleth:5.1.6_dev' in the installation example and change it
to 'janssenproject/shibboleth:<version>' (or describe using the latest/stable
tag) and update any surrounding text to explain how to select the version.
- Around line 61-62: Remove the deprecated top-level "version: '3.8'" entry from
the Docker Compose example in the installation.md file; edit the YAML so it
begins with the top-level services mapping (e.g., ensure "services:" is the
first key and that the existing "shibboleth:" service block remains correctly
indented under "services:") and do not add a Compose v2 "version" field.
- Around line 136-144: Update the hardcoded download URLs and filenames for
Shibboleth and Jetty to use the recommended newer releases: replace
"shibboleth-identity-provider-5.1.6" with "shibboleth-identity-provider-5.2.0"
in the wget and tar commands, and replace "jetty-home-12.0.25" with at least
"jetty-home-12.0.31" (or optionally "jetty-home-12.1.5") in the Jetty wget and
tar commands; ensure the tar filenames and extracted directory names match the
updated version strings so the commands wget
https://shibboleth.net/.../shibboleth-identity-provider-5.2.0.tar.gz and wget
https://repo1.maven.org/.../jetty-home-12.0.31.tar.gz (or
jetty-home-12.1.5.tar.gz) and their corresponding tar xzf invocations are
updated accordingly.
In `@docs/janssen-server/shibboleth-idp/README.md`:
- Around line 70-75: Update the "Version Information" block in README.md to
correct the Java version from "OpenJDK 17" to "OpenJDK 11" (this aligns with
maven.compiler.source and maven.compiler.target in jans-shibboleth-idp/pom.xml)
and change the Shibboleth IDP 5.1.6 release date from "January 2025" to "August
26, 2025" while leaving the Shibboleth version and Jetty version unchanged;
locate the "## Version Information" section in
docs/janssen-server/shibboleth-idp/README.md and update those two fields
accordingly.
In `@docs/janssen-server/shibboleth-idp/testing.md`:
- Around line 155-158: The label selector used by the kubectl commands (kubectl
wait --for=condition=ready pod and kubectl get pods -l) is incorrect; update
both commands to use the Helm-applied label key
app.kubernetes.io/name=shibboleth-idp instead of app=shibboleth-idp so they
match the deployment labels generated from _helpers.tpl (ensure both the wait
and the get commands use -l app.kubernetes.io/name=shibboleth-idp).
In `@jans-config-api/plugins/pom.xml`:
- Around line 18-26: The parent POM's <modules> list no longer references
kc-saml-plugin and kc-link-plugin while their directories still exist; either
delete the leftover directories jans-config-api/plugins/kc-saml-plugin and
jans-config-api/plugins/kc-link-plugin to complete the module removal, or re-add
<module>kc-saml-plugin</module> and/or <module>kc-link-plugin</module> into the
parent POM's modules block (the same section listing admin-ui-plugin,
jans-link-plugin, shibboleth-plugin, etc.) if those modules are still required;
verify shibboleth-plugin remains declared and unchanged.
- Around line 43-44: The maven-assembly-plugin is pinned to an old version
(artifactId maven-assembly-plugin, version 3.3.0); update the <version> value
for that plugin entry to 3.8.0 to use the latest stable release, then run a
local Maven build (mvn clean package) to confirm there are no compatibility
warnings or failures and adjust any plugin configuration if build errors
surface.
In
`@jans-config-api/plugins/shibboleth-plugin/docs/shibboleth-plugin-swagger.yaml`:
- Around line 120-134: The path parameter entityId used by the
/shibboleth/trust/{entityId} GET (operationId get-shibboleth-trust-by-id) must
be URL-encoded because SAML entity IDs are URLs; update the OpenAPI doc to state
that callers must URL-encode the entityId (e.g., encode https://sp.example.org
as https%3A%2F%2Fsp.example.org) and include an example in the parameter
description, or alternatively change the parameter from a path param to a query
param (move entityId from "in: path" to "in: query" and mark required) to avoid
encoding issues.
- Around line 78-90: The OpenAPI spec for the GET /shibboleth/trust endpoint
currently returns an unbounded array of TrustedServiceProvider; update the
operation to support pagination by adding query parameters (e.g., page, limit or
offset, pageSize) to the GET /shibboleth/trust operation and change the response
schema to either (a) a paginated wrapper object with properties like total,
page, pageSize and items: array of TrustedServiceProvider, or (b) add a maxItems
constraint on the existing array (e.g., items with maxItems) and document the
maximum; reference the operation path "/shibboleth/trust" and the schema
"TrustedServiceProvider" when making these changes so clients and server
implementations can honor paging.
- Around line 1-15: The OpenAPI spec lacks a global security declaration—add a
top-level security block and ensure a matching security scheme under
components.securitySchemes (e.g., define the bearer or apiKey scheme you use) so
endpoints inherit a default; modify the document around the existing
openapi/info/servers section to include a global security: - <scheme-name> and
add a components.securitySchemes entry (matching the scheme-name) describing
type, scheme, bearerFormat or in/name for apiKey, while leaving per-endpoint
security overrides intact.
In `@jans-config-api/plugins/shibboleth-plugin/pom.xml`:
- Around line 80-84: Remove the invalid Main-Class manifest entry that points to
io.jans.configapi.plugin.shibboleth.rest.ShibbolethResource (a JAX-RS resource
with no main method); edit the pom.xml plugin configuration to delete the
<mainClass> element (or the entire <archive><manifest>...</manifest></archive>
block) so the build does not set ShibbolethResource as the jar Main-Class.
In
`@jans-config-api/plugins/shibboleth-plugin/src/main/java/io/jans/configapi/plugin/shibboleth/model/ShibbolethIdpConfiguration.java`:
- Line 10: Remove the unused import by deleting the "import java.util.List;"
line from ShibbolethIdpConfiguration.java; ensure no other code in the class
(constructors, fields, methods) references List—if a List type is actually
needed, replace the unused import with the correct usage or fully qualify types
where appropriate.
In
`@jans-config-api/plugins/shibboleth-plugin/src/main/java/io/jans/configapi/plugin/shibboleth/rest/ShibbolethResource.java`:
- Around line 185-201: The 501 response in getIdpMetadata() currently sets a
plain-text entity while the method is annotated with
`@Produces`(MediaType.APPLICATION_XML); remove the entity so the NOT_IMPLEMENTED
response has an empty body: change the Response returned in getIdpMetadata()
(which currently uses Response.Status.NOT_IMPLEMENTED and .entity(...)) to
simply build() the response without an entity, ensuring the response matches the
declared XML content type and follows the established pattern (e.g.,
OTPEnrollingWS).
- Around line 71-76: The OpenAPI `@ApiResponse` for the ShibbolethResource list
endpoint currently documents a single TrustedServiceProvider object but the
method returns List<TrustedServiceProvider>; update the response schema on the
method (the annotation block with operationId "get-shibboleth-trust") to use
`@ArraySchema`(schema = `@Schema`(implementation = TrustedServiceProvider.class)) so
the content declares an array of TrustedServiceProvider objects (keep
MediaType.APPLICATION_JSON and existing responseCode 200 and description).
- Around line 146-158: The request body parameter is not validated and can be
null, causing NPEs in updateTrustedServiceProvider (when calling
serviceProvider.setEntityId) and addTrustedServiceProvider (when calling
serviceProvider.getEntityId); add validation annotations (`@Valid` and `@NotNull`)
to the TrustedServiceProvider method parameters in both methods and add an
explicit null check returning Response.status(Response.Status.BAD_REQUEST) (or
similar) if the parameter is null before using it to ensure safe handling and
clear client feedback.
- Around line 121-132: The addTrustedServiceProvider method currently
dereferences serviceProvider.getEntityId() without checks; update the method in
ShibbolethResource to validate input: if the incoming TrustedServiceProvider
parameter is null or serviceProvider.getEntityId() is null/blank, return
Response.status(Response.Status.BAD_REQUEST) with a clear message and do not
call shibbolethService.getTrustedServiceProvider or
shibbolethService.addTrustedServiceProvider; optionally annotate the parameter
with `@Valid` and/or `@NotNull` on the method signature to mirror other plugins, but
ensure the explicit null/blank checks and 400 responses are in place before any
dereference.
In
`@jans-config-api/plugins/shibboleth-plugin/src/main/java/io/jans/configapi/plugin/shibboleth/service/ShibbolethService.java`:
- Around line 73-84: addTrustedServiceProvider currently assumes
getConfiguration().getShibbolethIdpProperties() and its trustedServiceProviders
list are non-null and blindly appends; update the method
(addTrustedServiceProvider) to null-check getShibbolethIdpProperties() and
instantiate/set a new ShibbolethIdpProperties object when absent, ensure the
providers list is initialized if null, then scan the providers list for an
existing TrustedServiceProvider with the same getEntityId() and if found
return/throw the appropriate 409-conflict error (per your API convention)
instead of adding a duplicate, otherwise add the serviceProvider and call
updateConfiguration(config).
- Around line 101-108: The deleteTrustedServiceProvider method can NPE if
getTrustedServiceProviders() returns null and currently silently no-ops when the
entityId is missing; to fix, in ShibbolethService.deleteTrustedServiceProvider
obtain ShibbolethIdpConfiguration via getConfiguration(), retrieve the providers
list via getTrustedServiceProviders(), if the list is null throw a not-found (or
appropriate WebApplicationException/ResourceNotFoundException) for the entityId,
otherwise attempt to remove matching provider(s) (e.g., find by
p.getEntityId().equals(entityId)), and if none matched throw the same not-found
exception; only call updateConfiguration(config) when a provider was actually
removed. Ensure you reference deleteTrustedServiceProvider,
getTrustedServiceProviders, ShibbolethIdpConfiguration, and updateConfiguration
when making the changes.
- Around line 27-36: The getConfiguration method currently catches all
Exceptions which hides real errors; update it to only catch the
persistence-layer "not found" exception (e.g., EntryNotFoundException /
NoResultException used by persistenceEntryManager.find) and return
createDefaultConfiguration() in that case, while letting other exceptions
propagate (or log and rethrow) so connectivity/permission failures are not
masked; modify the catch around persistenceEntryManager.find in getConfiguration
to catch the specific not-found exception and handle other exceptions by logging
an error and rethrowing.
- Around line 86-99: updateTrustedServiceProvider currently risks NPE because
config.getShibbolethIdpProperties().getTrustedServiceProviders() may be null and
silently no-ops if the entityId isn't found; update the method in
ShibbolethService to first null-check (or initialize) the list returned by
getShibbolethIdpProperties().getTrustedServiceProviders() to avoid NPE, then
search for the matching TrustedServiceProvider by entityId and if not found
either return a boolean (e.g., false) or throw a not-found exception so callers
(REST layer) can return 404; ensure you reference/update the getConfiguration(),
ShibbolethIdpProperties, and updateConfiguration(config) usage accordingly.
In
`@jans-config-api/plugins/shibboleth-plugin/src/main/java/io/jans/configapi/plugin/shibboleth/ShibbolethPluginApplication.java`:
- Around line 29-36: Update the SecurityScheme tokenUrl in
ShibbolethPluginApplication's `@SecurityScheme` annotation to use the complete
token endpoint path rather than the placeholder; replace
"https://{op-hostname}/.../token" with the actual endpoint used in the OpenAPI
spec (e.g., "https://{op-hostname}/jans-auth/restv1/token") so the annotation's
OAuthFlow tokenUrl matches the shibboleth-plugin-swagger.yaml definition.
In
`@jans-config-api/plugins/shibboleth-plugin/src/test/java/io/jans/configapi/plugin/shibboleth/test/ShibbolethResourceTest.java`:
- Around line 195-204: The cleanupTestSp method currently swallows exceptions;
update its catch block to include the thrown exception details in the log (e.g.,
pass the exception object to log.warn or append e.getMessage()/e) so failures
show the cause and stacktrace for debugging; locate cleanupTestSp and modify the
catch for Exception e (which wraps the HTTP delete using accessToken,
propertiesMap, and SHIBBOLETH_TRUST_ENDPOINT) to log the exception alongside the
existing message instead of only a generic message.
- Around line 137-177: The test updateConfiguration_shouldReturn200 overwrites
the live Shibboleth config and never restores it; modify the test to capture the
original configuration (e.g., String originalConfig =
getResponse.getBody().asString()) before performing the PUT, then perform the
update inside a try block and restore the originalConfig in a finally block by
issuing a PUT to the same SHIBBOLETH_CONFIG_ENDPOINT with the saved
originalConfig and the same Authorization header (use accessToken); ensure any
assertions remain but restoration always runs even if assertions fail.
- Around line 104-119: In the
getTrustedServiceProvider_nonExistent_shouldReturn404 test the request URL is
built by concatenating nonExistentId (which contains characters like ':' and
'/') into the path; replace that concatenation with a proper path parameter or
URL-encoding: set the path parameter for "entityId" (e.g., using
given().pathParam("entityId", nonExistentId) and call
get(propertiesMap.get("shibbolethUrl") + SHIBBOLETH_TRUST_ENDPOINT +
"/{entityId}")) or alternatively URL-encode nonExistentId with
URLEncoder.encode(nonExistentId, StandardCharsets.UTF_8) before appending;
update the test method getTrustedServiceProvider_nonExistent_shouldReturn404
accordingly to use the pathParam or encoded value.
In
`@jans-config-api/plugins/shibboleth-plugin/src/test/resources/test.properties`:
- Around line 9-10: The test properties are missing required SSA scopes causing
Shibboleth config-api tests to fail; update the scopes property named "scopes"
to include the three SSA scopes (https://jans.io/auth/ssa.admin,
https://jans.io/auth/ssa.portal, https://jans.io/auth/ssa.developer) in addition
to the existing shibboleth.readonly and shibboleth.write entries so the line
contains all five space-separated scope URLs.
In
`@jans-config-api/server/src/main/resources/example/idp/trust-idp/post-saml-identity-provider.json`:
- Line 4: The JSON file has "displayName" and "description" concatenated on one
line which breaks the repository's formatting consistency; update the object so
"displayName" and "description" are on separate lines with proper indentation
and a trailing comma where required (ensure the "displayName" line ends with a
comma and "description" is its own indented line), adjusting spacing to match
other example files and maintaining valid JSON around the surrounding
properties.
In `@jans-config-api/server/src/main/resources/example/saml/config/saml-put.json`:
- Around line 6-16: The example JSON has inconsistent path formatting:
idpRootDir contains a trailing slash while the idpConfigs[0].rootDir does not;
update saml-put.json so both idpRootDir and the idpConfigs[].rootDir values use
the same format (preferably remove the trailing slash and set idpRootDir to
"/opt/shibboleth-idp" to match rootDir), or alternatively add trailing slashes
to both — ensure idpRootDir, rootDir and any related fields in idpConfigs are
consistent so downstream code (e.g., metadataDir/metadataTempDir resolution)
won't produce double or missing slashes.
In `@jans-config-api/server/src/main/resources/example/token/get-all-token.json`:
- Line 31: The "scope" value contains a stray token "readonly" between
"https://jans.io/oauth/config/saml-scope.write" and
"https://jans.io/oauth/config/user.readonly"; open the JSON "scope" entry and
either remove the standalone "readonly" token or replace it with the correct
namespaced scope (e.g., prefix with "https://jans.io/..." if that was intended)
so the scope list is consistent; verify the corrected token preserves the
intended permission and update the string in the "scope" field accordingly.
In `@jans-linux-setup/jans_setup/jans_setup.py`:
- Around line 146-147: The import was changed to ShibbolethInstaller but the
code still instantiates JansSamlInstaller(), causing a NameError; replace the
JansSamlInstaller() instantiation with ShibbolethInstaller() where
Config.profile == 'jans' and rename the variable (e.g., jans_saml_installer ->
shibboleth_installer) or keep the original variable name consistently; also
update any subsequent references to that variable (the usages currently around
the later installer handling) so they match the new installer class/variable
name.
In `@jans-linux-setup/jans_setup/setup_app/installers/jans.py`:
- Around line 508-513: The add_yacron_job routine currently appends jobs
blindly; update add_yacron_job in setup_utils.py to load the existing yacron
config (cron-jobs.yaml), search for any job with the same name as the incoming
job (e.g., name='super-gluu-license-renewer'), and remove or replace that entry
before appending the new job; alternatively, if running on a fresh install
detect that state and replace the entire jobs list atomically. Ensure the
implementation reads/writes the same YAML keys used by the current code, handles
missing files gracefully, and keeps the function signature and callers (like the
call from jans.py) unchanged.
In `@jans-linux-setup/jans_setup/setup_app/installers/shibboleth.py`:
- Line 54: Replace the hardcoded "Installing Shibboleth IDP 5.1.6" log message
in the self.logIt call with a dynamic version value (either from
base.current_app.app_info where the Shibboleth version is stored or a class
constant like SHIBBOLETH_VERSION) so the logged version stays in sync with the
actual installed version; update the self.logIt invocation to format the message
using the selected symbol (e.g., base.current_app.app_info['shibboleth_version']
or self.SHIBBOLETH_VERSION).
- Around line 133-157: Wrap the critical credential-generation calls (the three
self.run invocations that create the signing cert/key, encryption cert/key, and
the keytool sealer keystore) in try/except so failures are caught and surfaced;
when calling run() for the openssl and keytool commands, check the returned
result or catch exceptions from run(), log a clear error including the command
and any stdout/stderr, and abort (raise an exception or exit) if the command
failed; ensure you preserve setting Config.shibboleth_sealer_password from
getPW() only after successful keystore creation and reference the
methods/variables run(), getPW(), and Config.shibboleth_sealer_password to
locate the exact spots to modify.
- Around line 203-204: The PR introduces two similar directory-creation
methods—create_folders and create_directories—causing potential confusion;
consolidate them by removing create_folders and having callers use
create_directories (or vice versa) so directory creation is centralized. Update
references that call create_folders to call create_directories (or move the
create_folders logic into create_directories) and ensure
createDirs(self.output_dir) behavior is preserved inside create_directories; if
create_folders is required by an external lifecycle hook, refactor
create_folders to delegate to create_directories (i.e., call create_directories)
so there is a single source of truth for directory creation.
- Around line 166-175: The variable jans_properties_output in
configure_jans_authentication is assigned but never used; remove that unused
assignment to clean up the function. Edit the configure_jans_authentication
method to delete the line that defines jans_properties_output and leave the
template rendering and Config.templateRenderingDict updates intact (references:
configure_jans_authentication, jans_properties_output, renderTemplateInOut).
- Around line 159-164: In configure_idp_properties remove the unused local
variable idp_properties_output (assigned but never used) to avoid dead code;
either delete the line that sets idp_properties_output or, if intended, pass
that path into renderTemplateInOut instead of the current os.path.join(...)
call—look for the configure_idp_properties function and the renderTemplateInOut
invocation when making the change.
- Line 79: The assignment to jans_scopes from self.dbUtils.get_scopes() is
unused; remove the unused variable to avoid dead code by deleting the line that
sets jans_scopes and keep the subsequent individual scope lookups (calls to
self.dbUtils.get_scope/<whatever methods> on lines following this). Ensure no
other code references jans_scopes (if it does, replace those references with
direct calls to self.dbUtils.get_scopes() or the specific scope lookups).
- Around line 1-9: The import list contains unused modules; remove the unused
imports json, datetime, tempfile, and Path from the top of shibboleth.py while
keeping os, shutil and glob (glob is referenced later around the usage at line
~192). Update the import statement(s) so only the needed modules remain (e.g.,
keep "import os", "import glob", "import shutil") to eliminate dead imports.
- Around line 210-211: There is a consistent typo: the dict key
'jvm_heap_ration' should be 'jvm_heap_ratio'; update every occurrence of that
key (e.g., in the 'memory' dict in shibboleth.py and the matching entries in
jetty.py and collect_properties.py) and also update any code that reads that key
to use 'jvm_heap_ratio' (search for 'jvm_heap_ration' and replace, ensuring
default lookups, property builders, or tests referencing the old name are
adjusted to the new key so behavior remains unchanged).
- Around line 187-193: The copy_static_files method iterates entries from
static_dir and calls shutil.copy on each, which raises IsADirectoryError for
subdirectories; update copy_static_files to distinguish files vs directories by
checking os.path.isfile(f) and calling shutil.copy for files, and for
directories use shutil.copytree(os.path.join(self.shibboleth_home,
os.path.basename(f))) (or skip directories) and handle existing destination
directories appropriately; reference copy_static_files, static_dir, and
self.shibboleth_home when making the change.
In `@jans-linux-setup/jans_setup/templates/jans-shibboleth-idp/config.ldif`:
- Line 5: The LDIF single line for jansConfApp (the JSON value containing
"shibbolethIdp" and placeholders like %(shibboleth_idp_entity_id)s) exceeds the
76‑char RFC 2849 limit; fold the JSON value across multiple lines by inserting
line breaks and making each continuation line start with a single space
character so the LDIF parser treats them as continuations (preserve the exact
JSON content and placeholders, just break the long string into concatenated
continuation lines beginning with a space).
In `@jans-linux-setup/jans_setup/templates/jans-shibboleth-idp/idp.properties`:
- Around line 1-2: The comment line "Shibboleth IDP 5.1.6 Configuration"
hardcodes the version and should be made maintainable; either replace the
literal "5.1.6" with a template variable (e.g., ${shibboleth.version} or a
build/property placeholder) so the value is injected from the build
configuration, or remove the version token entirely from the comment in
idp.properties to avoid drift—locate the exact comment line containing
"Shibboleth IDP 5.1.6 Configuration" and update it accordingly.
In
`@jans-shibboleth-idp/idp-conf/src/main/resources/conf/authn/jans-authn-config.xml`:
- Around line 2-10: The beans root currently declares an unused XML namespace
xmlns:context (and its corresponding context schema URL in xsi:schemaLocation);
remove the unused xmlns:context attribute from the <beans> element and also
remove the matching "http://www.springframework.org/schema/context
http://www.springframework.org/schema/context/spring-context.xsd" pair from the
xsi:schemaLocation so the file no longer references the unused context
namespace.
In `@jans-shibboleth-idp/idp-conf/src/main/resources/conf/jans.properties`:
- Line 1: Add a clear header comment block at the top of the jans.properties
file that documents which properties are required vs optional (explicitly call
out client.id and client.secret as mandatory), list prerequisites such as "OAuth
client must be pre-registered in Janssen Auth Server with a matching redirect
URI", and state whether changes require an IDP restart to take effect (or which
settings are hot-reloadable if applicable); reference the exact property keys
(e.g., client.id, client.secret, redirect.uri) in the comments so operators can
quickly see requirements and deployment steps.
In
`@jans-shibboleth-idp/keygenerator/src/main/java/io/jans/idp/keygen/KeyGenerator.java`:
- Around line 17-18: The generateSelfSignedCertificate method uses fully
qualified BouncyCastle types; to improve readability, add imports for the
BouncyCastle classes used (e.g., X500Name, ContentSigner,
JcaContentSignerBuilder, X509v3CertificateBuilder/JcaX509v3CertificateBuilder,
JcaX509CertificateConverter, and any other BC types referenced) at the top of
KeyGenerator.java and then replace the fully qualified names in the
generateSelfSignedCertificate method with the short class names to reduce
verbosity while preserving existing logic.
- Line 92: Remove the unused local variable X500Principal subject that is
created from subjectDn in KeyGenerator.java; delete the declaration
"X500Principal subject = new X500Principal(subjectDn)" and rely on the existing
X500Name constructions that use subjectDn (the X500Name creation code around
where subjectDn is used) so there are no unused imports or variables left.
- Around line 70-72: The inline key generation at the top of KeyGenerator
duplicates existing logic in the class; replace the manual KeyPairGenerator
creation with a call to the class's generateKeyPair(...) helper (the
generateKeyPair method in KeyGenerator) to follow DRY. Call the appropriate
generateKeyPair overload with the same keySize (and algorithm if applicable),
ensure the same SecureRandom usage and exception handling behavior is retained,
and remove the duplicated KeyPairGenerator/initialize/generateKeyPair block.
- Line 40: The keySize parsing in KeyGenerator (int keySize = args.length > 5 ?
Integer.parseInt(args[5]) : DEFAULT_KEY_SIZE;) lacks validation and can throw
NumberFormatException or accept insecure sizes; update the parsing in the
KeyGenerator main (or method that reads args) to catch NumberFormatException for
args[5], log/print a clear error and exit or fall back to DEFAULT_KEY_SIZE, and
then validate the parsed value is >= 2048 (or another configured minimum) before
use; reference the keySize variable and DEFAULT_KEY_SIZE when implementing the
parse, try/catch, and minimum-size check so invalid or too-small inputs produce
a friendly error and do not proceed.
In `@jans-shibboleth-idp/pom.xml`:
- Around line 20-35: Update the Maven compiler properties to target Java 17:
change the values of the maven.compiler.source and maven.compiler.target
properties in the POM's <properties> section (the symbols maven.compiler.source
and maven.compiler.target) from "11" to "17" so the project builds against Java
17 required by Spring 6.2.10 and Shibboleth 5.1.6; ensure any related build
plugins or toolchains (if present) are consistent with Java 17 after making this
change.
In
`@jans-shibboleth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/action/ProcessJansCallbackAction.java`:
- Around line 54-74: Fetch or create the JansAuthenticationContext before
checking the OAuth error and, when error != null, populate its error message
field with the combined error and error_description (e.g., error + " - " +
errorDescription) before firing ActionSupport.buildEvent; use
authenticationContext.getSubcontext(JansAuthenticationContext.class, true) to
obtain/create the context and call the appropriate setter (e.g.,
setErrorMessage) or assign to jansContext.errorMessage so DisplayError can
render the message, then return as before.
In
`@jans-shibboleth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/action/RedirectToJansAction.java`:
- Around line 15-16: The imports org.springframework.webflow.execution.Event and
org.springframework.webflow.execution.RequestContext in RedirectToJansAction are
unused; remove those two import lines so the class no longer contains unused
imports (locate them at the top of RedirectToJansAction.java and delete the
lines referencing Event and RequestContext).
- Around line 52-61: doExecute and getRedirectUrl duplicate the logic to fetch
JansAuthenticationContext and read externalProviderUri; refactor doExecute to
call getRedirectUrl(authenticationContext) instead of repeating retrieval.
Update doExecute (which currently accesses JansAuthenticationContext directly)
to use the returned String from getRedirectUrl, handle the null case the same
way it currently does, and keep JansAuthenticationContext and
getExternalProviderUri usage only inside getRedirectUrl to centralize the logic.
In
`@jans-shibboleth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/context/JansAuthenticationContext.java`:
- Line 3: Remove the unused import of javax.annotation.Nonnull from the
JansAuthenticationContext class; locate the import statement "import
javax.annotation.Nonnull;" at the top of the JansAuthenticationContext.java file
and delete it so there are no unused imports in the class.
- Line 16: Rename the misspelled field relayingPartyId to relyingPartyId and
update its accessor methods and usages: change the field name in
JansAuthenticationContext, rename getRelayingPartyId() to getRelyingPartyId()
and setRelayingPartyId(...) to setRelyingPartyId(...), and update the toString()
implementation to reference relyingPartyId; then update all callers (e.g.,
InitializeJansAuthenticationAction.setRelayingPartyId(...) ->
setRelyingPartyId(...) and JansAuthenticationService.getRelayingPartyId() ->
getRelyingPartyId()) so compilation succeeds and the SAML term is corrected
throughout. Ensure any serialization annotations or reflection-based access (if
present) are adjusted to the new name.
In
`@jans-shibboleth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/impl/JansAuthenticationService.java`:
- Around line 123-126: The code currently calls
extractUserPrincipal(tokenResponse) without validating the ID token; update
JansAuthenticationService to pass the JansAuthenticationContext into
extractUserPrincipal (e.g., extractUserPrincipal(tokenResponse, context)) and
implement validation inside that method: verify the ID token signature using the
authorization server's public key, validate standard claims (iss, aud, exp, iat)
against the configured issuer/audience and current time, and enforce nonce ===
context.getNonce(); if any check fails, treat the token as invalid (do not
return a principal and do not mark the context authenticated) and fall back to
getUserInfoSubject only for valid access tokens.
In
`@jans-shibboleth-idp/shib-jans-authn/src/main/resources/flows/authn/jans-authn-beans.xml`:
- Around line 10-22: The scopes for the jansAuthenticationService bean are
hardcoded; change the <property name="scopes"> to use a property placeholder
(e.g. refer to a new property jans.auth.scopes) instead of literal <value>
entries and ensure io.jans.idp.authn.impl.JansAuthenticationService can accept a
delimited string (or update it to parse a space/comma-delimited property into a
List) or alternatively wire Spring's util:list from a property; update
configuration and the JansAuthenticationService constructor/setter accordingly
to split/convert the property into the required List<String>.
In `@pyproject.toml`:
- Line 4: Replace the placeholder value for the toml key description in
pyproject.toml (the line currently reading description = "Add your description
here") with a concise, accurate project description that summarizes the
library's purpose and main features; update the description string to reflect
the actual project intent and audience before release so published package
metadata is meaningful.
In `@terraform-provider-jans/jans/resource_shibboleth_configuration.go`:
- Around line 63-69: The code uses d.GetOk("metadata_providers") which treats
empty lists as absent and prevents clearing providers; fix this by enforcing
non-empty lists at the schema level and reading the attribute unconditionally:
add MinItems: 1 to the "metadata_providers" schema entry so empty lists are not
allowed, and replace the d.GetOk branch with an unconditional read (use
d.Get("metadata_providers") → iterate the []interface{} to build providers) and
assign to config.MetadataProviders; reference symbols: metadata_providers,
d.GetOk, d.Get, config.MetadataProviders.
In `@terraform-provider-jans/jans/resource_shibboleth_trusted_sp.go`:
- Around line 84-90: The code uses d.GetOk when reading "released_attributes"
which returns false for empty lists and prevents clearing attributes; update the
read logic to use d.GetOkExists (or alternatively use d.HasChange in update
handlers) so empty slices are propagated to sp.ReleasedAttributes; locate the
block referencing d.GetOk("released_attributes") and replace the presence check
with GetOkExists (and ensure the conversion to []string and assignment to
sp.ReleasedAttributes remains unchanged) and apply the same change to the
analogous block that appears later (the other occurrence around the 161-167
region).
In `@terraform-provider-jans/jans/shibboleth_configuration.go`:
- Around line 107-109: The trust endpoint is built by concatenating entityId
directly which can contain path-breaking characters; update the callers that
build the path (the get calls that use
"/jans-config-api/shibboleth/trust/"+entityId in shibboleth_configuration.go and
the similar occurrences around the noted regions) to URL-encode the entityId
path segment using url.PathEscape(entityId) before concatenation; ensure you
import net/url if needed and apply the same change to the other two occurrences
mentioned so all trust endpoint paths use PathEscape.
Comment on lines
+70
to
+72
| KeyPairGenerator keyGen = KeyPairGenerator.getInstance(DEFAULT_ALGORITHM); | ||
| keyGen.initialize(keySize, new SecureRandom()); | ||
| KeyPair keyPair = keyGen.generateKeyPair(); |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Refactor to reuse generateKeyPair method.
The key pair generation logic here duplicates the generateKeyPair method at lines 122-126. Consider reusing that method to maintain DRY principle.
♻️ Proposed refactor
LOG.info("Generating RSA key pair with {} bits", keySize);
- KeyPairGenerator keyGen = KeyPairGenerator.getInstance(DEFAULT_ALGORITHM);
- keyGen.initialize(keySize, new SecureRandom());
- KeyPair keyPair = keyGen.generateKeyPair();
+ KeyPair keyPair = generateKeyPair(keySize);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| KeyPairGenerator keyGen = KeyPairGenerator.getInstance(DEFAULT_ALGORITHM); | |
| keyGen.initialize(keySize, new SecureRandom()); | |
| KeyPair keyPair = keyGen.generateKeyPair(); | |
| KeyPair keyPair = generateKeyPair(keySize); |
🤖 Prompt for AI Agents
In
`@jans-shibboleth-idp/keygenerator/src/main/java/io/jans/idp/keygen/KeyGenerator.java`
around lines 70 - 72, The inline key generation at the top of KeyGenerator
duplicates existing logic in the class; replace the manual KeyPairGenerator
creation with a call to the class's generateKeyPair(...) helper (the
generateKeyPair method in KeyGenerator) to follow DRY. Call the appropriate
generateKeyPair overload with the same keySize (and algorithm if applicable),
ensure the same SecureRandom usage and exception handling behavior is retained,
and remove the duplicated KeyPairGenerator/initialize/generateKeyPair block.
| private static X509Certificate generateSelfSignedCertificate(KeyPair keyPair, String subjectDn) | ||
| throws Exception { | ||
|
|
||
| X500Principal subject = new X500Principal(subjectDn); |
Contributor
There was a problem hiding this comment.
Remove unused variable subject.
The X500Principal subject variable is created but never used. The method directly creates X500Name objects from the subjectDn string at lines 99-100.
🧹 Proposed fix
private static X509Certificate generateSelfSignedCertificate(KeyPair keyPair, String subjectDn)
throws Exception {
- X500Principal subject = new X500Principal(subjectDn);
-
Date notBefore = new Date();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| X500Principal subject = new X500Principal(subjectDn); | |
| private static X509Certificate generateSelfSignedCertificate(KeyPair keyPair, String subjectDn) | |
| throws Exception { | |
| Date notBefore = new Date(); |
🤖 Prompt for AI Agents
In
`@jans-shibboleth-idp/keygenerator/src/main/java/io/jans/idp/keygen/KeyGenerator.java`
at line 92, Remove the unused local variable X500Principal subject that is
created from subjectDn in KeyGenerator.java; delete the declaration
"X500Principal subject = new X500Principal(subjectDn)" and rely on the existing
X500Name constructions that use subjectDn (the X500Name creation code around
where subjectDn is used) so there are no unused imports or variables left.
| @@ -0,0 +1,133 @@ | |||
| package io.jans.idp.authn.context; | |||
|
|
|||
| import javax.annotation.Nonnull; | |||
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused import: @Nonnull is imported but never used.
Remove the unused import to keep the code clean.
🧹 Proposed fix
package io.jans.idp.authn.context;
-import javax.annotation.Nonnull;
import javax.annotation.Nullable;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import javax.annotation.Nonnull; | |
| package io.jans.idp.authn.context; | |
| import javax.annotation.Nullable; |
🤖 Prompt for AI Agents
In
`@jans-shibboleth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/context/JansAuthenticationContext.java`
at line 3, Remove the unused import of javax.annotation.Nonnull from the
JansAuthenticationContext class; locate the import statement "import
javax.annotation.Nonnull;" at the top of the JansAuthenticationContext.java file
and delete it so there are no unused imports in the class.
| private String state; | ||
| private String nonce; | ||
| private String acrValues; | ||
| private String relayingPartyId; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all occurrences of the misspelled 'relayingPartyId'
rg -n "relayingPartyId" --type javaRepository: JanssenProject/jans
Length of output: 865
🏁 Script executed:
#!/bin/bash
# Also search for the correct spelling to see what exists
rg -n "relyingPartyId" --type javaRepository: JanssenProject/jans
Length of output: 792
🏁 Script executed:
#!/bin/bash
# Read the target file to verify current state
cat -n "jans-shibboleth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/context/JansAuthenticationContext.java" | head -100Repository: JanssenProject/jans
Length of output: 3074
🏁 Script executed:
#!/bin/bash
# Search for ProcessJansCallbackAction and its usages of the typo
rg -n "ProcessJansCallbackAction" --type java -A 20 -B 5Repository: JanssenProject/jans
Length of output: 4732
🏁 Script executed:
#!/bin/bash
# Search for all usages of setRelayingPartyId and getRelayingPartyId methods
rg -n "setRelayingPartyId|getRelayingPartyId" --type javaRepository: JanssenProject/jans
Length of output: 776
🏁 Script executed:
#!/bin/bash
# Read lines 80-130 of JansAuthenticationContext to verify the complete picture
sed -n '80,130p' "jans-shibboleth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/context/JansAuthenticationContext.java"Repository: JanssenProject/jans
Length of output: 1442
Fix typo: relayingPartyId should be relyingPartyId
In SAML terminology, the correct term is "relying party" (a service provider that relies on the identity provider), not "relaying party." This typo is used in the field, getter/setter methods, and toString() method, and is already propagated to consuming classes: InitializeJansAuthenticationAction (line 48) and JansAuthenticationService (line 68).
✏️ Proposed fix
- private String relayingPartyId;
+ private String relyingPartyId; `@Nullable`
- public String getRelayingPartyId() {
- return relayingPartyId;
+ public String getRelyingPartyId() {
+ return relyingPartyId;
}
- public void setRelayingPartyId(`@Nullable` String relayingPartyId) {
- this.relayingPartyId = relayingPartyId;
+ public void setRelyingPartyId(`@Nullable` String relyingPartyId) {
+ this.relyingPartyId = relyingPartyId;
} `@Override`
public String toString() {
return "JansAuthenticationContext{" +
"userPrincipal='" + userPrincipal + '\'' +
", authenticated=" + authenticated +
- ", relayingPartyId='" + relayingPartyId + '\'' +
+ ", relyingPartyId='" + relyingPartyId + '\'' +
", acrValues='" + acrValues + '\'' +
'}';
}Update method calls in:
InitializeJansAuthenticationAction.java:48— changesetRelayingPartyId()tosetRelyingPartyId()JansAuthenticationService.java:68— changegetRelayingPartyId()togetRelyingPartyId()
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private String relayingPartyId; | |
| private String relyingPartyId; |
🤖 Prompt for AI Agents
In
`@jans-shibboleth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/context/JansAuthenticationContext.java`
at line 16, Rename the misspelled field relayingPartyId to relyingPartyId and
update its accessor methods and usages: change the field name in
JansAuthenticationContext, rename getRelayingPartyId() to getRelyingPartyId()
and setRelayingPartyId(...) to setRelyingPartyId(...), and update the toString()
implementation to reference relyingPartyId; then update all callers (e.g.,
InitializeJansAuthenticationAction.setRelayingPartyId(...) ->
setRelyingPartyId(...) and JansAuthenticationService.getRelayingPartyId() ->
getRelyingPartyId()) so compilation succeeds and the SAML term is corrected
throughout. Ensure any serialization annotations or reflection-based access (if
present) are adjusted to the new name.
pyproject.toml
Outdated
| [project] | ||
| name = "repl-nix-workspace" | ||
| version = "0.1.0" | ||
| description = "Add your description here" |
Contributor
There was a problem hiding this comment.
Replace placeholder project description.
description = "Add your description here" should be a real project description before release to avoid publishing placeholder metadata.
🤖 Prompt for AI Agents
In `@pyproject.toml` at line 4, Replace the placeholder value for the toml key
description in pyproject.toml (the line currently reading description = "Add
your description here") with a concise, accurate project description that
summarizes the library's purpose and main features; update the description
string to reflect the actual project intent and audience before release so
published package metadata is meaningful.
...leth-idp/shib-jans-authn/src/main/java/io/jans/idp/authn/impl/JansAuthenticationService.java
Show resolved
Hide resolved
jans-shibboleth-idp/shib-jans-authn/src/main/resources/flows/authn/jans-authn-beans.xml
Show resolved
Hide resolved
moabu
changed the title
mo-auto
added
the
kind-enhancement
devrimyatar
previously approved these changes
Feb 2, 2026
Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
|
|
uprightech
requested changes
Feb 2, 2026
Contributor
There was a problem hiding this comment.
jans-config-api/plugins/kc-saml-pluginshould be renamed instead of deleted. Maybe toshib-plugin? A priori because it IDP agnostic except for it's name and a few KC specific configuration details.jans-config-api/docs/kc-saml-plugin-swagger.yamlshould be renamed tojans-config-api/docs/shib-plugin.yamlfor the same reason.jans-keycloak-integration/job-schedulershould be moved to the project directory for shib related components and probably maintain it's name. We'll then remove references to KC within the project and replace them with references to Shibboleth.
Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
uprightech
previously approved these changes
Feb 2, 2026
iromli
requested changes
Feb 2, 2026
Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
iromli
requested changes
Feb 2, 2026
Signed-off-by: moabu <47318409+moabu@users.noreply.github.com>
iromli
approved these changes
Feb 2, 2026
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
5 participants
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.
Prepare
Description
Target issue
closes #13122
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
Release Notes
New Features
Deprecated
Documentation