Return 404 from the SAML group links API for a missing group

What does this MR do and why?

The four SAML group links routes answer 401 Unauthorized when the group in the path does not exist, for example after it was deleted. As the issue reports, a client reads that as a problem with its token: the GitLab Terraform provider fails its plan instead of noticing that the group, and with it the link, is gone.

The cause is the lookup @dblessing pointed at in the issue thread. Each route called find_group(params[:id]), which returns nil for a group that does not exist, and then unauthorized! unless can?(current_user, :admin_saml_group_links, group), which refuses every nil group. This MR makes the four routes call find_group! instead, the lookup the user_group helper and most group routes use. It answers 404 Group Not Found for a group that does not exist, and in the same words for a group the caller cannot see. The permission check after it is unchanged. find_group! also applies the organization maintenance check to the group, so while the group's organization is under maintenance these routes now refuse with the maintenance message (403, or 503 with Retry-After), as the group routes that already use it do.

Request Before After
Group ID or path that matches no group 401 Unauthorized 404 Group Not Found
Private group the caller is not a member of 401 Unauthorized 404 Group Not Found
Group the caller can see but whose SAML group links it may not administer (a Developer of the group, or a group without SAML group sync) 401 Unauthorized 401 Unauthorized (unchanged)
Existing group, link name that matches no link 404 Not Found 404 Not Found (unchanged)

On the security question raised in the thread: the answer still does not tell a caller whether a group it cannot see exists. Before, a missing group and a private group the caller cannot see both answered 401; now both answer the same 404. Only a group the caller can already see gets the 401, as before.

On the status code policy: the API style guide counts changing a status code other than 500 as a breaking change. I read this one as a bug fix rather than a change of contract. The 401 for a missing group was never documented: the desc blocks of the add, get and delete routes declare 404 as a failure, the OpenAPI document lists 404 and no 401 for all four routes, and the issue, a bug marked ready for development, asks for the 404. A fine-grained personal access token already gets 404 Not Found from all four routes for a group that does not exist, because the lookup of its boundary group fails before the route runs; this MR gives every other caller a 404 as well. The permission refusal keeps its 401 for the same policy reason, and !260774 (merged) documented that some endpoints answer a missing permission with 401. If you would rather treat the missing group as a breaking change too, I am happy to follow whatever the API reviewers prefer.

With this change the Terraform provider handles a deleted group on its own: resourceGitlabGroupSamlLinkRead in internal/provider/sdk/resource_gitlab_group_saml_link.go removes the link from the state when GitLab answers 404 (read at terraform-provider-gitlab@6cbee813).

The commits:

  1. Return 404 from the SAML group links API for a missing group: the four lookups, and request specs for each route with a group ID that matches no group, a path that matches no group, and a private group the caller is not a member of. Each asserts the 404, its message, and that no SAML group link was created or removed. It carries the Changelog: fixed and EE: true trailers and describes the whole change, so it is the right message if the MR is squashed.
  2. Cover the permission refusal of reading one SAML group link: reading a single link was the one route without a spec for a signed-in user who may not administer the links (its 401 spec sends no user, which authenticate! refuses first). This pins that a Developer of the group still gets 401 there.

I found this while handling GitLab's 401 answers in gitlab-mcp-server, which serves these four routes and has to tell its users whether a 401 means a bad token or a missing permission.

References

Closes #383531 (closed)

The analysis this follows: #383531 (comment 1188605186)

I left doc/api/saml.md alone because !261285 (merged) is editing that page. If you would like the 404 mentioned there, I can add it here or after !261285 (merged) merges.

Screenshots or screen recordings

Not a UI change. GET /api/v4/groups/<ID that matches no group>/saml_group_links as a signed-in user, from the request specs:

Before After
401 {"message":"401 Unauthorized"} 404 {"message":"404 Group Not Found"}
Specs I ran

All on master at b00391b1ec3, in EE, with bundle exec rspec ee/spec/requests/api/saml_group_links_spec.rb:

  • master with only the spec of commit 2 added: 74 examples, 0 failures.
  • The specs of commit 1 without its change to ee/lib/api/saml_group_links.rb: 85 examples, 12 failures, all of them the new examples, each expected the response to have status code :not_found but it was 401. The response was: {"message":"401 Unauthorized"}.
  • Commit 1: 85 examples, 0 failures.
  • Commit 2 (the branch): 86 examples, 0 failures.
  • RuboCop on the Ruby files of each commit: no offenses. Both commit message linters (scripts/lint/commit_linter.rb and the Danger one) on each commit: no problems. git diff --check: clean.
  • A throwaway request spec, not part of this MR, calling the four routes for a group ID that matches no group with a fine-grained personal access token (create(:granular_pat, ...) holding the route's permission) and the list route with a classic one. With ee/lib/api/saml_group_links.rb from master: the fine-grained token got 404 {"message":"404 Not Found"} on all four, the classic token 401 {"message":"401 Unauthorized"}. With this MR: the fine-grained token still gets 404 {"message":"404 Not Found"}, the classic token 404 {"message":"404 Group Not Found"}.

How to set up and validate locally

  1. On a Premium or Ultimate instance, create a top-level group, enable SAML SSO for it, and add a SAML group link under Settings > SAML Group Links.
  2. Note the group ID, then delete the group (or use any ID that matches no group).
  3. Call curl --header "PRIVATE-TOKEN: <token>" "http://gdk.test:3000/api/v4/groups/<group id>/saml_group_links".
  4. Before this MR the response is 401 {"message":"401 Unauthorized"}; with it, 404 {"message":"404 Group Not Found"}. The same holds for GET and DELETE /groups/<group id>/saml_group_links/<name> and for POST /groups/<group id>/saml_group_links.
  5. As a Developer of an existing group with SAML enabled, the same calls still answer 401 Unauthorized.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Merge request reports

Loading
Loading