Restrict user autocompletes (!30607) · Merge requests · GitLab.org / GitLab

Magdalena Frankiewicz requested to merge fixed-restrict-user-autocompletes into master Apr 28, 2020

What does this MR do?

It fixes problem in !28115 (merged), that was reverted because of problem described here #215886 (closed)

For database review

BEFORE modification

A. Query when you focus on the field without inputting any characters:

SELECT
    "users".*
FROM
    "users"
WHERE ("users"."state" IN ('active'))
    AND ("users"."user_type" IS NULL
        OR "users"."user_type" IN (NULL, 6, 4))
ORDER BY
    "users"."name" ASC
LIMIT 20

plan:

 Limit  (cost=0.56..3.63 rows=20 width=1216) (actual time=12.062..61.882 rows=20 loops=1)
   Buffers: shared read=25
   I/O Timings: read=61.532
   ->  Index Scan using index_users_on_name on public.users  (cost=0.56..844808.63 rows=5506343 width=1216) (actual time=12.026..61.834 rows=20 loops=1)
         Filter: (((users.state)::text = 'active'::text) AND ((users.user_type IS NULL) OR (users.user_type = ANY ('{NULL,6,4}'::integer[]))))
         Rows Removed by Filter: 1
         Buffers: shared read=25
         I/O Timings: read=61.532

summary:

Time: 62.461 ms
  - planning: 0.472 ms
  - execution: 61.989 ms
    - I/O read: 61.532 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: 0
  - writes: 0

B. Query when at least 3 characters are inputted:

SELECT
    "users".*
FROM
    "users"
WHERE ("users"."state" IN ('active'))
    AND ("users"."user_type" IS NULL
        OR "users"."user_type" IN (NULL, 6, 4))
    AND (("users"."name" ILIKE '%all%'
            OR "users"."username" ILIKE '%all%')
        OR "users"."email" = 'all')
ORDER BY
    CASE WHEN users.name = 'all' THEN
        0
    WHEN users.username = 'all' THEN
        1
    WHEN users.email = 'all' THEN
        2
    ELSE
        3
    END,
    "users"."name" ASC
LIMIT 20

plan:

 Limit  (cost=200670.03..200670.08 rows=20 width=1220)
  ->  Sort  (cost=200670.03..200854.76 rows=73893 width=1220)
        Sort Key: (CASE WHEN ((name)::text = 'all'::text) THEN 0 WHEN ((username)::text = 'all'::text) THEN 1 WHEN ((email)::text = 'all'::text) THEN 2 ELSE 3 END), name
        ->  Bitmap Heap Scan on users  (cost=786.56..198703.76 rows=73893 width=1220)
              Recheck Cond: (((name)::text ~~* '%all%'::text) OR ((username)::text ~~* '%all%'::text) OR ((email)::text = 'all'::text))
              Filter: (((state)::text = 'active'::text) AND ((user_type IS NULL) OR (user_type = ANY ('{NULL,6,4}'::integer[]))))
              ->  BitmapOr  (cost=786.56..786.56 rows=75010 width=0)
                    ->  Bitmap Index Scan on index_users_on_name_trigram  (cost=0.00..497.19 rows=52692 width=0)
                          Index Cond: ((name)::text ~~* '%all%'::text)
                    ->  Bitmap Index Scan on index_users_on_username_trigram  (cost=0.00..231.38 rows=22318 width=0)
                          Index Cond: ((username)::text ~~* '%all%'::text)
                    ->  Bitmap Index Scan on users_email_key  (cost=0.00..2.56 rows=1 width=0)
                          Index Cond: ((email)::text = 'all'::text)

summary:

Time: 56.524 s
  - planning: 4.131 ms
  - execution: 56.520 s
    - I/O read: 54.648 s
    - I/O write: 0.000 ms

Shared buffers:
  - hits: 11 (~88.00 KiB) from the buffer pool
  - reads: 58971 (~460.70 MiB) from the OS file cache, including disk I/O
  - dirtied: 590 (~4.60 MiB)
  - writes: 0

AFTER modification

A. Query when you focus on the field without inputting any characters:

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

plan

 Limit  (cost=59.65..59.67 rows=9 width=1216) (actual time=2.038..2.038 rows=0 loops=1)
   Buffers: shared read=2
   I/O Timings: read=1.977
   ->  Sort  (cost=59.65..59.67 rows=9 width=1216) (actual time=2.037..2.037 rows=0 loops=1)
         Sort Key: users.name
         Sort Method: quicksort  Memory: 25kB
         Buffers: shared read=2
         I/O Timings: read=1.977
         ->  Nested Loop  (cost=0.72..59.50 rows=9 width=1216) (actual time=2.027..2.027 rows=0 loops=1)
               Buffers: shared read=2
               I/O Timings: read=1.977
               ->  Index Scan using index_identities_on_saml_provider_id on public.identities  (cost=0.29..19.31 rows=9 width=4) (actual time=2.026..2.026 rows=0 loops=1)
                     Index Cond: (identities.saml_provider_id = 2)
                     Buffers: shared read=2
                     I/O Timings: read=1.977
               ->  Index Scan using users_pkey on public.users  (cost=0.43..4.46 rows=1 width=1216) (actual time=0.000..0.000 rows=0 loops=0)
                     Index Cond: (users.id = identities.user_id)
                     Filter: (((users.state)::text = 'active'::text) AND ((users.user_type IS NULL) OR (users.user_type = ANY ('{NULL,6,4}'::integer[]))))
                     Rows Removed by Filter: 0

summary

Time: 3.032 ms
  - planning: 0.820 ms
  - execution: 2.212 ms
    - I/O read: 1.977 ms
    - I/O write: 0.000 ms

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

B. Query when at least 3 characters are inputted:

SELECT
    "users".*
FROM
    "users"
    INNER JOIN "identities" ON "identities"."user_id" = "users"."id"
WHERE
    "identities"."saml_provider_id" = 2
    AND ("users"."state" IN ('active'))
    AND ("users"."user_type" IS NULL
        OR "users"."user_type" IN (NULL, 6, 4))
    AND (("users"."name" ILIKE '%all%'
            OR "users"."username" ILIKE '%all%')
        OR "users"."email" = 'all')
ORDER BY
    CASE WHEN users.name = 'all' THEN
        0
    WHEN users.username = 'all' THEN
        1
    WHEN users.email = 'all' THEN
        2
    ELSE
        3
    END,
    "users"."name" ASC
LIMIT 20

plan

 Limit  (cost=59.59..59.59 rows=1 width=1220) (actual time=0.025..0.026 rows=0 loops=1)
   Buffers: shared hit=2
   ->  Sort  (cost=59.59..59.59 rows=1 width=1220) (actual time=0.024..0.024 rows=0 loops=1)
         Sort Key: (CASE WHEN ((users.name)::text = 'all'::text) THEN 0 WHEN ((users.username)::text = 'all'::text) THEN 1 WHEN ((users.email)::text = 'all'::text) THEN 2 ELSE 3 END), users.name
         Sort Method: quicksort  Memory: 25kB
         Buffers: shared hit=2
         ->  Nested Loop  (cost=0.72..59.58 rows=1 width=1220) (actual time=0.012..0.012 rows=0 loops=1)
               Buffers: shared hit=2
               ->  Index Scan using index_identities_on_saml_provider_id on public.identities  (cost=0.29..19.31 rows=9 width=4) (actual time=0.012..0.012 rows=0 loops=1)
                     Index Cond: (identities.saml_provider_id = 2)
                     Buffers: shared hit=2
               ->  Index Scan using users_pkey on public.users  (cost=0.43..4.46 rows=1 width=1216) (actual time=0.000..0.000 rows=0 loops=0)
                     Index Cond: (users.id = identities.user_id)
                     Filter: (((users.state)::text = 'active'::text) AND ((users.user_type IS NULL) OR (users.user_type = ANY ('{NULL,6,4}'::integer[]))) AND (((users.name)::text ~~* '%all%'::text) OR ((users.username)::text ~~* '%all%'::text) OR ((users.email)::text = 'all'::text)))
                     Rows Removed by Filter: 0

summary

Time: 6.040 ms
  - planning: 5.817 ms
  - execution: 0.223 ms
    - I/O read: 0.000 ms
    - I/O write: 0.000 ms

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

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
Tested in all supported browsers
Informed Infrastructure department of a default or new setting change, if applicable per definition of done

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 Apr 29, 2020 by Magdalena Frankiewicz

Restrict user autocompletes

What does this MR do?

For database review

BEFORE modification

A. Query when you focus on the field without inputting any characters:

plan:

summary:

B. Query when at least 3 characters are inputted:

plan:

summary:

AFTER modification

A. Query when you focus on the field without inputting any characters:

plan

summary

B. Query when at least 3 characters are inputted:

plan

summary

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Merge request reports