fix: ensure user admins can always see users table #15226
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,3 +1,5 @@ | ||||||
| import type { AuthorizationCheck } from "api/typesGenerated"; | ||||||
|
|
||||||
| export const checks = { | ||||||
| viewAllUsers: "viewAllUsers", | ||||||
| updateUsers: "updateUsers", | ||||||
|
|
@@ -11,13 +13,17 @@ export const checks = { | |||||
| viewUpdateCheck: "viewUpdateCheck", | ||||||
| viewExternalAuthConfig: "viewExternalAuthConfig", | ||||||
| viewDeploymentStats: "viewDeploymentStats", | ||||||
| readWorkspaceProxies: "readWorkspaceProxies", | ||||||
| editWorkspaceProxies: "editWorkspaceProxies", | ||||||
| createOrganization: "createOrganization", | ||||||
| editAnyOrganization: "editAnyOrganization", | ||||||
| viewAnyGroup: "viewAnyGroup", | ||||||
| createGroup: "createGroup", | ||||||
| viewAllLicenses: "viewAllLicenses", | ||||||
| viewNotificationTemplate: "viewNotificationTemplate", | ||||||
| } as const satisfies Record<string, string>; | ||||||
|
|
||||||
| type PermissionType = keyof typeof checks; | ||||||
|
There was a problem hiding this comment. this is technically incorrect, and only working because all of the key value pairs are symmetrical. what you really want is...
Suggested change
|
||||||
|
|
||||||
| export const permissionsToCheck = { | ||||||
| [checks.viewAllUsers]: { | ||||||
|
|
@@ -94,6 +100,12 @@ export const permissionsToCheck = { | |||||
| }, | ||||||
| action: "read", | ||||||
| }, | ||||||
| [checks.readWorkspaceProxies]: { | ||||||
| object: { | ||||||
| resource_type: "workspace_proxy", | ||||||
| }, | ||||||
| action: "read", | ||||||
| }, | ||||||
| [checks.editWorkspaceProxies]: { | ||||||
| object: { | ||||||
| resource_type: "workspace_proxy", | ||||||
|
|
@@ -116,7 +128,6 @@ export const permissionsToCheck = { | |||||
| [checks.viewAnyGroup]: { | ||||||
| object: { | ||||||
| resource_type: "group", | ||||||
|
There was a problem hiding this comment. Ended up deleting this because the property wasn't compatible. It also didn't seem to be used anywhere @Emyrk Not sure if we should update the types so that this can be added back There was a problem hiding this comment. Can you delete this unused check then? There was a problem hiding this comment. Oh, sorry – I meant that the |
||||||
| }, | ||||||
| action: "read", | ||||||
| }, | ||||||
|
|
@@ -132,6 +143,12 @@ export const permissionsToCheck = { | |||||
| }, | ||||||
| action: "read", | ||||||
| }, | ||||||
| [checks.viewNotificationTemplate]: { | ||||||
| object: { | ||||||
| resource_type: "notification_template", | ||||||
| }, | ||||||
| action: "read", | ||||||
| }, | ||||||
| } as const satisfies Record<PermissionType, AuthorizationCheck>; | ||||||
|
|
||||||
| export type Permissions = Record<PermissionType, boolean>; | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,188 @@ | ||
| import { Permission } from "api/typesGenerated"; | ||
| import type { Permissions } from "contexts/auth/permissions"; | ||
| import { MockNoPermissions, MockPermissions } from "testHelpers/entities"; | ||
| import { isManagementRoutePermitted } from "./ManagementSettingsLayout"; | ||
|
|
||
| describe(isManagementRoutePermitted.name, () => { | ||
| describe("General behavior", () => { | ||
| it("Rejects malformed routes", () => { | ||
| const invalidRoutes: readonly string[] = [ | ||
| // It's expected that the hostname will be stripped off | ||
| "https://dev.coder.com/deployment/licenses/add", | ||
|
|
||
| // Missing the leading / | ||
| "organizations", | ||
| ]; | ||
|
|
||
| for (const r of invalidRoutes) { | ||
| } | ||
|
|
||
| // Currently only checking whether route ends with / | ||
| const invalidRoute = "organizations"; | ||
| expect( | ||
| isManagementRoutePermitted(invalidRoute, MockPermissions, true), | ||
| ).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Organization routes", () => { | ||
| it("Delegates to showOrganizations value for any /organizations routes", () => { | ||
| expect( | ||
| isManagementRoutePermitted("/organizations", MockNoPermissions, true), | ||
| ).toBe(true); | ||
|
|
||
| expect( | ||
| isManagementRoutePermitted( | ||
| "/organizations/nested/arbitrarily", | ||
| MockNoPermissions, | ||
| true, | ||
| ), | ||
| ).toBe(true); | ||
|
|
||
| expect( | ||
| isManagementRoutePermitted( | ||
| "/organizations/sample-organization", | ||
| MockNoPermissions, | ||
| true, | ||
| ), | ||
| ).toBe(true); | ||
|
|
||
| expect( | ||
| isManagementRoutePermitted("/organizations", MockPermissions, false), | ||
| ).toBe(false); | ||
| }); | ||
| }); | ||
|
|
||
| describe("Deployment routes", () => { | ||
| it("Will never let the user through if they have no active permissions", () => { | ||
| let result = isManagementRoutePermitted( | ||
| "/deployment", | ||
| MockNoPermissions, | ||
| true, | ||
| ); | ||
| expect(result).toBe(false); | ||
|
|
||
| result = isManagementRoutePermitted( | ||
| "/deployment/licenses", | ||
| MockNoPermissions, | ||
| true, | ||
| ); | ||
| expect(result).toBe(false); | ||
|
|
||
| result = isManagementRoutePermitted( | ||
| "/deployment/appearance", | ||
| MockNoPermissions, | ||
| false, | ||
| ); | ||
| expect(result).toBe(false); | ||
| }); | ||
|
|
||
| it("Will let users access base /deployment route if they have at least one permission", () => { | ||
| const mocks: readonly Permissions[] = [ | ||
| MockPermissions, | ||
| { | ||
| ...MockNoPermissions, | ||
| createGroup: true, | ||
| }, | ||
| { | ||
| ...MockNoPermissions, | ||
| createOrganization: true, | ||
| deleteTemplates: true, | ||
| }, | ||
| { | ||
| ...MockNoPermissions, | ||
| editDeploymentValues: true, | ||
| viewNotificationTemplate: true, | ||
| readWorkspaceProxies: true, | ||
| }, | ||
| ]; | ||
|
|
||
| for (const m of mocks) { | ||
| expect(isManagementRoutePermitted("/deployment", m, true)).toBe(true); | ||
| expect(isManagementRoutePermitted("/deployment", m, false)).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| it("Rejects unknown deployment routes", () => { | ||
| const sampleRoutes: readonly string[] = [ | ||
| "/deployment/definitely-not-right", | ||
| "/deployment/what-is-this", | ||
| ]; | ||
|
|
||
| for (const r of sampleRoutes) { | ||
| const result = isManagementRoutePermitted(r, MockPermissions, true); | ||
| expect(result).toBe(false); | ||
| } | ||
| }); | ||
|
|
||
| it("Supports deployment routes that are nested more than one level", () => { | ||
| const routes: readonly string[] = [ | ||
| "/deployment/licenses/add", | ||
|
|
||
| // Including oauth2-provider routes, even though they're not | ||
| // currently exposed via the UI | ||
| "/deployment/oauth2-provider/apps", | ||
| "/deployment/oauth2-provider/apps/add", | ||
| ]; | ||
|
|
||
| for (const r of routes) { | ||
| let result = isManagementRoutePermitted(r, MockPermissions, true); | ||
| expect(result).toBe(true); | ||
|
|
||
| result = isManagementRoutePermitted(r, MockPermissions, false); | ||
| expect(result).toBe(true); | ||
| } | ||
| }); | ||
|
|
||
| it("Granularly associates individual deployment routes with specific permissions", () => { | ||
| type PairTuple = readonly [ | ||
| route: `/${string}`, | ||
| permissionKey: keyof Permissions, | ||
| ]; | ||
|
|
||
| const pairs: readonly PairTuple[] = [ | ||
| ["/general", "viewDeploymentValues"], | ||
| ["/licenses", "viewAllLicenses"], | ||
| ["/appearance", "editDeploymentValues"], | ||
| ["/userauth", "viewDeploymentValues"], | ||
| ["/external-auth", "viewDeploymentValues"], | ||
| ["/network", "viewDeploymentValues"], | ||
| ["/workspace-proxies", "readWorkspaceProxies"], | ||
| ["/security", "viewDeploymentValues"], | ||
| ["/observability", "viewDeploymentValues"], | ||
| ["/users", "viewAllUsers"], | ||
| ["/notifications", "viewNotificationTemplate"], | ||
| ["/oauth2-provider", "viewExternalAuthConfig"], | ||
| ]; | ||
|
|
||
| for (const [route, permName] of pairs) { | ||
| // Using NoPermissions version as baseline to make sure that we | ||
| // don't get false positives from all permissions being set to | ||
| // true at the start | ||
| const mutablePermsCopy = { ...MockNoPermissions }; | ||
| const fullRoute = `/deployment${route}`; | ||
|
|
||
| mutablePermsCopy[permName] = true; | ||
| let result = isManagementRoutePermitted( | ||
| fullRoute, | ||
| mutablePermsCopy, | ||
| true, | ||
| ); | ||
| expect(result).toBe(true); | ||
|
|
||
| result = isManagementRoutePermitted( | ||
| `${fullRoute}/example-subpath`, | ||
| mutablePermsCopy, | ||
| true, | ||
| ); | ||
| expect(result).toBe(true); | ||
|
|
||
| mutablePermsCopy[permName] = false; | ||
| result = isManagementRoutePermitted(fullRoute, mutablePermsCopy, true); | ||
| expect(result).toBe(false); | ||
| } | ||
|
|
||
| expect.hasAssertions(); | ||
| }); | ||
| }); | ||
| }); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could even take this a step further and just say...