-
Notifications
You must be signed in to change notification settings - Fork 404
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
Redirecting user to same page after login. #467
Conversation
Thanks @mspalti! It seems to work if you go to e.g. a collection homepage and log in from there. On master that would redirect you to the home page, in this branch it doesn't. However if I'm not logged in, and I enter |
Oh, I see. The log-in component is shared. When you follow the /submit route (or any other guarded route) you are redirected to the login page, and in that case, we do not want to reset the redirectUrl (to be '/login'), rather we should use the existing value. The naive solution (which does seem to work) is to not set the redirectUrl if the current url is '/login' If that's too brittle, perhaps we could reset the redirectUrl only when the current value is an empty string and not an actual route. |
OK. Given what I currently see, I think the best idea is probably to set the redirectUrl in the submit method only if the AuthState.redirectUrl value is empty. I've added this to the PR. (Of course, we could also hard-code the '/login' path and use that instead, or something else.) |
While working on a different PR (lacking any of the code changes here) I noticed that when logged in and editing an item, selecting the withdraw or make private option triggers a new login dialog because of the route guard. Also, after logging in, the page redirect fails to work; it works if I click the login button a second time. I verified the same behavior when running the code in this PR. Followup: I looked into this a bit more. When I click on "withdraw" the REST api sees an invalid token signature and returns null authentication, after which the angular client redirects to the login page. Looking in Chrome devtools, it seems that the authorization token is not sent in the withdraw request. |
Revised again for the following:
Much of this is based on the AuthService.LOGIN_ROUTE. I'm not checking to see if the current redirect url isEmpty(), as I was in the previous commit. |
this.isAuthenticated = this.store.pipe(select(isAuthenticated)); | ||
|
||
// for mobile login, set the redirect url to the previous route | ||
this.windowService.isXs().pipe(take(1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, we need to determine whether we're on an XS screen, because on XS screens the login component is rendered as a separate page, rather than a dropdown at the top of another page?
If that's the case I'd prefer if you added an input to this component:
@Input() isStandalonePage: boolean
or something to that effect, and use that to determine whether or not to set the redirect url.
That way, if we ever change it so that e.g. the page is also used on SM screens, we won't have to update this logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the idea. Although looking more closely I see that the separate login page is used for both xs and sm screens. I should have checked that sooner.
I've implemented the Input(), passing "true" from the login-page template and "false" from auth-nav-menu template. Checked the result and it seems to be working.
So I think this line in the AuthenticatedGuard class might be redundant now, but does no harm. |
@mspalti I'm sorry if I'm just answering now. |
Hi @atarix83 Thanks for the feedback. I see what you mean about the authentication guard. Based on further testing I don't think the snippet I mentioned is redundant! So if I understand things correctly, there seem to be at least 4 contexts. Setting the redirectUrl for:
The last two seem to require taking into account UI state. Specifically, for login requests initiated from the app menu the redirect url is the current page, for login requests from a standalone login page the redirect url is the previous page before the login. @artlowel suggested passing in the standalone page status as component input. I've experimented a bit more, and think that the next step may be to add a I'll update the PR with this suggestion. |
The latest idea (the I also thought about passing the |
@atarix83, I can see that your recommendation is the best idea. If the I'll update |
I made the change to Unfortunately the travis build keeps failing with a typescript import error. Initially I guessed this might have something to do with angular modules, but that seems unlikely after more investigation. On my machine, I can build the project ( |
Actually, the travis error appeared on Friday... the same import had worked fine on Thursday. |
… the user is returned to the same page after login).
… been set already.
… url. Also added check for mobile layout that forces setting of the redirect url.
Minor typedoc and import updates.
…nent is being used in the standalone login page or the drop-down.
…patches auth request.
…f no redirect url is found in the store. Removed unused import that was causing merge conflict. Once again try to fix merge conflict. Added routeService to server module providers. Changed order of providers. Minor change to ServerAuthService to make method signature consistent with AuthService. Try adding RouteService to browser-app module to see if that fixes travis build. One more try at getting the CI build to work. Removed change to browser module.
…base, test now passes.
OK. I just needed to rebase to pick up the new path to RouteService. Tests now passing. |
src/app/core/auth/auth.service.ts
Outdated
@@ -337,7 +339,7 @@ export class AuthService { | |||
/** | |||
* Redirect to the route navigated before the login | |||
*/ | |||
public redirectToPreviousUrl() { | |||
public redirectToPreviousUrl(isStandalonePage: boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a good idea to rename this method to something like redirectAfterLoginSuccess
-- especially since the redirect is actually not to previous url in all cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that's better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mspalti please consider that in the auth service SSR instance there the same redirectToPreviousUrl that needs to be fixed too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the method name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good, so I'll approve it now.
Feel free to still change the name of that method if you like.
@@ -1,9 +1,9 @@ | |||
import { filter, map, takeWhile } from 'rxjs/operators'; | |||
import { Component, OnDestroy, OnInit } from '@angular/core'; | |||
import {filter, map, takeWhile } from 'rxjs/operators'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this file some imports have spaces before/after curly braces but some imports don't have. in general spaces there should be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I updated typescript settings in intellij to add the spaces & fixed these
@atarix83 , you mentioned the SSR authService. Up to this point I have only changed the method signature, mostly because I didn't understand how redirection works in server rendered code. Now that I've looked more, I think I basically understand how the redirectUrl is obtained and used server side. But I don't think browser history is similarly available, which means that my fix is not possible. And I'm not sure that this matters. It seems like user-initiated logins (and redirects) will always take place when the app is running in the browser. Let me know if I missed something! |
I looked at transfer state and saw that, of course, the current url is added to history in state. Using that history in the SSR authService is helpful when a page is reloaded by an authenticated user. The server side knows to render the current page (and not home). So I am making this change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mspalti thanks for the changes
It looks good to me now
This is at +2, so merging. Thanks @mspalti ! |
Relates to #451 |
Simple update to log-in component. This seems to be all that we need. My first effort on the angular side so let me know if I failed to see something.