-
Notifications
You must be signed in to change notification settings - Fork 692
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
Setting up a site with mandatory login and basic edge cases is insanely complicated #1146
Comments
I fully agree, which is why I created a sample myself as well a few years back (and kept it up to date). It's very similar to your choices OP, main difference being I tried to keep it out of the app.component and isolate it in a service. The initial login sequence is annotated in my sample but the gist of it is: public runInitialLoginSequence(): Promise<void> {
return this.oauthService.loadDiscoveryDocument()
.then(() => this.oauthService.tryLogin())
.then(() => {
if (this.oauthService.hasValidAccessToken()) {
return Promise.resolve();
}
return this.oauthService.silentRefresh()
.then(() => Promise.resolve())
.catch(result => {
const errorResponsesRequiringUserInteraction = ['interaction_required', 'login_required', 'account_selection_required', 'consent_required'];
if (result
&& result.reason
&& errorResponsesRequiringUserInteraction.indexOf(result.reason.error) >= 0) {
return Promise.resolve(); // User interaction is needed to log in, we will wait for the user to manually log in
}
return Promise.reject(result);
});
})
.then(() => {
this.isDoneLoadingSubject$.next(true);
if (this.oauthService.state && this.oauthService.state !== 'undefined' && this.oauthService.state !== 'null') {
let stateUrl = this.oauthService.state;
if (stateUrl.startsWith('/') === false) {
stateUrl = decodeURIComponent(stateUrl);
}
this.router.navigateByUrl(stateUrl);
}
})
.catch(() => this.isDoneLoadingSubject$.next(true));
} I'm not sure if it can be part of the library, since both our login flows are quite opinionated and possibly even specific to your IDS and flow. So sharing sample usages might be the best we can do? The available samples are listed at the top of the readme, not sure if you had seen that? It might be useful to add a link to your post as well? Or would you have other ideas on how to improve this repository around this? |
yes, thanks I found your sample (even starred it to find it later, but couldn't :P) but found it way too complex to integrate into my app, and also I don't required route guards, but after all the things I ended up doing, I might havae as well used your And even with that, your I used to use a server side guard before doing it client side, and it was much easier to handle... IMO we need to warn users about those timings issues that are not really documented and can drive you crazy:
|
All good and fair points! Just a small extra observation regarding:
Very true! Since I originally created my sample there are now in fact async APP_INITIALIZERs (not there before, I think), and I think I would do the login sequence there. In fact, I do so in some of my production applications. This only solves some of the issues you mention, but I do think it solves many of the important edge cases. FWIW. |
Hey is this code:
in your auth redirect component because you find that if you just have the part in the else block, then your app will (for some reason) send you to the place you want to redirect to, but then instantly go BACK to this redirect component, and then you have to tell it again to redirect? Because that's what happened to me and so I have like the exact thing in my own code. |
this Your case that you instantly go back to your redirectComponent is probably because of a route guard in your code, my app does not have any logic to redirect to authRedirect, it's only activated via the redirect_uri passed to AD provider. If it's goes back to the AD provider after that, it's probably because your app does not check if the authentication is still in progress, that's why I used the |
Yea I presume it's the last paragraph happening. An enum in the library saying the current state of the auth process would be very helpful. |
I think you need to use the sessionStorage at the some point because of the redirect, you lose the JS context. |
not sure if this is still helpful, but i've done something like this:
then, the earlier we call this, the better, so as mentioned above, doing this in
Thanks to converting to an observable, that solution can be easily extended by e.g. calling the api to fetch some core data of an app (by e.g. chaining a |
Describe the bug
Avoid entering a redirect loop, avoid requests triggered before valid token is available (those will trigger errors client side), handling the refresh_token flow, redirecting user to initial url after login, new tabs share the same credentials than first tab...
All of those seems to be basic features you would like built-in in oauth2 framework like this. However, here how my implementation turned out and I am really disappointed by its lack of simplificity nor existing example that cover all of those.
How my implementation looks like
Read the comments to understand why I did things like this...
oauth.module.ts with useful configs
app.component.ts with most of the logic to know when to redirect to login
loginRedirect.component.ts
Additional context
Hopefully it helps someone who have the same needs, and you can provide feedback it you feel it could be improved somehow.
Maybe adding a sample that covers all of use cases could be useful to others.
Thanks.
The text was updated successfully, but these errors were encountered: