From 78202cfb255a1411b4cb66853263c19f30d7e410 Mon Sep 17 00:00:00 2001 From: Prabhat Khera <91852476+prabhat-org@users.noreply.github.com> Date: Mon, 25 Sep 2023 09:08:37 +1300 Subject: [PATCH] fix(permissions): non admin access to view users [EE-5825] (#10353) * fix(security): added restrictions to see user names [EE-5825] --- api/http/handler/users/user_list.go | 38 ++++++++++++++++--- api/http/handler/users/user_list_test.go | 18 +-------- .../AccessControlPaneDetails.test.tsx | 7 ++-- .../AccessControlPanelDetails.tsx | 28 +++++++++++--- .../EditDetails/EditDetails.tsx | 6 +-- .../EditDetails/useLoadState.ts | 4 +- .../users/teams/ListView/ListView.tsx | 2 +- 7 files changed, 66 insertions(+), 37 deletions(-) diff --git a/api/http/handler/users/user_list.go b/api/http/handler/users/user_list.go index 4535dc73a..edc529b14 100644 --- a/api/http/handler/users/user_list.go +++ b/api/http/handler/users/user_list.go @@ -26,14 +26,18 @@ import ( // @failure 500 "Server error" // @router /users [get] func (handler *Handler) userList(w http.ResponseWriter, r *http.Request) *httperror.HandlerError { - users, err := handler.DataStore.User().ReadAll() + securityContext, err := security.RetrieveRestrictedRequestContext(r) if err != nil { - return httperror.InternalServerError("Unable to retrieve users from the database", err) + return httperror.InternalServerError("Unable to retrieve info from request context", err) } - securityContext, err := security.RetrieveRestrictedRequestContext(r) + if !securityContext.IsAdmin && !securityContext.IsTeamLeader { + return httperror.Forbidden("Permission denied to access users list", err) + } + + users, err := handler.DataStore.User().ReadAll() if err != nil { - return httperror.InternalServerError("Unable to retrieve info from request context", err) + return httperror.InternalServerError("Unable to retrieve users from the database", err) } availableUsers := security.FilterUsers(users, securityContext) @@ -43,7 +47,10 @@ func (handler *Handler) userList(w http.ResponseWriter, r *http.Request) *httper endpointID, _ := request.RetrieveNumericQueryParameter(r, "environmentId", true) if endpointID == 0 { - return response.JSON(w, availableUsers) + if securityContext.IsAdmin { + sanitizeUsers(users) + } + return response.JSON(w, users) } // filter out users who do not have access to the specific endpoint @@ -61,6 +68,9 @@ func (handler *Handler) userList(w http.ResponseWriter, r *http.Request) *httper for _, user := range availableUsers { // the users who have the endpoint authorization if _, ok := user.EndpointAuthorizations[endpoint.ID]; ok { + if securityContext.IsAdmin { + sanitizeUser(&user) + } canAccessEndpoint = append(canAccessEndpoint, user) continue } @@ -72,9 +82,27 @@ func (handler *Handler) userList(w http.ResponseWriter, r *http.Request) *httper } if security.AuthorizedEndpointAccess(endpoint, endpointGroup, user.ID, teamMemberships) { + if securityContext.IsAdmin { + sanitizeUser(&user) + } canAccessEndpoint = append(canAccessEndpoint, user) } } return response.JSON(w, canAccessEndpoint) } + +func sanitizeUser(user *portainer.User) { + user.Password = "" + user.EndpointAuthorizations = nil + user.ThemeSettings = portainer.UserThemeSettings{} + user.PortainerAuthorizations = nil + user.UserTheme = "" + user.TokenIssueAt = 0 +} + +func sanitizeUsers(users []portainer.User) { + for i := range users { + sanitizeUser(&users[i]) + } +} diff --git a/api/http/handler/users/user_list_test.go b/api/http/handler/users/user_list_test.go index 094c59530..261857c38 100644 --- a/api/http/handler/users/user_list_test.go +++ b/api/http/handler/users/user_list_test.go @@ -111,28 +111,14 @@ func Test_userList(t *testing.T) { } }) - t.Run("standard user cannot list amdin users", func(t *testing.T) { + t.Run("standard user cannot list users", func(t *testing.T) { req := httptest.NewRequest(http.MethodGet, "/users", nil) req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", jwt)) rr := httptest.NewRecorder() h.ServeHTTP(rr, req) - is.Equal(http.StatusOK, rr.Code) - - body, err := io.ReadAll(rr.Body) - is.NoError(err, "ReadAll should not return error") - - var resp []portainer.User - err = json.Unmarshal(body, &resp) - is.NoError(err, "response should be list json") - - is.Len(resp, 2) - if len(resp) > 0 { - for _, user := range resp { - is.NotEqual(portainer.AdministratorRole, user.Role) - } - } + is.Equal(http.StatusForbidden, rr.Code) }) // Case 2: the user is under an environment group and the environment group has endpoint access. diff --git a/app/react/portainer/access-control/AccessControlPanel/AccessControlPaneDetails.test.tsx b/app/react/portainer/access-control/AccessControlPanel/AccessControlPaneDetails.test.tsx index dd78df8af..9970da4d9 100644 --- a/app/react/portainer/access-control/AccessControlPanel/AccessControlPaneDetails.test.tsx +++ b/app/react/portainer/access-control/AccessControlPanel/AccessControlPaneDetails.test.tsx @@ -4,6 +4,7 @@ import { createMockTeams, createMockUsers } from '@/react-tools/test-mocks'; import { renderWithQueryClient } from '@/react-tools/test-utils'; import { rest, server } from '@/setup-tests/server'; import { Role } from '@/portainer/users/types'; +import { withUserProvider } from '@/react/test-utils/withUserProvider'; import { ResourceControlOwnership, @@ -143,11 +144,9 @@ async function renderComponent( resourceType: ResourceControlType = ResourceControlType.Container, resourceControl?: ResourceControlViewModel ) { + const WithUser = withUserProvider(AccessControlPanelDetails); const queries = renderWithQueryClient( - + ); await expect(queries.findByText('Ownership')).resolves.toBeVisible(); diff --git a/app/react/portainer/access-control/AccessControlPanel/AccessControlPanelDetails.tsx b/app/react/portainer/access-control/AccessControlPanel/AccessControlPanelDetails.tsx index 992da5bbd..3201959cc 100644 --- a/app/react/portainer/access-control/AccessControlPanel/AccessControlPanelDetails.tsx +++ b/app/react/portainer/access-control/AccessControlPanel/AccessControlPanelDetails.tsx @@ -8,6 +8,7 @@ import { UserId } from '@/portainer/users/types'; import { TeamId } from '@/react/portainer/users/teams/types'; import { useTeams } from '@/react/portainer/users/teams/queries'; import { useUsers } from '@/portainer/users/queries'; +import { pluralize } from '@/portainer/helpers/strings'; import { Link } from '@@/Link'; import { Tooltip } from '@@/Tip/Tooltip'; @@ -43,6 +44,25 @@ export function AccessControlPanelDetails({ const users = useAuthorizedUsers(restrictedToUsers.map((ra) => ra.UserId)); const teams = useAuthorizedTeams(restrictedToTeams.map((ra) => ra.TeamId)); + const teamsLength = teams.data ? teams.data.length : 0; + const unauthoisedTeams = restrictedToTeams.length - teamsLength; + + let teamsMessage = teams.data && teams.data.join(', '); + if (unauthoisedTeams > 0 && teams.isFetched) { + teamsMessage += teamsLength > 0 ? ' and' : ''; + teamsMessage += ` ${unauthoisedTeams} ${pluralize( + unauthoisedTeams, + 'team' + )} you are not part of`; + } + + const userMessage = users.data + ? users.data.join(', ') + : `${restrictedToUsers.length} ${pluralize( + restrictedToUsers.length, + 'user' + )}`; + return ( @@ -62,17 +82,13 @@ export function AccessControlPanelDetails({ {restrictedToUsers.length > 0 && ( - + )} {restrictedToTeams.length > 0 && ( - + )} diff --git a/app/react/portainer/access-control/EditDetails/EditDetails.tsx b/app/react/portainer/access-control/EditDetails/EditDetails.tsx index bd3b959ce..3c2e108de 100644 --- a/app/react/portainer/access-control/EditDetails/EditDetails.tsx +++ b/app/react/portainer/access-control/EditDetails/EditDetails.tsx @@ -32,7 +32,7 @@ export function EditDetails({ }: Props) { const { user, isAdmin } = useUser(); - const { users, teams, isLoading } = useLoadState(environmentId); + const { users, teams, isLoading } = useLoadState(environmentId, isAdmin); const handleChange = useCallback( (partialValues: Partial) => { onChange({ ...values, ...partialValues }); @@ -41,7 +41,7 @@ export function EditDetails({ [values, onChange] ); - if (isLoading || !teams || !users) { + if (isLoading || !teams || (isAdmin && !users) || !values.authorizedUsers) { return null; } @@ -61,7 +61,7 @@ export function EditDetails({ {isAdmin && ( handleChange({ authorizedUsers })} value={values.authorizedUsers} errors={errors?.authorizedUsers} diff --git a/app/react/portainer/access-control/EditDetails/useLoadState.ts b/app/react/portainer/access-control/EditDetails/useLoadState.ts index 2af95d582..c713f27d6 100644 --- a/app/react/portainer/access-control/EditDetails/useLoadState.ts +++ b/app/react/portainer/access-control/EditDetails/useLoadState.ts @@ -2,9 +2,9 @@ import { useTeams } from '@/react/portainer/users/teams/queries'; import { useUsers } from '@/portainer/users/queries'; import { EnvironmentId } from '@/react/portainer/environments/types'; -export function useLoadState(environmentId?: EnvironmentId) { +export function useLoadState(environmentId?: EnvironmentId, enabled = true) { const teams = useTeams(false, environmentId); - const users = useUsers(false, environmentId); + const users = useUsers(false, environmentId, enabled); return { teams: teams.data, diff --git a/app/react/portainer/users/teams/ListView/ListView.tsx b/app/react/portainer/users/teams/ListView/ListView.tsx index 6c45119cf..01bc181bc 100644 --- a/app/react/portainer/users/teams/ListView/ListView.tsx +++ b/app/react/portainer/users/teams/ListView/ListView.tsx @@ -18,7 +18,7 @@ export function ListView() { <> - {usersQuery.data && teamsQuery.data && ( + {isAdmin && usersQuery.data && teamsQuery.data && ( )}
Authorized users - {users.data && users.data.join(', ')} - {userMessage}
Authorized teams - {teams.data && teams.data.join(', ')} - {teamsMessage}