Skip to content
Snippets Groups Projects

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?

What are the relevant issue numbers?

Edited by username-removed-384663

Merge request reports

Pipeline #12107835 failed

Pipeline failed on hvlad:feature/oauth_generic_provider

Test coverage 50.70% from 1 job
The source branch feature/oauth_generic_provider does not exist. Please restore it or use a different source branch.
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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?
  • Is it possible that user is not nil but user.ldap_user? is true?

  • 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.

  • @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.

  • Please register or sign in to reply
  • username-removed-128633
  • added ~889916 label

  • @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)

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading