ui: Correct responses for restricted access to non-admin users by koorosh · Pull Request #50869 · cockroachdb/cockroach

koorosh · 2020-07-01T14:16:12Z

Depends on cockroachdb/admin-ui-components#37
Depends on cockroachdb/yarn-vendored#44

Previously, request handlers for node locations and eventslogs didn't
check if the user has an admin role or not and proceeds with an attempt to query
data from tables with granted permissions for admin users only. This
error handled as server error then.

Current change validates if a user has admin role before requesting
data from tables and responds with Permission denied error when user
doesn't have an admin role.

UI changes:

Add InlineAlert component to display redesigned messages;

Screens
Single info message (Restricted permissions)

Single error message

Multiple error messages

cockroach-teamcity · 2020-07-01T14:16:21Z

This change is

dhartunian · 2020-07-24T23:41:30Z

pkg/server/admin.go

 	}
-
+	if !isAdmin {
+		return nil, errInsufficientPrivilege


Wondering if the better thing to do in these cases is find a good way to pass the SQL privilege error through to the UI. Otherwise we get stuck "double checking" all these instances which is both error prone and confusing.

Did you explore that option @koorosh? I think sticking to SQL as "the truth" as much as possible will be good here because the permissions model will change in the future but as long as we use the SQL-level error the UI will always be doing the right thing.

Good question. My concern is that we have role_members table with an explicit isAdmin field to indicate users' privilege. I thought it is the source of truth right now. Will it be changed in the future?

I agree that right we don't handle permission error in a generic way with other possible SQL errors but on the other hand, restricted access for querying tables isn't the only reason for permission errors (for instance, it could be restricted depending on context).

@dhartunian since last discussion, there was a change on server which introduced unified validation for admin permissions (2be60c1) so now it returns "this operation requires admin privilege" error message from server.

current:

This information is not available due to the current value of the 'server.remote_debugging.mode' setting.

message from server:

RequestError: this operation requires admin privilege

@Annebirzin , would it be okay to show server message instead of current one?

@koorosh yep I think this is actually more clear than the current message

dhartunian

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @elkmaster, and @koorosh)

pkg/server/admin.go, line 1501 at r3 (raw file):

Previously, Annebirzin (Anne Birzin) wrote…

@koorosh yep I think this is actually more clear than the current message

Sounds good @koorosh, use the new requireAdminUser function in these places.

dhartunian

Reviewed 10 of 10 files at r9, 2 of 2 files at r10, 3 of 3 files at r11, 2 of 2 files at r12, 5 of 5 files at r13.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @elkmaster, and @koorosh)

pkg/ui/src/views/cluster/containers/events/index.tsx, line 181 at r10 (raw file):

          loading={!events}
          error={lastError}
          render={this.renderContent}

I think renderContent needs to be bound to this in this component, otherwise the Events page fails to render. Or change renderContent so it doesn't depend on this.props. I wasn't able to get it to render without doing that.

pkg/ui/src/views/shared/components/loading/loading.spec.tsx, line 19 at r11 (raw file):

import Loading from "src/views/shared/components/loading";
import { ReactWrapper, mount } from "enzyme";
import { InlineAlert } from "oss/src/components/inlineAlert/inlineAlert";

remove oss

koorosh · 2020-11-24T14:40:54Z

Updated PR with fix for Events page and also updated imports to use direct imports from @cockroachlabs/admin-ui-components

dhartunian

Does this need a version bump for admin-ui-components to pick up latest Loading version?

Reviewed 22 of 22 files at r14.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @elkmaster, and @koorosh)

koorosh · 2020-11-25T10:48:15Z

Does this need a version bump for admin-ui-components to pick up latest Loading version?

Correct, bumped admin-ui-components to the 0.1.24 version (cockroachdb/admin-ui-components#37)

dhartunian · 2020-11-25T17:30:24Z

@koorosh now there's a conflict w/ yarn-vendor can you give that an update as well since we've merged other updates recently.

Current change reuses `requireAdminUser` function to validate whether user has admin role to request resources or not for Events endpoint. In addition, removed unnecessary validation for admin role on Locations endpoint. This endpoint can be accessed by users without admin role as well. Release note: none

Previously, request handlers for node locations and eventslogs didn't check if user has admin role or not and proceeds with attempt to query data from tables with granted permissions for admin users only. This error handled as server error then. Current change validates if user has admin role before requesting data from tables and responds with Permission denied error when user doesn't have admin role. Release note (admin ui change): Show Permission denied error on Node Map and Events views for non-admin users.

Loading component is responsible to provide feedback for user whether data is ready, is loading or failed to load with some error. Previously, when loading failed, error message was rendered inside of Loading component. Now, rendering of errors are delegated to InlineAlert component. Loading component is responsible to properly map error data to InlineAlert props. And it has to take into account next cases: - single error - multiple errors with the same intent - multiple errors with different intent - Restricted permissions error is handled as special case and has to be rendered as "info" alert. Release note (admin ui change): Redesign inline error alerts on pages. Alerts that shown when user has insufficient rights to see some resources (for instance non admin user on Databases page).

Initially, Admin UI messages for restricted permissions were custom and didn't provide enough details. Now, client consumes and decodes server Response errors and displays error messages provided by server. Release note (ui change): Show error messages provided by server instead of custom generic messages defined on client side. For instance, messages about permission restrictions to show pages for non-admin roles

Currently, Loading component has been defined in two projects: in pkg/ui and admin-ui-components which caused code duplication. admin-ui-components package has updated component styles so it is reused in Admin UI. To avoid extra changes across entire project, Loading component is re-exported from its initial location to preserve the same import paths where it is used. Release note (ui change): display error messages provided by server in alert messages (for instance when permissions denied to display some page).

dhartunian

nice work on this!

Reviewed 29 of 29 files at r15, 1 of 2 files at r16.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Annebirzin, @dhartunian, @elkmaster, and @koorosh)

koorosh · 2020-12-01T11:28:14Z

bors r+

craig · 2020-12-01T12:04:02Z

Build succeeded:

GitHub CI (Cockroach)

koorosh added the do-not-merge bors won't merge a PR with this label. label Jul 1, 2020

koorosh requested a review from a team July 1, 2020 14:16

koorosh changed the title ~~WIP. ui: Correct responses for restricted access to Localities and Events for non-admin users~~ WIP. ui: Correct responses for restricted access to non-admin users Jul 2, 2020

koorosh requested a review from vladlos July 3, 2020 10:29

koorosh removed the do-not-merge bors won't merge a PR with this label. label Jul 3, 2020

koorosh changed the title ~~WIP. ui: Correct responses for restricted access to non-admin users~~ ui: Correct responses for restricted access to non-admin users Jul 3, 2020

dhartunian requested changes Jul 24, 2020

View reviewed changes

dhartunian reviewed Oct 28, 2020

View reviewed changes

koorosh force-pushed the ui-permission-errors-handling branch from 7d6fb82 to f03fe4a Compare November 4, 2020 14:39

koorosh requested a review from a team as a code owner November 4, 2020 14:39

koorosh requested a review from dhartunian November 4, 2020 14:40

koorosh force-pushed the ui-permission-errors-handling branch 2 times, most recently from 31c9bdc to b1ed0a1 Compare November 13, 2020 17:15

koorosh added the backport-20.2.x label Nov 17, 2020

dhartunian requested changes Nov 23, 2020

View reviewed changes

koorosh force-pushed the ui-permission-errors-handling branch from b1ed0a1 to 357cc4a Compare November 24, 2020 14:38

dhartunian reviewed Nov 24, 2020

View reviewed changes

koorosh force-pushed the ui-permission-errors-handling branch from 357cc4a to 93df8ed Compare November 25, 2020 10:43

koorosh added 4 commits November 26, 2020 11:00

koorosh force-pushed the ui-permission-errors-handling branch from 93df8ed to 07da94c Compare November 26, 2020 09:02

koorosh force-pushed the ui-permission-errors-handling branch from 07da94c to fd16474 Compare November 27, 2020 12:51

koorosh requested a review from dhartunian November 27, 2020 13:37

dhartunian approved these changes Nov 30, 2020

View reviewed changes

craig bot merged commit c5c4165 into cockroachdb:master Dec 1, 2020

rafiss added this to the 20.2 milestone Apr 22, 2021

nkodali mentioned this pull request Dec 22, 2022

ui: node map produces 500 error with unprivileged user #50360

Closed

Conversation

koorosh commented Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jul 1, 2020

Uh oh!

dhartunian Jul 24, 2020

Choose a reason for hiding this comment

Uh oh!

koorosh Aug 11, 2020

Choose a reason for hiding this comment

Uh oh!

koorosh Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

Annebirzin Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

koorosh commented Nov 24, 2020

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

koorosh commented Nov 25, 2020

Uh oh!

dhartunian commented Nov 25, 2020

Uh oh!

dhartunian left a comment

Choose a reason for hiding this comment

Uh oh!

koorosh commented Dec 1, 2020

Uh oh!

craig bot commented Dec 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

koorosh commented Jul 1, 2020 •

edited

Loading