-
Notifications
You must be signed in to change notification settings - Fork 49
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
api/oauth: also activate user after successful oauth authentication #4779
api/oauth: also activate user after successful oauth authentication #4779
Conversation
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 think we miss to cover the case, if the user has created a email/password login and then authenticates with oAuth. Shouldn't that activate the user as well?
That's exactly what this PR should do. I updated the PR description to make it hopefully clearer. |
I think the question comes from here: https://github.com/BacLuc/ecamp3/blob/80ae096544e8156607a4588cd36d275c7d7c492a/api/src/Security/OAuth/GoogleAuthenticator.php#L72-L80 Is this |
oh, it was a little late yesterday. |
80ae096
to
809b447
Compare
I fixed it |
@@ -67,6 +67,9 @@ public function authenticate(Request $request): Passport { | |||
$profile->surname = $googleUser->getLastName(); | |||
$user = new User(); | |||
$user->profile = $profile; | |||
} | |||
|
|||
if (in_array($user->state, [null, User::STATE_NONREGISTERED, User::STATE_REGISTERED])) { |
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.
Looks very clean now. But you have an early return above, so if $existingProfile
is true, you will never get here.
I think the best is if you extract the logic above into a separate function (e.g. findOrCreateUser) and then you could keep the cleanliness of this last state-check.
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.
An existing Profile has to have run through the If below in a previous request, else it cannot be an existing Profile.
As long as we don't reset the state, this should be good enough
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.
Ah, get it. Thanks for clarifying.
Issue: #4670
Previously this only happened if the user was created after the oauth authentication.
Now this also happens if an existing not activated user authenticates with oauth, because we trust the email provided via oauth.