For an IdP initiated flow the source application may not know if there is a valid drupal session for a given user. In AccessDeniedSubscriber there is already an event capturing the scenario where an already authenticated user tries to access the login and acs route if the requesting URL has a relay state we can respect the redirect and push the user to the requested page.

Patch to follow.

CommentFileSizeAuthor
#2 allow-relay-state-3129800-1.patch3.58 KBndrake86

Comments

ndrake86 created an issue. See original summary.

ndrake86’s picture

ndrake86 commented
StatusFileSize
new allow-relay-state-3129800-1.patch3.58 KB

Patch. Adds path validator check to the relaystate URL

  • roderik committed 83748f8 on 8.x-3.x authored by ndrake86 
    Issue #3129800 by ndrake86: Process relay state in...
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Assigned: Unassigned » roderik

Thanks. I see no downsides to this; committed. I'll keep this issue open, though, to remind myself to think of whether

  • we should by default redirect to whatever is configured in settings, rather than always to the profile page. (That's likely a yes.)
  • we should display a message if the user is logged in already ( / What Does Core Do when e.g. following a one-time login link and the user is already logged in

  • roderik committed ccfc26e on 8.x-3.x 
    #3129800 followup: move 'redirection from ACS route when logged in' from...
roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Assigned: roderik » Unassigned
Related issues: +#2901757: /saml/login should not yield "access denied", +#3155968: Validate existing session before redirecting to RelayState

My self-assigned to-dos are one. FYI:

"We should by default redirect to whatever is configured in settings" is solved by moving the /saml/acs route completely out of the event subscriber, and just handling it in the controller route. This already gives us the 'process RelayState' for free, and I never should have put it in the subscriber (in #2901757: /saml/login should not yield "access denied") so you wouldn't have had to override it there. But I didn't see that when reviewing this issue.

"We should display a message": done now. The message itself is temporary (because at the moment it doesn't know if you are the same or a different user logging in) and will be changed as part of #3155968: Validate existing session before redirecting to RelayState .

(And "what does core do" made for an interesting diversion, because one-time login links don't fully work in D8 - leading to opening #3188375: Fix direct-login links (referer header leakage / UX) when already logged in.)

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
commented
Status: Active » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.