Skip to content

Fix regression and allow SCIM to create SAML identity

Drew Blessing requested to merge dblessing-scim-identitites-new-user-flow into master

What does this MR do?

Related to #216646 (closed). Prior to scim_identities SCIM would create new users along with their SAML identity. This allowed new users creating by SCIM to sign in with the SAML identity without requiring them to reset their GitLab local password. This change restores that ability.

Database migration details

NOTE: I worked on the database migration with @ahegyi help. He helped formulate the query and we looked at the query plans, etc. on GitLab.com

I estimate the most time this could take on production would be around 30 seconds. In #database-lab the first batch on a cold cache took about 9 seconds. Further batches took around 1 second. If I buffer those a bit (round up to 10 seconds and 1.5 seconds respectively) and consider about 12 batches it may take around 26.5 seconds.

SQL

INSERT INTO identities (extern_uid, provider, user_id, created_at, updated_at, saml_provider_id)
SELECT
    scim_identities.extern_uid,
    'group_saml',
    scim_identities.user_id,
    CURRENT_TIMESTAMP,
    CURRENT_TIMESTAMP,
    saml_providers.id
FROM
    "scim_identities"
    INNER JOIN "users" ON "users"."id" = "scim_identities"."user_id"
    INNER JOIN saml_providers ON saml_providers.group_id = scim_identities.group_id
WHERE (date_trunc('second', scim_identities.created_at) at time zone 'UTC' = date_trunc('second', users.created_at))
    AND "scim_identities"."user_id" NOT IN (
        SELECT
            "identities"."user_id"
        FROM
            "identities"
            INNER JOIN "saml_providers" ON "saml_providers"."id" = "identities"."saml_provider_id")
ON CONFLICT
    DO NOTHING;

Migrate

== 20200506154421 MigrateScimIdentitiesToSamlForNewUsers: migrating ===========
-- execute("insert into identities (extern_uid, provider, user_id, created_at, updated_at, saml_provider_id) SELECT scim_identities.extern_uid, 'group_saml', scim_identities.user_id, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP, saml_providers.id FROM \"scim_identities\" INNER JOIN \"users\" ON \"users\".\"id\" = \"scim_identities\".\"user_id\" inner join saml_providers on saml_providers.group_id=scim_identities.group_id WHERE \"scim_identities\".\"id\" >= 1 AND (date_trunc('second',scim_identities.created_at) at time zone 'UTC' = date_trunc('second',users.created_at)) AND \"scim_identities\".\"user_id\" NOT IN (SELECT \"identities\".\"user_id\" FROM \"identities\" INNER JOIN \"saml_providers\" ON \"saml_providers\".\"id\" = \"identities\".\"saml_provider_id\") on conflict do nothing")
   -> 0.0026s
== 20200506154421 MigrateScimIdentitiesToSamlForNewUsers: migrated (0.0153s) ==

Rollback (no-op)

Rollback is a no-op. We don't need to remove anything or worry if a rollback is necessary.

== 20200506154421 MigrateScimIdentitiesToSamlForNewUsers: reverting ===========
== 20200506154421 MigrateScimIdentitiesToSamlForNewUsers: reverted (0.0000s) ==

EXPLAIN ANALYZE (Note we will be running this in batches of 100. This explain is for the entire chunk)

 Nested Loop  (cost=1.44..268803126.17 rows=26 width=92) (actual time=19479.192..19479.192 rows=0 loops=1)
   Buffers: shared hit=19248
   ->  Nested Loop  (cost=1.16..268803117.88 rows=26 width=48) (actual time=19479.191..19479.191 rows=0 loops=1)
         Buffers: shared hit=19248
         ->  Seq Scan on public.scim_identities  (cost=0.73..268781217.06 rows=5206 width=56) (actual time=31.068..19474.747 rows=211 loops=1)
               Filter: (NOT (SubPlan 1))
               Rows Removed by Filter: 11741
               Buffers: shared hit=18404
               SubPlan 1
                 ->  Materialize  (cost=0.73..46104.93 rows=2211871 width=4) (actual time=0.000..0.548 rows=8781 loops=11952)
                       Buffers: shared hit=18165
                       ->  Merge Join  (cost=0.73..481.57 rows=2211871 width=4) (actual time=0.020..20.702 rows=19429 loops=1)
                             Merge Cond: (saml_providers_1.id = identities.saml_provider_id)
                             Buffers: shared hit=18165
                             ->  Index Only Scan using saml_providers_pkey on public.saml_providers saml_providers_1  (cost=0.28..36.49 rows=678 width=4) (actual time=0.009..0.187 rows=677 loops=1)
                                   Heap Fetches: 220
                                   Buffers: shared hit=56
                             ->  Index Scan using index_identities_on_saml_provider_id on public.identities  (cost=0.29..24338.75 rows=2211871 width=8) (actual time=0.007..14.379 rows=19429 loops=1)
                                   Buffers: shared hit=18109
         ->  Index Scan using users_pkey on public.users  (cost=0.43..4.20 rows=1 width=12) (actual time=0.018..0.018 rows=0 loops=211)
               Index Cond: (users.id = scim_identities.user_id)
               Filter: (timezone('UTC'::text, date_trunc('second'::text, scim_identities.created_at)) = date_trunc('second'::text, users.created_at))
               Rows Removed by Filter: 1
               Buffers: shared hit=844
   ->  Index Scan using index_saml_providers_on_group_id on public.saml_providers  (cost=0.28..0.30 rows=1 width=8) (actual time=0.000..0.000 rows=0 loops=0)
         Index Cond: (saml_providers.group_id = scim_identities.group_id)

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 Drew Blessing

Merge request reports