social-login-redirect: Redirection added for social auth users #306
Conversation
| $ids = \Drupal::entityQuery('user') | ||
| ->condition('name', $name) | ||
| ->range(0, 1) | ||
| ->accessCheck() |
There was a problem hiding this comment.
| ->accessCheck() | |
| ->accessCheck(FALSE) |
I think access check should be disabled in this case as well. I know anonymous users can read user info but this will be useful in case that ever changes.
| if ($ids) { | ||
| // Check if there is a social auth profile. | ||
| $socialIds = \Drupal::entityQuery("social_auth") | ||
| ->accessCheck(FALSE) | ||
| ->condition("user_id", reset($ids)) | ||
| ->execute(); | ||
| } | ||
|
|
||
| // If the user has no social auth profile, allow the login to continue. | ||
| if (empty($socialIds)) { | ||
| return; | ||
| } | ||
|
|
||
| // If the user has social auth profile, redirect them to /user/login/google. | ||
| $response = new RedirectResponse('/user/login/google'); | ||
| $response->send(); | ||
| user_logout(); | ||
| } |
There was a problem hiding this comment.
Consider the flow:
If no user with that email exists, $ids will be empty. In that case, the first if block never gets executed and $socialIds is never set. This means that the second block won't be entered. Finally, in this case, the user will get redirected to Google login. This might be okay in registration case but it is not very likely that someone will come this way to register. It will be clearer to the user if they are actually sent back. In our case, we just have to return from validation because the login system in Drupal will throw the error.
The root of this problem here is that the second if block is outside of the first if block and there is a chance it never gets set. You won't see errors because you are checking for empty but the problem still remains (as I have described above).
To fix this, you can make it very simple by considering this pseudo-code.
- If the user doesn't exists (
$idsis empty), then immediately return. - Now we know that the user definitely exists. Check if there is a social auth profile. If not, the immediately exit.
- Now we know that the user exists AND has a social auth profile. Redirect to Google auth.
There was a problem hiding this comment.
Hi, I have updated the code. Please verify
…cial auth profile
| if (empty($ids)) { | ||
| return; | ||
| } | ||
| else { |
There was a problem hiding this comment.
You don't need an else block here. If the condition above is true, it will return. Otherwise, it will execute this block anyway. This is one of the ways we can avoid else blocks. I recommend you read https://medium.com/swlh/return-early-pattern-3d18a41bba8
| if (empty($socialIds)) { | ||
| return; | ||
| } | ||
| else { |
There was a problem hiding this comment.
Same as above. This else is not necessary.
| // If the user has social auth profile, redirect them to /user/login/google. | ||
| $response = new RedirectResponse('/user/login/google'); | ||
| $response->send(); | ||
| user_logout(); |
There was a problem hiding this comment.
Do we need to do this? If the form validation fails, the user will not log in, so there is nothing to logout. Speaking of which, you should fail the validation here even if we are sending a redirect response.
No description provided.