From 6fab0ad05af42be4989b63eb2979094b23666962 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 6 Dec 2023 13:35:48 +0100 Subject: [PATCH] graph/users: Fix http status codes for unprivileged requests Neither 'BadRequest' (as expected in the unit test) nor 'Unauthorized' (as expected in the API tests) seem correct here. We're no returning 'Forbidden' when an unprivileged users issues a GetUsers request that it is not allowed to perform. --- services/graph/pkg/service/v0/users.go | 4 ++-- services/graph/pkg/service/v0/users_test.go | 4 ++-- tests/acceptance/features/apiGraph/getUser.feature | 6 +++--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 5e346fea9..d048b7d9f 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -226,14 +226,14 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) { if !ctxHasFullPerms && (odataReq.Query == nil || odataReq.Query.Search == nil || len(odataReq.Query.Search.RawValue) < g.config.API.IdentitySearchMinLength) { // for regular user the search term must have a minimum length logger.Debug().Interface("query", r.URL.Query()).Msgf("search with less than %d chars for a regular user", g.config.API.IdentitySearchMinLength) - errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "search term too short") + errorcode.AccessDenied.Render(w, r, http.StatusForbidden, "search term too short") return } if !ctxHasFullPerms && (odataReq.Query.Filter != nil || odataReq.Query.Apply != nil || odataReq.Query.Expand != nil || odataReq.Query.Compute != nil) { // regular users can't use filter, apply, expand and compute logger.Debug().Interface("query", r.URL.Query()).Msg("forbidden query elements for a regular user") - errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "query has forbidden elements for regular users") + errorcode.AccessDenied.Render(w, r, http.StatusForbidden, "query has forbidden elements for regular users") return } diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index c021dcebe..4cc9561e2 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -269,14 +269,14 @@ var _ = Describe("Users", func() { r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) svc.GetUsers(rr, r) - Expect(rr.Code).To(Equal(http.StatusBadRequest)) + Expect(rr.Code).To(Equal(http.StatusForbidden)) }) It("denies using to short search terms for unprivileged users", func() { permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=a", nil) svc.GetUsers(rr, r) - Expect(rr.Code).To(Equal(http.StatusBadRequest)) + Expect(rr.Code).To(Equal(http.StatusForbidden)) }) It("only returns a restricted set of attributes for unprivileged users", func() { permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) diff --git a/tests/acceptance/features/apiGraph/getUser.feature b/tests/acceptance/features/apiGraph/getUser.feature index e9f2cd0d0..94a1e6e52 100644 --- a/tests/acceptance/features/apiGraph/getUser.feature +++ b/tests/acceptance/features/apiGraph/getUser.feature @@ -229,7 +229,7 @@ Feature: get users Scenario Outline: non-admin user tries to get all users Given the administrator has assigned the role "" to user "Alice" using the Graph API When user "Brian" tries to get all users using the Graph API - Then the HTTP status code should be "401" + Then the HTTP status code should be "403" And the JSON data of the response should match """ { @@ -831,7 +831,7 @@ Feature: get users And group "tea-lover" has been created And user "Alice" has been added to group "tea-lover" When the user "Brian" gets all users of the group "tea-lover" using the Graph API - Then the HTTP status code should be "401" + Then the HTTP status code should be "403" And the JSON data of the response should match """ { @@ -968,7 +968,7 @@ Feature: get users Scenario Outline: non-admin user tries to get users with a certain role Given the administrator has assigned the role "" to user "Alice" using the Graph API When the user "Alice" gets all users with role "" using the Graph API - Then the HTTP status code should be "401" + Then the HTTP status code should be "403" And the JSON data of the response should match """ {