Skip to content
Snippets Groups Projects

Switch top-level-ns codepath from quering namespace endpoint to quering group endpoint

Merged Pawel Rozlach requested to merge prozlach/gatekeeper_top-level-ns_fix into main
All threads resolved!

Resolves #2376 (closed) Followup for a slack discussion here

This MR makes gatekeeper query /groups GL API endpoint instead of the /namespace

The problem is that GL does not allow to obtain information about private top-level group directly in case when you have a GL Access Token for one of its subgroups.

It is possible to get this information using indirect API calls (see below), but only using the /group API endpoint and not the /namespace endpoint because in case of a namespace query, even public groups are unavailable. This is due to the fact that access check is based on read_namespace when accessing the /namespaces/:id API but read_group when accessing the /groups/:id API and these are different.

For example, let's assume that you create a private top-level group with ID 96, and a subgroup of this group with ID 97. Using the group-acccess token for group 97, you will not be able to query group 96' data directly, only indirectly:

$ curl -kH "private-token: glpat-..." https://gdk.devvm:3443/api/v4/groups/96 | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100    33  100    33    0     0     16      0  0:00:02  0:00:02 --:--:--    16
{
  "message": "404 Group Not Found"
}

but you can list groups, and this will include the top-level group(!!!!)

$ curl -kH "private-token: glpat-..." https://gdk.devvm:3443/api/v4/groups | jq .                                                                                              
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current                                                                                                                 
                                 Dload  Upload   Total   Spent    Left  Speed                                                                                                                   
100  1510  100  1510    0     0   1732      0 --:--:-- --:--:-- --:--:--  1731                                                                                                                  
[                                                                                                                                                                                               
  {                                                                                                                                                                                             
    "id": 97,  
...
  },
  {
    "id": 96,
...
}
]

Same happens for projects:

$ curl -kH "private-token: glpat-...." https://gdk.devvm:3443/api/v4/projects/19/groups | jq .
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   338  100   338    0     0    236      0  0:00:01  0:00:01 --:--:--   236
[
  {
    "id": 97,
    "web_url": "https://gdk.devvm:3443/groups/private-tl/private-sg",
    "name": "private-sg",
    "avatar_url": null,
    "full_name": "private-tl / private-sg",
    "full_path": "private-tl/private-sg"
  },
  {
    "id": 96,
    "web_url": "https://gdk.devvm:3443/groups/private-tl",
    "name": "private-tl",
    "avatar_url": null,
    "full_name": "private-tl",
    "full_path": "private-tl"
  }
]

So there is an information assymetry in Gitlab API. According to the discussion here this is just an edge-case and even the indirect queries should not work.

In theory we could use the symbolic names for groups that we could simply extract from the full_path attribute, but this is a Pandora box I am unwilling to open. Numeric IDs are stable even in case of renaming, symbolic names are not and projects/groups may move. I am afraid of edge-cases in our authentication and rate-limiting, hence I implemented a workaround for now until we come up with a better approach.

FYSA: @ankitbhatnagar @arun.sori @joe-shaw @mappelman

Tested using devvm

Edited by Pawel Rozlach

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Pawel Rozlach marked this merge request as draft

    marked this merge request as draft

  • Pawel Rozlach changed the description

    changed the description

  • Pawel Rozlach changed the description

    changed the description

  • Pawel Rozlach marked this merge request as ready

    marked this merge request as ready

  • Pawel Rozlach requested review from @joe-shaw

    requested review from @joe-shaw

  • Pawel Rozlach changed the description

    changed the description

  • Pawel Rozlach changed the description

    changed the description

    • Resolved by Pawel Rozlach

      I'm thinking, given the prior implementation did just split full_path you can do the same here without opening "pandoras box". It's already open, in that case, and it will avoid potentially many expensive API calls to GitLab listing all the groups.

  • Joe Shaw
  • Pawel Rozlach added 4 commits

    added 4 commits

    • 084ca72a...110e32f3 - 2 commits from branch main
    • 61e0ae37 - Switch top-level-ns codepath from quering namespace endpoint to quering group endpoint
    • b53f5660 - Add pagination, limit only to top-level groups

    Compare with previous version

  • Pawel Rozlach added 1 commit

    added 1 commit

    • 662b7895 - Add pagination, limit only to top-level groups

    Compare with previous version

  • Joe Shaw
  • Pawel Rozlach added 1 commit

    added 1 commit

    • 9cf56ad6 - Remove AllAvailable attribute

    Compare with previous version

  • Pawel Rozlach resolved all threads

    resolved all threads

  • Joe Shaw approved this merge request

    approved this merge request

  • Joe Shaw enabled an automatic merge when the pipeline for 9cf56ad6 succeeds

    enabled an automatic merge when the pipeline for 9cf56ad6 succeeds

  • merged

  • Joe Shaw mentioned in commit a5498f72

    mentioned in commit a5498f72

  • Pawel Rozlach mentioned in issue #2387

    mentioned in issue #2387

  • Pawel Rozlach resolved all threads

    resolved all threads

  • Please register or sign in to reply
    Loading