docs(shibboleth): document the SWITCH edu-ID login flow - #1159
Conversation
WalkthroughThe README adds documentation for the SWITCH edu-ID SAML login flow, including AuthnRequest and RelayState handling, response validation, encryption, certificates, attribute mapping, account linking, and redirects. It defines federation metadata, Resource Registry, and SONAR configuration. It adds testing and troubleshooting instructions, SAML capture handling guidance, and an evaluation of OIDC as a possible alternative. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@sonar/modules/shibboleth_authenticator/README.md`:
- Around line 154-157: Add the text language identifier to the three fenced code
blocks in the README, including the blocks around the visible attribute table
and the referenced sections, so each opening fence satisfies markdownlint MD040.
- Around line 109-114: Correct the serialized RelayState example in the README
so its "next" value matches the documented login request’s next_param of "/".
Alternatively, update the request example to include the corresponding next
query parameter, keeping both examples consistent.
- Around line 358-360: Remove the "$ " shell prompt markers from the OpenSSL
command examples in the README, including the corresponding example referenced
near the second location, while preserving the commands themselves.
- Around line 452-456: The README’s intended-audience test uses linked
affiliations with a private-identity-only configuration, so it cannot verify
issuer refusal. Update the documented test procedure to select and validate the
issuer explicitly, or register an affiliation identity for the negative case;
for the existing configuration, describe verification of linked-affiliation
attribute release instead.
- Around line 306-317: Update the “Which certificate does what” table to state
that the browser validates both the SONAR and identity-provider TLS
certificates, while only private keys remain server-side. Clarify that the
service-provider public certificate is registered with SWITCH and the IdP
signing certificate is published in federation metadata. Qualify the “never used
to sign” statement as applying only to the current default configuration, since
signed requests may be enabled.
- Around line 117-120: Update the callback flow described around sid and
RelayState so responses with missing or invalid RelayState are rejected before
reaching authorized_signup_handler, ensuring every accepted response remains
bound to the initiating session; alternatively, explicitly document that replay
protection is conditional.
- Around line 263-278: Update the linked-affiliation section of the
documentation to reflect the provider mappings: state that SONAR requests
swissEduIDLinkedAffiliation but does not consume it because get_account_info
keeps only mapped attributes. Do not describe membership checks based on this
attribute unless you also add its mapping in sonar/config.py and consumption in
utils.py::get_account_info.
- Around line 227-232: Update the README explanation to distinguish the identity
provider entity_id in SHIBBOLETH_IDENTITY_PROVIDERS["eduidtest"]["entity_id"]
from the service provider entity_id in SHIBBOLETH_SERVICE_PROVIDER["entity_id"],
noting that the former is used to validate response and assertion Issuer values
while the latter identifies SONAR and the AuthnRequest issuer.
- Around line 512-558: Update the OIDC decision record around “Where things
stood in August 2026” and “What the integration would look like” to reflect
current SWITCH documentation: state that OIDC supports both affiliation and
personal identity models and is recommended for new services, replace the
deprecated User.Read scope with https://eduid.ch/scope/userinfo.read, and
describe confidential-client authentication as using either private_key_jwt with
a registered public key or a client secret. If retaining historical framing,
label it with a fixed date rather than current-state wording.
- Around line 92-100: Update init_saml_auth to enable authnRequestsSigned
through the constructed python3-saml settings or SHIBBOLETH_SERVICE_PROVIDER
mapping, and document the required IdP/Resource Registry registration of the
signing certificate, including explicit agreement when reusing the encryption
certificate for signing.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db6c0d96-bd86-4e06-ab3a-e041e4e2d3f8
⛔ Files ignored due to path filters (1)
sonar/modules/shibboleth_authenticator/eduid-consent.pngis excluded by!**/*.png
📒 Files selected for processing (1)
sonar/modules/shibboleth_authenticator/README.md
The module had no documentation, and the SAML exchange it implements is hard to reconstruct from the code alone: four interaction steps share a single URL, the assertion arrives encrypted, and four different certificates take part without the code ever naming them together. The flow is described step by step from a real capture of the test federation, with the HTTP requests, the SAML messages and, for each step, which certificate is doing the work. A summary table maps the four certificates to who holds their private key and where each is configured, and a section records which SWITCH federation aggregate the identity provider certificates are extracted from. Also documented: the Resource Registry configuration this flow depends on, including why the private identity and the linked affiliation attribute have to be required together; how to test, starting with giving the test account an organisational identity, which is the step whose omission invalidates every other; and a decision record on the OpenID Connect alternative, kept as a reference rather than a plan. The consent screenshot is redacted: the name and e-mail are masked, the home organisation and the linked affiliations are kept since they are what the flow exists to obtain. Personal data in the attribute listing is fictitious. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Documents the SAML exchange between SONAR and SWITCH edu-ID, step by step from a real capture of the test federation.
The module had no documentation, and the flow is hard to reconstruct from the code: four interaction steps share a single URL, the assertion arrives encrypted, and four certificates take part without the code ever naming them together.
Contents
+idpURLsReview notes
.certs/dev.crt, which maj fix certificates #1157 introduces. Merging that one first keeps the table accurate, but nothing breaks either way.🤖 Generated with Claude Code