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

[AdminBundle] OAuthUserCreator Should query on username and email #1154

Merged
merged 1 commit into from
May 26, 2016
Merged

[AdminBundle] OAuthUserCreator Should query on username and email #1154

merged 1 commit into from
May 26, 2016

Conversation

Numkil
Copy link
Contributor

@Numkil Numkil commented May 10, 2016

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets #1144

When someone signs in for the first time using google sign in, his user should be searched based on
email or username.

@Numkil
Copy link
Contributor Author

Numkil commented May 10, 2016

I just realised there could be still a problem with this code in the following use case.
There are 2 users in the database. One with username == [the email we search for] and one with email == [the email we search for]. That way it would be impossible to decide which user should be logged in...

@jockri
Copy link
Contributor

jockri commented May 11, 2016

Perhaps we should have logic that it first searches for matching email. If not found, also search username. What do you think?

@Numkil
Copy link
Contributor Author

Numkil commented May 12, 2016

I gave find by email a higher priority as find by username. That way if there are 2 accounts present with that email address the one with the email address in the email field will be chosen.

@mlebkowski
Copy link
Contributor

How about giving the developer a choice? I.e. extract some kind UserFinderInterface, create a default service implementing it with the above logic, but with the possibility for the developer to replace it with his own implementation (replacing a service in the container), hmm?

@jockri jockri changed the title [AdminBundle] OAuthUserCreator Should query on username and email [WIP] [AdminBundle] OAuthUserCreator Should query on username and email May 19, 2016
@Numkil
Copy link
Contributor Author

Numkil commented May 20, 2016

@mlebkowski following your advice I pulled everything apart and set everything behind interfaces. This should allow other people to override either only the code for finding a user based on the googleId and email address or the complete login procedure.

@jockri I think this should be quite ready now

* @param string email
* @param string googleId
*
* @return AbstractUser Implementation
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@return mixed AbstractUser Implementation — this way IDE won’t expect to return AbstractUser instance. Maybe it should be a UserInterface instance from FOS?

When someone signs in for the first time using google sign in, his user should be searched based on
email or username.

2 seperate steps find email address in database
[AdminBundle] Find by email has higher priority as find by username

Abstract all parts oauth login
[AdminBundle] Put each part of the login functionality behind an interface and pull each step apart,
find user, create user, login user.

Expect EntityManagerInterface and mixed AbstractUser in comments
@Numkil Numkil changed the title [WIP] [AdminBundle] OAuthUserCreator Should query on username and email [AdminBundle] OAuthUserCreator Should query on username and email May 24, 2016
@jockri jockri merged commit 5c7958e into Kunstmaan:master May 26, 2016
@jockri jockri added this to the 3.5.2 milestone May 26, 2016
jockri pushed a commit that referenced this pull request Jun 1, 2016
* master:
  Video thumbnail fix (#1179)
  [AdminBundle] fix regex to check if admin preview (#1182)
  Fix adminlist SimpleItemAction template
  [AdminListBundle] Updated list template to use an icon for the View link
  Fix translation (#1177)
  [AdminBundle] OAuthUserCreator Should query on username and email (#1154)
  [All bundle] Translation fixes (#1172)
  Added `update ACL command` to update specific role with given permission(s) for all nodes
  [ArticleBundle] Added ability to select which overview page to add an article page to (#1160)
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.

None yet

3 participants