Make oauth provider login generic
What does this MR do?
Extended the oauth login implementation to allow different login implementation specific for each provider (using polymorphism).
Are there points in the code the reviewer needs to double check?
Why was this MR needed?
The current implementation is relying only on the password stored in the Gitlab DB, which in some situations is not recommended/possible/allowed as it requires to store the password of an user outside of the identity provider system.
Some examples where this approach could be used are:
- LDAP login: the login function for the LDAP provider can do a BIND instead of synchronizing the user password in Gitlab DB
- Token based login: the user could provide a token as password, token that is generated by an third party identity provider (i.e. OIDC)
At the same time this approach will ease the introduction of new providers in the future.
Does this MR meet the acceptance criteria?
-
Changelog entry added -
Documentation created/updated -
API support added -
Tests -
Added for this feature/bug -
All builds are passing
-
-
Conform by the merge request performance guides -
Conform by the style guides -
Branch has no merge conflicts with master
(if it does - rebase it please) -
Squashed related commits together
What are the relevant issue numbers?
Merge request reports
Activity
added 20 commits
-
6080c39f...73c54366 - 19 commits from branch
gitlab-org:master
- a0609d16 - Make oauth provider login generic
-
6080c39f...73c54366 - 19 commits from branch
@hvlad Thanks for the contribution! Can you create a corresponding issues with a feature proposal so we can first discuss if we want to use this approach and then make the change (in this MR)?
@adamniedzielski In fact I have another MR !5773 (closed) that is pending for more than 5 months which is working fine and relating to an Issue. Please have a look at the other MR and after you give some feedback I can close this one without merge.
@hvlad Thank you for the answer, now I understand the context better. I'm sorry that neither your proposal nor the previous merge request received any attention. It's not my area of expertise either, but I'll try to get the attention of other team members who may better understand the proposal.
added ~480950 oauth labels
27 27 def find_with_user_password(login, password) 28 28 user = User.by_login(login) 29 29 30 # If no user is found, or it's an LDAP server, try LDAP. 30 # If no user is found, try LDAP. 31 31 # LDAP users are only authenticated via LDAP 32 if user.nil? || user.ldap_user? 33 # Second chance - try LDAP authentication 34 return nil unless Gitlab::LDAP::Config.enabled? 35 32 if user.nil? The login is not as simple as could be in the current implementation to just avoid such checks. The user login works as follow:
- user registered with Gitlab and got an internal user that has no identity entry
- do a password check agains DB
- user is registered with an external provider but never logged in over UI => no user yet in DB
- if provider is LDAP then user is allowed to login by doing a successful LDAP Bind
- if provider is not LDAP then user is not allowed
- user is registered with an external provider an has previously logged in over UI => user found in DB and has an identity entry for the provider
- if provider is LDAP then user is allowed to login by doing a successful LDAP Bind
- if provider is not LDAP then user is calling Provider login implementation
Some of the problems above could be easily by adding the LDAP and internal DB cases as Providers and creating an identity in DB for this cases too. This part I have left outside in this MR to make sure is done in smaller steps, but that still improve code
What do you think? Smaller steps with less risk ;)
Another question is why is the LDAP user handled differently than any other user? Why allow an LDAP user to use CLI without logging in over the UI in first place?
If this exception would be removed than it would lead to the requirement that any user has first to register or login using an external provider over the UI to ensure the user entry is populated in DB.
I do not think that it would be such a loss for UX to ask any user to login first over the UI, as this is currently only for LDAP the exception.
- user registered with Gitlab and got an internal user that has no identity entry
@rymai Can you provide feedback?
Some of the problems above could be easily by adding the LDAP and internal DB cases as Providers and creating an identity in DB for this cases too. This part I have left outside in this MR to make sure is done in smaller steps, but that still improve code
What do you think? Smaller steps with less risk ;)
@hvlad Yep, smaller steps is better :) Adding an identity to every current user in the system will take a lot of time, and (if we decide to do that) will need to be done carefully.
Another question is why is the LDAP user handled differently than any other user? Why allow an LDAP user to use CLI without logging in over the UI in first place?
Users within a company that uses GitLab don't necessarily know that they use LDAP/ActiveDirectory, they're just used to being able to use those credentials everywhere, so we help them out.
If this exception would be removed than it would lead to the requirement that any user has first to register or login using an external provider over the UI to ensure the user entry is populated in DB.
I do not think that it would be such a loss for UX to ask any user to login first over the UI, as this is currently only for LDAP the exception.
Since we currently have this exception, it was apparently at one point requested by a customer :) If we take it away, we revert a feature that they (and us, at the time at least) thought was useful.
- Resolved by username-removed-384663
- Resolved by username-removed-384663
- Resolved by Douwe Maan
@rymai Looks like this MR is a dupe of https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5773 which I just reviewed :)
@hvlad Please take into account feedback from both MRs! I will close the earlier one in favor of this one.
assigned to @DouweM
@hvlad I think we need some more tests here for the new classes and changed logic!
mentioned in merge request !5773 (closed)