[x] Compare this PR's implementation with the OpenID Connect Spec. Add notes to this PR's conversation about how well its implementation matches the spec.
[x] Look at how oidc-client.js uses state and see how that meshes with what the requirement for #49.
[x] Look at the pros and cons of storing application state in the state parameter instead of in other storage locations such as local storage or a cookie.
[x] Bring the test coverage for this PR up to 100% on npm run test.
Opaque value used to maintain state between the request and the callback. Typically, Cross-Site Request Forgery (CSRF, XSRF) mitigation is done by cryptographically binding the value of this parameter with a browser cookie.
Is this the most appropriate location to store state though?
The concern right now with using args.data is that our OpenIdConnect.login((args) method accepts an args parameter that could include a data property. In that case, if we store the redirectUrl in the data property, we will overwrite the caller's value.
One way to solve that problem is to warn the caller with something like this:
public async login(args: any = {}): Promise<void> {
const instruction = this.router.currentInstruction;
const redirectUrl = instruction.queryParams.loginRedirectRoute || getInstructionUrl(instruction);
if (redirectUrl && args.data) {
const message
= 'The login method received an args object with an args.data value.'
+ 'That value will be overwritten by the loginRedirectRoute parameter in the address bar.';
this.logger.warn(message);
}
args.data = redirectUrl;
await this.userManager.signinRedirect(args);
}
Another way to handle that is to concatenate our loginRedirectRoute value to the data, and then strip that value when we read the user.state later.
After the authentication request completes, the data value is only available via the return value from UserManager.signinRedirectCallback. We do not expose that return value to the client application, so it is not necessary to consider how the client application might make use of a value it cannot access.
@gerbendekker This is a second attempt at resolving #49 after my first failed merge of https://github.com/aurelia-contrib/aurelia-open-id-connect/pull/51. A review by you would be welcome.
stateparameter instead of in other storage locations such as local storage or a cookie.npm run test.It does seem appropriate to maintain application state in the OpenID Connect authentication request's
stateparameter.The OpenID Connect specification says this of the
stateparameter:The oidc-client-js library now accepts either a
dataor astateparameter, and uses its value as part of the sign in request state value.Is this the most appropriate location to store state though?
The concern right now with using
args.datais that ourOpenIdConnect.login((args)method accepts anargsparameter that could include adataproperty. In that case, if we store theredirectUrlin thedataproperty, we will overwrite the caller's value.One way to solve that problem is to warn the caller with something like this:
Another way to handle that is to concatenate our
loginRedirectRoutevalue to thedata, and then strip that value when we read theuser.statelater.After the authentication request completes, the
datavalue is only available via the return value fromUserManager.signinRedirectCallback. We do not expose that return value to the client application, so it is not necessary to consider how the client application might make use of a value it cannot access.