Skip to content

Support a caller-supplied redirect on /reset-remember-wayf - #2097

Open
kayjoosten wants to merge 1 commit into
wayf-reset-endpointfrom
wayf-reset-redirect-param
Open

kayjoosten wants to merge 1 commit into
wayf-reset-endpointfrom
wayf-reset-redirect-param

Conversation

@kayjoosten

Copy link
Copy Markdown
Contributor

What

Extends GET /reset-remember-wayf (introduced in #2085 / #2094) so it
accepts an optional redirect query parameter, and always appends a
wayfReset=removed|none query parameter to whichever redirect target is
used. This is a prerequisite for
OpenConext/OpenConext-profile#345,
which wants to link from Profile's "my personal data" page to this
endpoint and land the user back on that same page afterwards, knowing
whether a cookie was actually cleared.

Stacked on #2094

This branches from wayf-reset-endpoint (#2094, not yet merged), since it
directly extends ResetRememberedWayfController added there. The diff
shown here is the incremental change only; once #2094 merges to main,
this PR's base should be retargeted to main.

Details

  • New config wayf.reset_choice_allowed_redirect_hosts is a host
    allowlist (defaulting to profile.dev.openconext.local in dev/CI). The
    redirect parameter is only honoured when it parses to an http(s) URL
    whose host appears in this list; otherwise the endpoint silently falls
    back to the existing wayf.reset_choice_per_idp_redirect target. This
    mirrors the "parse and check an allowed value" shape of the existing
    AllowedSchemeValidator rather than introducing a new general-purpose
    URL validation abstraction for a single call site.
  • Resolving the redirect target and appending the wayfReset signal are
    extracted into small private methods, so __invoke reads as a straight
    sequence: clear/log the cookie if present, resolve where to send the
    user, tell them what happened.
  • The entry-count/log-message behaviour from Add endpoint to remove "remember my choice" cookie #2085 is unchanged: a
    present-but-unparseable cookie is still treated the same as a valid one
    for logging and for the wayfReset signal (both are cleared and
    reported), since from the caller's perspective the cookie no longer
    exists either way.
  • Existing unit/functional tests were updated for the new ?wayfReset=
    suffix on every redirect, and new cases cover an allowed redirect host,
    a redirect with its own query string, a disallowed host, a malformed
    URL, and a disallowed scheme.

Testing

All run inside the Docker dev container (PHP 8.5):

  • phpmd / phpcs / phpcs-legacy / docheader: clean.
  • eb4: 244/244. unit: 1008/1008 (1003 baseline + 5 new). functional
    (APP_ENV=test): 123/123 (121 baseline + 2 new). integration:
    105/105.
  • Behat default suite: 300/300 scenarios, 5557/5557 steps (unchanged —
    no new scenarios needed, this is pure PHPUnit-covered logic).
  • Twig lint: 114/114.

Refs: OpenConext/OpenConext-profile#345

Let /reset-remember-wayf accept an optional `redirect` query parameter
so callers such as Profile can send the user back to the page they
came from once their remembered per-SP WAYF choices are cleared,
instead of always sending everyone to the single operator-configured
URL from #2085. Every redirect (whether the caller-supplied target or
the configured fallback) now also carries a `wayfReset=removed|none`
query parameter, so the caller can tell whether a cookie actually
existed and was cleared.

- New config wayf.reset_choice_allowed_redirect_hosts is a host
  allowlist (defaulting to profile.dev.openconext.local in dev/CI):
  the `redirect` parameter is only honoured when it parses to an
  http(s) URL whose host appears in this list, otherwise the endpoint
  silently falls back to the existing wayf.reset_choice_per_idp_redirect
  target. This mirrors the "parse and check an allowed value" shape of
  the existing AllowedSchemeValidator rather than introducing a new
  general-purpose URL validation abstraction for a single call site.
- Resolving the redirect target and appending the wayfReset signal are
  both extracted into small private methods so the __invoke method
  reads as a straight sequence: clear/log the cookie if present,
  resolve where to send the user, tell them what happened.
- The entry-count/log-message behaviour from #2085 is unchanged: a
  present-but-unparseable cookie is still treated the same as a valid
  one for logging and for the wayfReset signal (both are cleared and
  reported), since from the caller's perspective the cookie no longer
  exists either way.
- Existing unit/functional tests were updated for the new
  `?wayfReset=` suffix on every redirect, and new cases cover an
  allowed redirect host, a redirect with its own query string, a
  disallowed host, a malformed URL, and a disallowed scheme.

Needed by OpenConext/OpenConext-profile#345, which links back to
Profile's "my personal data" page after resetting a user's remembered
WAYF choices.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant