Skip to content

Restrict user autocompletes to SAML enforced members

Magdalena Frankiewicz requested to merge restrict-user-autocompletes into master

What does this MR do?

Solves #13699 (closed)

To satisfy the acceptance criteria, the frontend was changed to pass a new param saml_provider_id when SAML/SSO is enforced by the group. When that parameter is passed the Autocomplete::UsersFinder will modify the query to only include users that have an identity for the given SAML provider.

Local Testing

  1. Install an EE license: https://about.gitlab.com/handbook/developer-onboarding/#working-on-gitlab-ee.
  2. Enable the following feature flags: group_saml, enforced_sso, and enforced_sso_requires_session
  3. Add group_saml to config/gitlab.yml. See https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/master/doc/howto/saml.md#gitlab-configuration. Note: Ensure this is added to the development: section of your config
  4. Follow the instructions in https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/master/doc/howto/saml.md for setting up a local SAML provider using docker. You will need to enable HTTPS
  5. Create a group
  6. Navigate to "Settings -> SAML SSO"
  7. Toggle on "Enable SAML authentication for this group." and add the "Identity provider single sign on URL" and "Certificate fingerprint" from https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/master/doc/howto/saml.md#configuring-the-group
  8. Navigate to "GitLab single sign on URL" found in "Settings -> SAML SSO" and authorize your account.
  9. In "Settings -> SAML SSO" enable "Enforce SSO-only authentication for this group."
  10. In a private/incognito window navigate to the "GitLab single sign on URL" found in "Settings -> SAML SSO" and create a user with the user2 credentials (https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/master/doc/howto/saml.md#credentials)
  11. With your group owner account navigate to the "Members" section of the group. The dropdown should now only show user's with SAML enabled.

For database review

Before modification

query:

SELECT
    "users".*
FROM
    "users"
WHERE ("users"."state" IN ('active'))
    AND (ghost IS NOT TRUE)
    AND ("users"."user_type" IS NULL
        OR "users"."user_type" NOT IN (2, 1, 3))
ORDER BY
    "users"."name" ASC
LIMIT 20;

plan:

Limit  (cost=0.56..3.61 rows=20 width=1205) (actual time=26.154..70.718 rows=20 loops=1)
   Buffers: shared read=25 dirtied=1
   I/O Timings: read=68.955
   ->  Index Scan using index_users_on_name on public.users  (cost=0.56..818240.12 rows=5359135 width=1205) (actual time=26.152..70.702 rows=20 loops=1)
         Filter: ((users.ghost IS NOT TRUE) AND ((users.state)::text = 'active'::text) AND ((users.user_type IS NULL) OR (users.user_type <> ALL ('{2,1,3}'::integer[]))))
         Rows Removed by Filter: 1
         Buffers: shared read=25 dirtied=1
         I/O Timings: read=68.955

summary:

Time: 71.296 ms
  - planning: 0.482 ms
  - execution: 70.814 ms
    - I/O read: 68.955 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 0 from the buffer pool
  - reads: 25 (~200.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 1 (~8.00 KiB)
  - writes: 0

After modification

query:

SELECT DISTINCT
    "users".*
FROM
    "users"
    INNER JOIN "identities" ON "identities"."user_id" = "users"."id"
WHERE ("users"."state" IN ('active'))
    AND (ghost IS NOT TRUE)
    AND ("users"."user_type" IS NULL
        OR "users"."user_type" NOT IN (2, 1, 3))
    AND "identities"."saml_provider_id" = 3
ORDER BY
    "users"."name" ASC
LIMIT 20

plan:

Limit  (cost=58.99..60.90 rows=9 width=1205) (actual time=11.321..11.322 rows=1 loops=1)
   Buffers: shared hit=24 read=6
   I/O Timings: read=10.920
   ->  Unique  (cost=58.99..60.90 rows=9 width=1205) (actual time=11.319..11.320 rows=1 loops=1)
         Buffers: shared hit=24 read=6
         I/O Timings: read=10.920
         ->  Sort  (cost=58.99..59.01 rows=9 width=1205) (actual time=11.317..11.317 rows=1 loops=1)
               Sort Key: users.name, users.id, users.email, users.encrypted_password, users.reset_password_token, users.reset_password_sent_at, users.remember_created_at, users.sign_in_count, users.current_sign_in_at, users.last_sign_in_at, users.current_sign_in_ip, users.last_sign_in_ip, users.created_at, users.updated_at, users.admin, users.projects_limit, users.skype, users.linkedin, users.twitter, users.bio, users.failed_attempts, users.locked_at, users.username, users.can_create_group, users.can_create_team, users.color_scheme_id, users.password_expires_at, users.created_by_id, users.avatar, users.confirmation_token, users.confirmed_at, users.confirmation_sent_at, users.unconfirmed_email, users.hide_no_ssh_key, users.website_url, users.last_credential_check_at, users.admin_email_unsubscribed_at, users.notification_email, users.hide_no_password, users.password_automatically_set, users.location, users.public_email, users.encrypted_otp_secret, users.encrypted_otp_secret_iv, users.encrypted_otp_secret_salt, users.otp_required_for_login, users.otp_backup_codes, users.dashboard, users.project_view, users.consumed_timestep, users.layout, users.hide_project_limit, users.unlock_token, users.note, users.otp_grace_period_started_at, users.external, users.organization, users.incoming_email_token, users.auditor, users.ghost, users.require_two_factor_authentication_from_group, users.two_factor_grace_period, users.notified_of_own_activity, users.last_activity_on, users.preferred_language, users.email_opted_in, users.email_opted_in_ip, users.email_opted_in_source_id, users.email_opted_in_at, users.theme_id, users.accepted_term_id, users.feed_token, users.private_profile, users.roadmap_layout, users.include_private_contributions, users.commit_email, users.group_view, users.managing_group_id, users.bot_type, users.first_name, users.last_name, users.static_object_token, users.role, users.user_type
               Sort Method: quicksort  Memory: 27kB
               Buffers: shared hit=24 read=6
               I/O Timings: read=10.920
               ->  Nested Loop  (cost=0.72..58.85 rows=9 width=1205) (actual time=11.004..11.007 rows=1 loops=1)
                     Buffers: shared hit=1 read=6
                     I/O Timings: read=10.920
                     ->  Index Scan using index_identities_on_saml_provider_id on public.identities  (cost=0.29..18.65 rows=9 width=4) (actual time=4.453..4.454 rows=1 loops=1)
                           Index Cond: (identities.saml_provider_id = 3)
                           Buffers: shared read=3
                           I/O Timings: read=4.424
                     ->  Index Scan using users_pkey on public.users  (cost=0.43..4.46 rows=1 width=1205) (actual time=6.540..6.542 rows=1 loops=1)
                           Index Cond: (users.id = identities.user_id)
                           Filter: ((users.ghost IS NOT TRUE) AND ((users.state)::text = 'active'::text) AND ((users.user_type IS NULL) OR (users.user_type <> ALL ('{2,1,3}'::integer[]))))
                           Rows Removed by Filter: 0
                           Buffers: shared hit=1 read=3
                           I/O Timings: read=6.496

summary:

Time: 13.167 ms
  - planning: 1.620 ms
  - execution: 11.547 ms
    - I/O read: 10.920 ms
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 24 (~192.00 KiB) from the buffer pool
  - reads: 6 (~48.00 KiB) from the OS file cache, including disk I/O
  - dirtied: 0
  - writes: 0

Screenshots

Page Before After
Root Group Screen_Shot_2020-04-03_at_12.45.25_PM Screen_Shot_2020-04-03_at_12.51.16_PM
Subgroup Screen_Shot_2020-04-03_at_12.46.03_PM Screen_Shot_2020-04-03_at_12.36.32_PM

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Heinrich Lee Yu

Merge request reports