Skip to content

Fix namespace validation (unique path) on group creation

Jonas Wälter requested to merge siemens/gitlab:fix/namespace-existence-check into master

What does this MR do?

There are two bugs in validating the namespace (unique path) when creating a group:

Initial situation:

There's a top-level group group-1 and its sub group group-2 resulting in the full path group-1/group-2:

Group Path Full path
Group 1 group-1 group-1
Group 2 (sub group of Group 1) group-2 group-1/group-2

Using the API, it's possible to create a new top-level group with path group-2 but it's not possible to create a new sub group with path group-2 in group Group 1 (resulting in full path group-1/group-2). This behavior is correct.

1. ~bug: Add a top-level group

Using the UI (https://gitlab.com/groups/new), if you enter Group 2 as group name, the path is set to group-2, but the path is changed to group-21 after a second. If you manually change the path back to group-2, the error Group path is already taken. Suggestions: group-21 is shown.

What happens in the background: To ensure that the path is unique, it checks if a namespace with the same path already exists. However, this check ignores nesting. So the validation prevents to create a top-level group group-2, because a nested group group-2 (with full path group-1/group-2) exists. This bug is fixed by this MR.

before MR after MR
Create_group_before Create_group_after

2. ~bug: Add a sub group

Using the UI (https://gitlab.com/groups/new?parent_id= <id of Group 1>), there is no unique path validation at all. So if you enter Group 2 as group name, the path is set to group-2 and doesn't change. Only after you press Create group, the error Group URL has already been taken is shown. This MR enables unique path validation for sub group as well (in the scope of its parent group).

before MR after MR
Create_subgroup_before Create_subgroup_after

Changes by this MR:

Previously, the endpoint /users/:namespace/suggests was used for the group path validation. This MR implements a separate API endpoint /api/v4/namespaces/:namespace/exists. The new endpoint is designed for namespaces and takes into account the nesting of namespaces.

🛠 with ❤ at Siemens

/cc @bufferoverflow

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

Database Review Information

Query info for Namespace.by_parent(params[:parent_id]).filter_by_name_or_path(namespace_path).exists? 
SELECT 1 AS one 
FROM "namespaces" 
WHERE "namespaces"."parent_id" = 9970
  AND (lower(path) = 'gitlab' OR lower(name) = 'gitlab')
LIMIT 1

Plan - no match - https://console.postgres.ai/shared/65f7b299-21b3-4c70-acf3-5dacfdf6f228

 Limit  (cost=0.56..15.72 rows=1 width=4) (actual time=16.948..16.950 rows=0 loops=1)
   Buffers: shared read=55 dirtied=1
   I/O Timings: read=15.828
   ->  Index Scan using index_namespaces_on_parent_id_and_id on public.namespaces  (cost=0.56..15.72 rows=1 width=4) (actual time=16.937..16.938 rows=0 loops=1)
         Index Cond: (namespaces.parent_id = 9970)
         Filter: ((lower((namespaces.path)::text) = 'gitlab'::text) OR (lower((namespaces.name)::text) = 'gitlab'::text))
         Rows Removed by Filter: 51
         Buffers: shared read=55 dirtied=1
         I/O Timings: read=15.828

Time: 26.003 ms
  - planning: 8.939 ms
  - execution: 17.064 ms
    - I/O read: 15.828 ms
    - I/O write: N/A

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

Plan - with match - https://console.postgres.ai/shared/8c741db7-5f2a-4525-a221-5330b618641c

 Limit  (cost=6.67..8.19 rows=1 width=4) (actual time=25.163..25.169 rows=1 loops=1)
   Buffers: shared read=14
   I/O Timings: read=24.753
   ->  Bitmap Heap Scan on public.namespaces  (cost=6.67..8.19 rows=1 width=4) (actual time=25.160..25.165 rows=1 loops=1)
         Buffers: shared read=14
         I/O Timings: read=24.753
         ->  BitmapAnd  (cost=6.67..6.67 rows=1 width=0) (actual time=21.911..21.915 rows=0 loops=1)
               Buffers: shared read=13
               I/O Timings: read=21.526
               ->  Bitmap Index Scan using index_namespaces_on_parent_id_and_id  (cost=0.00..2.12 rows=8 width=0) (actual time=6.251..6.252 rows=51 loops=1)
                     Index Cond: (namespaces.parent_id = 9970)
                     Buffers: shared read=4
                     I/O Timings: read=6.166
               ->  BitmapOr  (cost=4.30..4.30 rows=7 width=0) (actual time=15.649..15.651 rows=0 loops=1)
                     Buffers: shared read=9
                     I/O Timings: read=15.360
                     ->  Bitmap Index Scan using index_on_namespaces_lower_path  (cost=0.00..2.08 rows=2 width=0) (actual time=6.963..6.963 rows=2 loops=1)
                           Index Cond: (lower((namespaces.path)::text) = 'geo-team'::text)
                           Buffers: shared read=4
                           I/O Timings: read=6.890
                     ->  Bitmap Index Scan using index_on_namespaces_lower_name  (cost=0.00..2.22 rows=5 width=0) (actual time=8.684..8.684 rows=1 loops=1)
                           Index Cond: (lower((namespaces.name)::text) = 'geo-team'::text)
                           Buffers: shared read=5
                           I/O Timings: read=8.471

Time: 27.178 ms
  - planning: 1.936 ms
  - execution: 25.242 ms
    - I/O read: 24.753 ms
    - I/O write: N/A

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

Edited by Jonas Wälter

Merge request reports