`Member.add_user`doesn't detect existing members that have requested access

Member.add_user should check for an existing access request and remove the requested_at timestamp to make it a real/active member. Currently, it will silently fail to add the user as a member.

LDAP group sync worked around this temporarily at https://gitlab.com/gitlab-org/gitlab-ee/blob/master/lib/ee/gitlab/ldap/sync/group.rb#L170-174

mentioned in issue #830 (closed)

Thanks @dblessing

In addition to this behaviour, #830 (closed) questioned whether it made sense that users can request access to a group where LDAP sync is enabled. It seems users should request membership of the LDAP group instead (not via GitLab obviously). It may be that we still want to offer the possibility to request access in GitLab in order to notify group admins of the request, but then the notification text sent to admins should probably somehow take into account the fact that the user then has to be added to a LDAP group and it won't be possible to accept or reject the access request from the GitLab UI.

Added ~481018 label

Added ~113221 label

Added ~313021 and removed ~113221 labels

Requested by customer https://gitlab.zendesk.com/agent/tickets/33423 | @dblessing Can you take a look at this.

Also reported by https://gitlab.zendesk.com/agent/tickets/33632

@DouweM This is an issue with Member.add_user not handling existing access requests. I thought it would be an easy fix but because of the current method signature it makes the problem a little bit more thorny.

Basically, members are passed to the method, and members do not included requesters. We could do something like:

member = members.first.source.requesters.find_by(user_id: user.id)
member ||= members.find_or_initialize_by(user_id: user.id)

but that is really ugly and requires that the group/project have at least one member. If not, it all breaks down. So are we forced to rewrite the entire method and change the signature? If the group/project object was passed instead we could do it.

Any ideas how we can handle this? If we can't come up with a good permanent fix for 8.11, I'm inclined to add some hack to LDAP sync that looks for a requester first, deletes it (if present), and then goes forward with adding the user. I think the Member.add_user method should detect requests, though. Otherwise, we get a silent failure.

@rymai Please share your thoughts here, as the resident expert on access requests :)

I think the Member.add_user method should detect requests, though.

@dblessing Definitely, I think this method should be able to detect a requester and accepts their access request.

I'm inclined to add some hack to LDAP sync that looks for a requester first, deletes it (if present), and then goes forward with adding the user.

That's what I do in the access requests API (https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4833/diffs#67e5b7398c48d3a276a5258e5e30cc50725d78f3_0_65) but I'm not proud of it, we should definitely improve Member.add_user to handle this case (I could do it for %8.12 if you'd like, and you can hack LDAP for %8.11... As you prefer.

As someone who is dealing with the fallout from this for the second time in two days, I vote for the soonest possible solution.

@dstanley Try this patch if you want to fix this now. Make the change in lib/gitlab/ldap/group_sync.rb (8.9 and lower) or lib/ee/gitlab/ldap/sync/group.rb (8.10).

          def add_or_update_user_membership(user, group, access)
            # Prevent the last owner of a group from being demoted
            if access < ::Gitlab::Access::OWNER && group.last_owner?(user)
              warn_cannot_remove_last_owner(user, group)
            else
              member = group.requesters.find_by(user_id: user.id)
              if member.present?
                member.requested_at = nil
                member.save
              else
                # If you pass the user object, instead of just user ID,
                # it saves an extra user database query.
                group.add_users([user], access, skip_notification: true)
              end
            end
          end

The portion of the method I changed is:

              group.add_users([user], access, skip_notification: true)

to

              member = group.requesters.find_by(user_id: user.id)
              if member.present?
                member.requested_at = nil
                member.save
              else
                # If you pass the user object, instead of just user ID,
                # it saves an extra user database query.
                group.add_users([user], access, skip_notification: true)
              end

mentioned in merge request !655 (merged)

@dstanley I submited !655 (merged) for this. Specs validate that the above code will work. You can apply it to your instance as a patch if you wish.

mentioned in commit ce06cc65

mentioned in commit 78f74c35

Changed title: LDAP group sync doesn't add users that have requested access → {+Member.add_userdoesn't detect existing memb+}ers that have requested access

Milestone changed to %8.12

@rymai Can you look at the proper fix here for 8.12? Potentially related to https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5566.

@DouweM Definitely, I will!

Reassigned to @rymai

Moved to gitlab-org/gitlab-ce#21983