Skip to content
Snippets Groups Projects

Issue #993: Fixed login failure when extern_uid changes

Issue #993: Fixed login failure when extern_uid changes
joelkoglin/gitlab-ce:feature_fix_ldap_auth_issue_993 into master
Merged username-removed-194173 requested to merge joelkoglin/gitlab-ce:feature_fix_ldap_auth_issue_993 into master

This MR fixes issue gitlab-org/gitlab-ce#993

I changed the Gitlab::LDAP::User#update_user_attributes function to always update a single identity instead of creating unique ones when an exact match is not found. This allows an LDAP user whose identities extern_uid (distinguishedName) has changed to be able to login.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Douwe Maan
  • username-removed-194173 Title changed from Issue #993 (closed): Fixed login failure when extern_uid changes to [WIP] Issue #993 (closed): Fixed login failure when extern_uid changes

    Title changed from Issue #993 (closed): Fixed login failure when extern_uid changes to [WIP] Issue #993 (closed): Fixed login failure when extern_uid changes

  • username-removed-194173 Added 25 commits:

    Added 25 commits:

    • 286fdb0a...6ae806b1 - 24 commits from branch gitlab-org:master
    • 729a965b - Issue #993 (closed): Fixed login failure when extern_uid changes
  • username-removed-194173 Added 1 commit:

    Added 1 commit:

    • 0ab285fe - Issue #993 (closed): Fixed login failure when extern_uid changes
  • username-removed-194173 Added 1 commit:

    Added 1 commit:

    • 4102204c - Issue #993 (closed): Fixed login failure when extern_uid changes
  • username-removed-194173
    username-removed-194173 @joelkoglin ·
    Author Contributor

    There seems to be a bug when running the tests using the mysql2 gem where the identity has an earlier created_at timestamp than the user, however with the rails logging set to debug I can again confirm that the insert user is happening before the insert identity.

    The same test repeatedly passes when using the pg gem. I have put in a conditional to detect the adapter in use at runtime of the tests and mark it as pending if using the mysql2 adapter.

  • username-removed-194173 Title changed from [WIP] Issue #993 (closed): Fixed login failure when extern_uid changes to Issue #993 (closed): Fixed login failure when extern_uid changes

    Title changed from [WIP] Issue #993 (closed): Fixed login failure when extern_uid changes to Issue #993 (closed): Fixed login failure when extern_uid changes

  • username-removed-194173 Title changed from Issue #993 (closed): Fixed login failure when extern_uid changes to [WIP] Issue #993 (closed): Fixed login failure when extern_uid changes

    Title changed from Issue #993 (closed): Fixed login failure when extern_uid changes to [WIP] Issue #993 (closed): Fixed login failure when extern_uid changes

  • username-removed-194173 Added 43 commits:

    Added 43 commits:

    • 4102204c...4a8076a3 - 42 commits from branch gitlab-org:master
    • f1756353 - Issue #993 (closed): Fixed login failure when extern_uid changes
  • username-removed-194173 Added 1 commit:

    Added 1 commit:

    • 0c7c8782 - Issue #993 (closed): Fixed login failure when extern_uid changes
  • username-removed-194173 Added 1 commit:

    Added 1 commit:

    • 85f05a7b - Issue #993 (closed): Fixed login failure when extern_uid changes
  • username-removed-194173 Title changed from [WIP] Issue #993 (closed): Fixed login failure when extern_uid changes to Issue #993 (closed): Fixed login failure when extern_uid changes

    Title changed from [WIP] Issue #993 (closed): Fixed login failure when extern_uid changes to Issue #993 (closed): Fixed login failure when extern_uid changes

  • username-removed-194173
    username-removed-194173 @joelkoglin ·
    Author Contributor

    @DouweM, can you take another look at this. I think this works the way we want. Also should I add a migration that runs a raw sql query to purge the duplicate entries of the same provider, leaving a single entry per provider?

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
    • lib/gitlab/ldap/user.rb
    44 44 gl_user.skip_reconfirmation!
    45 45 gl_user.email = auth_hash.email
    46 46
    47 # Build new identity only if we dont have have same one
    48 gl_user.identities.find_or_initialize_by(provider: auth_hash.provider,
    49 extern_uid: auth_hash.uid)
    47 # If we don't have an identity for this provider yet, create one
    48 if gl_user.identities.find_by(provider: auth_hash.provider).nil?
    49 gl_user.identities.new(extern_uid: auth_hash.uid, provider: auth_hash.provider)
    50 else # Update the UID attribute for this provider in case it has changed
    51 identity = gl_user.identities.select { |identity| identity.provider == auth_hash.provider }
    52 identity.first.extern_uid = auth_hash.uid
    53 end
    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer

      I don't understand how this is functionally different from:

      identity = gl_user.identities.find_or_initialize_by(provider: auth_hash.provider)
identity.extern_uid = auth_hash.uid

      Are you sure that doesn't work?

    • username-removed-194173
      username-removed-194173 @joelkoglin ·
      Author Contributor

      I am sure, running that block of code I get this:

        1) Gitlab::LDAP::User find_or_create connects to existing ldap user if the extern_uid changes
     Failure/Error: expect(existing_user.ldap_identity.extern_uid).to eql 'my-uid'

       expected: "my-uid"
            got: "old-uid"

       (compared using eql?)
     # ./spec/lib/gitlab/ldap/user_spec.rb:55:in `block (3 levels) in <top (required)>'

      My assumption is that find_or_initialize_by is returning a new copy of the identity object and is not a reference to the same identity returned by my select. This was also why I initially did not think the autosave: true in the user model was working. Following my assumption, I was then updating a copy of the identity that was not attached to the user object and the changes were never saved when user.save was called. However it did work when I called identity.save as it was a copy of the right identity, just not attached to the user object and getting the trigger to save when the user saved. This realization then brought me to my current solution where I select the identity from gl_user.idetites.

    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer

      All right, thank you, makes sense. Please add the migration, and I'll get this in.

  • Douwe Maan
    Douwe Maan @DouweM ·
    Maintainer

    Also should I add a migration that runs a raw sql query to purge the duplicate entries of the same provider, leaving a single entry per provider?

    @joelkoglin Yes! That would be useful.

  • username-removed-194173 Added 1 commit:

    Added 1 commit:

    • fa8517c9 - Issue #993 (closed): Adding migration to deduplicate user identities
  • username-removed-194173
    username-removed-194173 @joelkoglin ·
    Author Contributor

    fa8517c9: Added the db migration.

    • mysql does not support a DELETE FROM with a subselect on the same table
    • I create a temp table containing the fields used for the deduplication
    • The delete from is on the identities table with the subselect on the temp table
    • The temp table is then cleaned up

    I tested the queries on gitlab 7-13 on ubuntu 14.04 with standard mysql and postgresql packages

    I ran the final test of gitlab 7-8 upgrading to 7-13 with this migration added

    == 20150817163600 DeduplicateUserIdentities: migrating ========================
-- execute("DROP TABLE IF EXISTS tt_migration_DeduplicateUserIdentities;")
   -> 0.0011s
-- execute("CREATE TEMPORARY TABLE tt_migration_DeduplicateUserIdentities AS SELECT id,provider,user_id FROM identities;")
   -> 0.0021s
-- execute("DELETE FROM identities WHERE id NOT IN ( SELECT MIN(id) FROM tt_migration_DeduplicateUserIdentities GROUP BY user_id, provider);")
   -> 0.0013s
-- execute("DROP TABLE IF EXISTS tt_migration_DeduplicateUserIdentities;")
   -> 0.0008s
== 20150817163600 DeduplicateUserIdentities: migrated (0.0060s) ===============
  • Douwe Maan
    Douwe Maan @DouweM ·
    Maintainer

    @joelkoglin Awesome. I'll review tomorrow.

  • Douwe Maan
    Douwe Maan @DouweM started a thread on the diff
    • db/migrate/20150817163600_deduplicate_user_identities.rb 0 → 100644
    1 class DeduplicateUserIdentities < ActiveRecord::Migration
    2 def change
    3 execute 'DROP TABLE IF EXISTS tt_migration_DeduplicateUserIdentities;'
    4 execute 'CREATE TEMPORARY TABLE tt_migration_DeduplicateUserIdentities AS SELECT id,provider,user_id FROM identities;'
    5 execute 'DELETE FROM identities WHERE id NOT IN ( SELECT MIN(id) FROM tt_migration_DeduplicateUserIdentities GROUP BY user_id, provider);'
    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer

      Do we want to keep the first or the last identity? If we want the last, we'd need MAX rather than MIN, right?

    • username-removed-194173
      username-removed-194173 @joelkoglin ·
      Author Contributor

      The first identity created was correct until the extern_uid changed and a new identity was created. I feel like keeping the first identity since it was originally correct and should have been updated makes sense. On the next login the new code will update the original to have the correct extern_uid. There is also no guarantee that the MAX id will have the correct extern_uid currently as it may have changed again since the last login of that user.

      All that being said, it doesn't really matter either way.

      MIN:

      • closer to the real created_at date
      • most likely to be the wrong extern_uid (will be updated by code at login)

      MAX:

      • further from the original created_at date
      • most likely to have the currently correct extern_uid

      Let me know if you want me to change that. I would be happy to.

      Also should I use the drop_table from rails like this:

      drop_table 'obsolete_table' if ActiveRecord::Base.connection.table_exists? 'obsolete_table'

      It seemed a bit overkill for a temp table, but I also understand how ugly execute statements are in the migrations.

    • username-removed-194173
      username-removed-194173 @joelkoglin ·
      Author Contributor

      Also looks like there might be some errors in CI related to this migration. I will see if I can reproduce that and see what is going on.

      https://ci.gitlab.com/projects/4698/refs/feature_fix_ldap_auth_issue_993/commits/fa8517c956acd89467eb55a5c18ff37d27ad5b4d

    • Douwe Maan
      Douwe Maan @DouweM ·
      Maintainer
      • All right, MIN is fine.
      • I'm fine with execute in this case.
  • username-removed-194173 Added 1 commit:

    Added 1 commit:

    • ae392c47 - Issue #993 (closed): Updated the schema version to that of the new migration
  • username-removed-194173 Added 474 commits:

    Added 474 commits:

  • Douwe Maan Status changed to merged

    Status changed to merged

  • Douwe Maan
    Douwe Maan @DouweM ·
    Maintainer

    Thanks @joelkoglin, my apologies for taking so long to get it merged.

  • Douwe Maan mentioned in issue #1415 (closed)

    mentioned in issue #1415 (closed)

  • Douwe Maan mentioned in issue #1736 (closed)

    mentioned in issue #1736 (closed)

  • Drew Blessing
    Drew Blessing @dblessing ·
    Maintainer

    I can't say for sure, but this change may be causing issues for some users. We've had a large influx of LDAP identity issues in the weeks since the release. Way too many to call it a coincidence. In some cases users have no identities after upgrade and in some cases they have an identity but it doesn't work correctly. Deleting and re-creating fixes the issue.

    See #2442 (closed) for linked Zendesk issues. #2442 (closed) isn't the root cause but the feature would alleviate fixing the issue.

    /cc @DouweM

  • username-removed-194173
    username-removed-194173 @joelkoglin ·
    Author Contributor

    I don't have access to the zendesk ticket, but if you can provide some more info about the issue publicly I would be happy to look into it. Should we create a new issue ticket and link to this?

  • Stan Hu mentioned in issue #25853 (closed)

    mentioned in issue #25853 (closed)

  • username-removed-649880
    username-removed-649880 @daver ·

    The issue is still present in 9.2.5. Would be great to get this fix committed.

  • username-removed-1552775
    username-removed-1552775 @jeremy.collin.lnk ·

    The issue is still present in 9.5.2, I already had LDAP users created, but I can't have new users right now.

    EDIT: The sign-up module was inactive. It works now.

    Edited by username-removed-1552775
    • Please register or sign in to reply