My current context:

• ApplicationController always sets Current.organization ; I think for pre-signed-in pages this always defaults to organization_id = 1 .
• Controllers that do not inherit from ApplicationController may need investigation, particularly Devise and Doorkeeper controllers. It looks like this is already set for OmniauthCallbacksController, so maybe there's no code needed here

It looks like we have a service to reconcile claims between gitlab-rails and topology service here. I don't see where this is run from - is there a cron job or rake task that kicks off this process?

It would be important to backfill existing claims before allowing new users to be created on new cells, otherwise we risk having multiple users with the same primary email. I'm sure the problem has been solved for group / username / organization paths, I just want to make sure we're approaching it in the same way. Thanks!

Andrew Evans (5ff545b7) at 18 Mar 21:43

Andrew Evans (11845016) at 18 Mar 21:43

... and 1 more commit

What does this MR do and why?

Preserve SAML identities in the PruneDeletionsWorker.

This means that SAML identities are preserved for users who are removed from a group via the dormant group users feature.

Ultimately, this is to solve #588829. In that issue, we will push SCIM membership removal upon user deletion into a background job using the Members::ScheduleDeletionService. This is the same service that we use during membership removal for dormant group users.

When memberships are removed during a SCIM request to delete a user, the SAML identity is preserved. When memberships are removed during dormant membership removal, the SAML identity is deleted. We believe this seems to be a minor error and can be changed - the SAML identity can be preserved in this case as well.

So in this MR, we will preserve the SAML identity during dormant membership removal, so that it will also be preserved when we use the same service during SCIM user removal.

References

• Issue: #588829
• Draft MR and discussion: !226577
• !121226
• See discussion in this thread: !226577 (comment 3153267791)

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

What does this MR do and why?

Preserve SAML identities in the PruneDeletionsWorker.

This means that SAML identities are preserved for users who are removed from a group via the dormant group users feature.

Ultimately, this is to solve #588829. In that issue, we will push SCIM membership removal upon user deletion into a background job using the Members::ScheduleDeletionService. This is the same service that we use during membership removal for dormant group users.

When memberships are removed during a SCIM request to delete a user, the SAML identity is preserved. When memberships are removed during dormant membership removal, the SAML identity is deleted. We believe this seems to be a minor error and can be changed - the SAML identity can be preserved in this case as well.

So in this MR, we will preserve the SAML identity during dormant membership removal, so that it will also be preserved when we use the same service during SCIM user removal.

References

• Issue: #588829
• Draft MR and discussion: !226577
• !121226
• See discussion in this thread: !226577 (comment 3153267791)

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

note (non-blocking): this line is being reported as not having test coverage. I think it does have coverage, but only in the CE pipeline, not the primary one where coverage is reported. The issue is that the overridden method doesn't call super , so the CE-version of the method is never called. We could refactor to something like this so that super would be called and coverage would be reported:

::Members::DestroyService.new(scheduled_by).execute(member, **options_for_destroy_member)

private

def options_for_destroy_member
  { skip_subresources: true }
end

# in EE override class
def options_for_destroy_member
  super.merge(skip_saml_identity: true)
end

I see we already have pipeline:skip-undercoverage applied, and I'm comfortable with that here.