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

Possible to add custom relay state in SAML login requests? #849

Closed
reuzel opened this issue Feb 21, 2023 · 2 comments
Closed

Possible to add custom relay state in SAML login requests? #849

reuzel opened this issue Feb 21, 2023 · 2 comments

Comments

@reuzel
Copy link

reuzel commented Feb 21, 2023

const RelayState =
(req.query && req.query.RelayState) || (req.body && req.body.RelayState);
pulls the relay-state from the request, while it is actually formulating a request towards the SAML IdP.

Two questions:

  • Would it be not more opportune to either omit, or otherwise create it from options provided to the authenticate call? This way applications can associate some of their own state with the login request. The passport standard AuthenticateOptions construct has a default state option that might be used for this.
  • Would it be possible to expose the relaystate to the verify callback? I can pull it from the request, but I suppose it is cleaner if it is exposed in the API. This way the application can 'recover' the state associated with the login request after the callback.

Regards,
Joost

@srd90
Copy link

srd90 commented Feb 21, 2023

Would it be not more opportune to either omit, or otherwise create it from options provided to the authenticate call?

Side note:
Based on this comment #157 (comment)
in case of redirect binding (which is default for authnrequestbinding) is used one can already pass RelayState via options object for authenticate function.

See also these codes:

// Defaults to HTTP-Redirect
this.redirect(await this._saml.getAuthorizeUrlAsync(RelayState, host, options));

and
https://github.com/node-saml/node-saml/blob/7bf1593d8cb6d4e7b0adf0d709a35ee4725c942c/src/saml.ts#L502-L516
and
https://github.com/node-saml/node-saml/blob/7bf1593d8cb6d4e7b0adf0d709a35ee4725c942c/src/saml.ts#L498

@reuzel
Copy link
Author

reuzel commented Feb 24, 2023

Thank for pointing that out, overriding the RelayState via the options will work for me.

However, I still wonder why the RelayState is fetched from the request in this particular case. You may consider removing it, unless it is part of some particular flow I don't understand at this moment...

@reuzel reuzel closed this as completed Feb 24, 2023
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

No branches or pull requests

2 participants