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

Now 'signin redirect' option can be a function again #4482

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frapan
Copy link

@frapan frapan commented Oct 25, 2017

Description of changes

This pull request restores the Keystone 0.3 functionality to set the signin redirect option as a function. It didn't work in Keystone 4 any more.

This functionality is useful to customize the page you are redirect to after successful login, especially for not admin users.

The documentation has been updated either for the signin redirect option and for the signout redirect option.

Note: for the signout redirect option this functionality was already developed, but the documentation was not aligned.

Related issues (if any)

#4449
#4469

Testing

  • Please confirm npm run test-all ran successfully.

@piere129
Copy link

piere129 commented Mar 5, 2018

keystone.set('signin redirect', function(user, req, res){
var url = (user.isAdmin) ? '/keystone/post' : '/blog';
res.redirect(url);
});

are you sure it accepts functions? this code doesn't work for me

@frapan
Copy link
Author

frapan commented Mar 28, 2018

@piere129 I've seen the problem.
My implementation works only when the user is not admin.
Your code works if you login with a non-admin user but if you login with an admin user it doesn't because it redirects you to the default /keystone url and to /keystone/post.
I've always written code as the following (note "/keystone" instead of "/keystone/post"):

keystone.set('signin redirect', function(user, req, res){
var url = (user.isAdmin) ? '/keystone' : '/blog';
res.redirect(url);
});

and I've never noticed this problem.

To come to a solution with Keystone 4, we should change the behaviour of "signin redirect" option: it should accept only the user as a parameter (no "req" and "res") e should simply return the url (directly or, even better, by a callback).
That's because Keystone 4 uses an ajax call to sign in the user and, therefore, it would be useless to redirect to another page.
The solution should be to read the url returned by the "signin redirect" option in the admin/server/api/session/signin.js and return it to the client (together with "success" and "user" data) who, in turn, can do the redirect (in the admin/client/Signin/Signin.js file).
At the moment the client does this:

if (Keystone.redirect) {
    top.location.href = Keystone.redirect;
} else {
    top.location.href = this.props.from ? this.props.from : Keystone.adminPath;
}

but Keystone.redirect works only if you set the "signin redirect" as a string.
I suggest to deprecate the use of Keystone.redirect as it is set when you load the login page, before you submit the form, and therefore cannot deal with the user who logs in.
Then I suggest to replace it with a value returned by the ajax call to signin.

That would be compatible with the current use of "signin redirect" as a string and would allow to configure the redirect after signin in a very flexible way.
If you agree with this solution, I can work on it.
I hope I made myself clear, otherwise just ask me.

@stennie stennie added the bug label May 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants