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

feat(auth): added support for redirecting to initial page after login #719 #797

Merged
merged 2 commits into from
Sep 3, 2021

Conversation

grosswilerp
Copy link
Contributor

This PR should implement the feature request from issue #719.

When checking if the user has a session, before redirecting to /ui/login it will check if the URL is different than /ui/login and stores it in sessionStorage.setItem("returnTo").

Once the user has logged in, it will check if there is a value in sessionStorage.setItem("returnTo") and sets this instead of the default /ui for the pathname.

@tchiotludo
Copy link
Owner

Just make a quick try and it don't seems to work.

Adding here : https://github.com/tchiotludo/akhq/blob/dev/client/src/components/Root/Root.jsx#L9

  componentWillUnmount() {
    const pathname = window.location.pathname;

    if (pathname !== '/ui/login') {
      sessionStorage.setItem ('returnTo', pathname+window.location.search);
    }

    this.cancelAxiosRequests();
  }

and removing from Routes.js seems to be a better place to handle it ?

What do you think about that ?

@trancee
Copy link

trancee commented Aug 21, 2021

I am not sure this is a better way. Since the redirection back to the initial page should only happen if the user was not logged in and needs to log in. That's why I had put it in Routes.js where the logic is to redirect to the login page.

If we add it to componentWillUnmount it will be called every time a component is unmounted. I am not sure that is the intended behavior?

I have tested my code before creating the PR, and it worked for me. Unless I did something wrong when testing, could you share how you tested it, so I can reproduce this on my end as well?

@tchiotludo
Copy link
Owner

The expected behaviour is to redirect to previous page on every login.
I think you cover the hard redirect when there is no acls with no-roles group.

The componentWillUnmount try to cover a case when the user have a login button on the page and some acls defined (like reader for default, and logged with admin). I expected for them to get redirected to previous page.

What to do you think about that ?

@grosswilerp
Copy link
Contributor Author

The expected behaviour is to redirect to previous page on every login.
I think you cover the hard redirect when there is no acls with no-roles group.

Yes, you are absolutely right there.

The componentWillUnmount try to cover a case when the user have a login button on the page and some acls defined (like reader for default, and logged with admin). I expected for them to get redirected to previous page.

What to do you think about that ?

That makes sense. I will adjust my PR accordingly. Thank you for the explanation.

@grosswilerp
Copy link
Contributor Author

@tchiotludo Did you check the PR after your suggestions? Is there anything else I need to do?

@tchiotludo tchiotludo merged commit b5d73df into tchiotludo:dev Sep 3, 2021
@tchiotludo
Copy link
Owner

Oups totally forgot this one ;)
All is good, thanks 👍

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

Successfully merging this pull request may close these issues.

3 participants