Instance SAML does not respect `external_provider` configuration
Original Issue Description
GitLab OmniAuth has a feature to designate a provider as an external provider. When designated, it means that all users created from that provider will have their external
attribute set to true. External users are unable to see groups and projects that are Internal
visibility.
We found that instance SAML does not adhere to this configuration. Instead, SAML has external groups.
We may not have intended SAML to use the external providers configuration. However, since we document it in the example we should probably treat it as a bug. And probably a security bug at that. There's no reason instance SAML can't have both external provider and external group configurations. They have separate use-cases.
- The code to set
external
for users of external providers is at https://gitlab.com/gitlab-org/gitlab/-/blob/a3a513703384ac0224d271e60f650ebc66914b83/lib/gitlab/auth/o_auth/user.rb#L87. - The code to set
external
for users of SAML external groups is at https://gitlab.com/gitlab-org/gitlab/-/blob/a3a513703384ac0224d271e60f650ebc66914b83/lib/gitlab/auth/saml/user.rb#L22
Cause and Potential Solution
The bug occurs because the latter method above overrides the former. A potential solution is to move this logic outside the find_user
method and instead into the build_new_user
method. For OAuth::User
we can set the attribute after the build service (it's not an allowed attribute, I don't believe) and then for SAML::User
we can also move it to the build_new_user
portion where we tap super. We need to ensure that an external provider setting to true takes precedent over external groups - for lack of a user being in an external group, we should leave the setting as true if external provider set it. However, we should also make note in documentation that instances should avoid setting both external provider and external groups for the same provider.
Related to https://gitlab.com/gitlab-org/security/gitlab/-/issues/1225
Summary
GitLab OmniAuth allows customers to designate an OmniAuth provider as an external provider, which changes enables the user attribute 'external':true
, turning them into external users. External users cannot access groups and projects with Internal visibility.
SAML OmniAuth allows customers to create external users via external groups.
This results in a conflict where the external provider setting is overridden by the external groups configuration, leading to potential security/compliance concern.
For example:
- A user who is authenticated via an external provider should ideally be marked external regardless of their group memberships
- If a user is part of an external group that does not mark them as external, the group configuration can override the provider-level designation, leaving the user as internal when they should be external
Both configurations have separate use cases, and since this is documented as an example in GitLab, it should be treated as a bug.
Steps to Reproduce
- Configure an instance-wide SAML provider and set it as an external provider in GitLab
- Set up SAML external groups without enabling the external attribute
- Create a user via the SAML provider
- Verify that the user is marked as an internal user in the group
What is the current bug behaviour?
When a user is created via the SAML provider, the external groups setting overrides the external provider configuration. As a result, the user may not have the external
attribute set to true
, depending on their external group membership.
What is the expected correct behaviour?
The external
attribute should be set to true
for all users from the external provider, regardless of their external group membership.
External provider settings should take precedence over external groups.
Cause
The issue is caused by the logic in the codebase, where the external group setting for SAML users overrides the external provider setting. Specifically, the SAML group logic takes precedence in the find_user
method, instead of respecting the external provider setting.
- Code for setting external provider users: OAuth::User
- Code for setting external SAML group users: SAML::User
Potential Solution
A potential solution is to move this logic outside the find_user
method and instead into the build_new_user
method
- For the
OAuth::User
, we can set the attribute after the build service (it's not an allowed attribute, I don't believe) - For the
SAML::User
, we can also move it to thebuild_new_user
portion where we tap super.
We need to ensure that an external provider setting to true takes precedent over external groups - for lack of a user being in an external group, we should leave the setting as true if external provider set it.
We should also make note in documentation that instances should avoid setting both external provider and external groups for the same provider (i.e. The external provider will setting take precedence).