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

Why is the relayState attribute a part of EntitySettings? #163

Open
osuvaldo90 opened this issue Feb 2, 2018 · 10 comments
Open

Why is the relayState attribute a part of EntitySettings? #163

osuvaldo90 opened this issue Feb 2, 2018 · 10 comments

Comments

@osuvaldo90
Copy link

Perhaps it is my misunderstanding of the SAML protocol but it seems as if it would make more sense to have relayState as a parameter to createLoginRequest().

@tngan
Copy link
Owner

tngan commented Feb 2, 2018

@osuvaldo90 I quickly review the code, in fact, right now you can pass the relayState in the entity constructor.

const sp = serviceProvider({
  relayState: ...
});

@tngan tngan added the question label Feb 2, 2018
@osuvaldo90
Copy link
Author

osuvaldo90 commented Feb 2, 2018

Thanks @tngan. My concern is mainly with using RelayState to communicate back to my service provider what the user wanted to do before being redirected to the identity provider. The fact that relayState has to be specified in the ServiceProvider constructor means I have to create a new instance of ServiceProvider for every request which involves parsing the XML metadata. While the XML is small it seems wasteful and potentially inefficient to do this for every request with a large enough number of requests per second.

Additionally, I began working on a solution to #157 and encountered the following TypeScript error when attempting to pass in relayState to the ServiceProvider constructor:

Object literal may only specify known properties, and 'relayState' does not exist in type 'ServiceProviderSettings'.
const sampleRelayState = 'SampleRelayState'

const spNoAssertSignCustomConfig = serviceProvider({
  ...defaultSpConfig,
  metadata: spmetaNoAssertSign,
  signatureConfig: {
    prefix: 'ds',
    location: { reference: '/samlp:Response/saml:Issuer', action: 'after' },
  },
  relayState: sampleRelayState
});

@osuvaldo90
Copy link
Author

osuvaldo90 commented Feb 2, 2018

The SAML 2.0 Wikipedia page seems to imply that RelayState is certainly a property of the login request and response flow rather than the ServiceProvider as a whole.

@tngan
Copy link
Owner

tngan commented Feb 14, 2018

@osuvaldo90

Sorry for the late reply. Thanks for catching this. Yes, it would be good to move it into request level instead of entity level.

@eliasson
Copy link

eliasson commented Aug 2, 2018

I monkey patch the entitySetting prior to the call (to avoid recreating the sp object), is there any implications on doing this as a workaround?

sp.entitySetting.relayState = "http://example.com";
sp.createLoginRequest(idp, 'redirect');

(omitted code for restoring relayState value)

Would a change to move this to a parameter for createLoginRequest instead be a good entry-level feature for a contributor?

Thanks!

@tngan
Copy link
Owner

tngan commented Aug 3, 2018

@eliasson Yes, this is a mistake when I have designed the interface. I will find a way to put it back to request level.

@tngan tngan added the rc Release candidate label Oct 7, 2018
@tngan tngan removed the rc Release candidate label Oct 20, 2018
@perbergland
Copy link

I was just looking for a way to pass a returnurl to the login request and the relay state seems like what people use. Is there any progress on this since the last comment? I assume it’s not an easy fix

@ghost
Copy link

ghost commented May 9, 2020

@tngan Is there any progress on this issue?

@ABitShift
Copy link

Do you need help implementing this ?

@tngan
Copy link
Owner

tngan commented Feb 15, 2023

@ABitShift Yes, feel free to submit a PR, thanks for recalling this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants