Skip to content

User can be added as subgroup/project member with SSO Enforce with SAML identity present

Summary

With skip_saml_identity_destroy_during_scim_deprovision feature flag on, users' SAML identities were not being removed. With SSO enforce on, users cannot be added manually to the group/subgroup/project unless they are a member of the top-level group. However, it turns out that's not how we were enforcing it.

Steps to reproduce

  1. Turn on skip_saml_identity_destroy_during_scim_deprovision
  2. Have a group with SAML set up, and SSO enforce on.
  3. Have a user with SAML and SCIM identities.
  4. Remove the user through SCIM.
  5. User is removed from group (and everything underneath). User still has SAML and SCIM identities.
  6. Add user via UI or API to a subgroup or project.

What is the current bug behavior?

User can be manually added to subgroups/projects by others despite not being a member of the group.

What is the expected correct behavior?

User cannot be manually added anywhere in the group.

Relevant code

Our member model has a sso enforcement check, which checks ::Gitlab::Auth::GroupSaml::MembershipEnforcer.new(group).can_add_user?(user).

The membership enforcer only checks that a user has a SAML identity tied to the group using GroupSamlIdentityFinder.new(user: user).find_linked(group: root_group). Identity finder.

Output of checks

GitLab.com, 16.1.0-pre 3f94ec39

Possible fixes

From @dblessing

Easiest thing might be for us to check SCIM active: false [and] whether they’re a top-level member

We should also consider whether to add specs to check this sort of thing.

Possible methods of remediation

The feature flag was on from 2023-05-15 18:50 UTC until 2023-05-25 17:57 UTC.

  1. For users that were removed through SCIM during that time, you can update the SAML identifier via API for each user to something non-existent. This will not remove the SAML identity, but will prevent the user from signing in.
  2. Re-add those users through SCIM. Then, after enough time has passed for each user's SCIM active to be set to true. Remove them again through SCIM.
Edited by Cynthia "Arty" Ng