From f9114ef4be251c30bf6bba67a145de305b668136 Mon Sep 17 00:00:00 2001 From: Kiwi Date: Fri, 29 Mar 2019 19:55:32 +0100 Subject: [PATCH] [CORL-318] Overhaul Permissions Checks on the Client (#2246) * feat: overhaul admin permission checks * feat: overhaul stream permission checks --- src/core/client/admin/components/App.tsx | 7 +- .../admin/components/Navigation.spec.tsx | 2 +- .../client/admin/components/Navigation.tsx | 14 +- .../__snapshots__/App.spec.tsx.snap | 4 +- .../client/admin/containers/AppContainer.tsx | 1 + .../admin/containers/AuthCheckContainer.tsx | 21 ++- .../admin/containers/NavigationContainer.tsx | 38 +++++ .../containers/RedirectLoginContainer.tsx | 3 +- src/core/client/admin/permissions.tsx | 2 + src/core/client/admin/routeConfig.tsx | 52 ++++--- .../__snapshots__/restricted.spec.tsx.snap | 135 +++++++++++++++++- .../admin/test/auth/restricted.spec.tsx | 20 ++- .../admin/test/configure/permission.spec.tsx | 71 +++++++++ src/core/client/admin/test/fixtures.ts | 8 +- .../stream/containers/TabBarContainer.tsx | 4 +- .../client/stream/helpers/roleIsAtLeast.ts | 20 +-- .../stream/mutations/CreateCommentMutation.ts | 4 +- .../mutations/CreateCommentReplyMutation.ts | 4 +- src/core/client/stream/permissions.tsx | 32 +++++ src/core/client/stream/test/fixtures.ts | 11 +- 20 files changed, 388 insertions(+), 65 deletions(-) create mode 100644 src/core/client/admin/containers/NavigationContainer.tsx create mode 100644 src/core/client/admin/test/configure/permission.spec.tsx create mode 100644 src/core/client/stream/permissions.tsx diff --git a/src/core/client/admin/components/App.tsx b/src/core/client/admin/components/App.tsx index 300ba1932..9edc7ce4f 100644 --- a/src/core/client/admin/components/App.tsx +++ b/src/core/client/admin/components/App.tsx @@ -4,15 +4,16 @@ import { PropTypesOf } from "talk-framework/types"; import { Logo } from "talk-ui/components"; import { AppBar, Begin, Divider, End } from "talk-ui/components/AppBar"; +import NavigationContainer from "../containers/NavigationContainer"; import UserMenuContainer from "../containers/UserMenuContainer"; import DecisionHistoryButton from "./DecisionHistoryButton"; -import Navigation from "./Navigation"; import Version from "./Version"; import styles from "./App.css"; interface Props { - viewer: PropTypesOf["viewer"]; + viewer: PropTypesOf["viewer"] & + PropTypesOf["viewer"]; children: React.ReactNode; } @@ -24,7 +25,7 @@ const App: StatelessComponent = ({ children, viewer }) => ( - + diff --git a/src/core/client/admin/components/Navigation.spec.tsx b/src/core/client/admin/components/Navigation.spec.tsx index df82a1ccd..2f493507c 100644 --- a/src/core/client/admin/components/Navigation.spec.tsx +++ b/src/core/client/admin/components/Navigation.spec.tsx @@ -5,6 +5,6 @@ import Navigation from "./Navigation"; it("renders correctly", () => { const renderer = createRenderer(); - renderer.render(); + renderer.render(); expect(renderer.getRenderOutput()).toMatchSnapshot(); }); diff --git a/src/core/client/admin/components/Navigation.tsx b/src/core/client/admin/components/Navigation.tsx index 791c6c2ca..664de8c3d 100644 --- a/src/core/client/admin/components/Navigation.tsx +++ b/src/core/client/admin/components/Navigation.tsx @@ -5,7 +5,11 @@ import { AppBarNavigation } from "talk-ui/components"; import NavigationLink from "./NavigationLink"; -const Navigation: StatelessComponent = () => ( +interface Props { + showConfigure: boolean; +} + +const Navigation: StatelessComponent = props => ( Moderate @@ -16,9 +20,11 @@ const Navigation: StatelessComponent = () => ( Community - - Configure - + {props.showConfigure && ( + + Configure + + )} ); diff --git a/src/core/client/admin/components/__snapshots__/App.spec.tsx.snap b/src/core/client/admin/components/__snapshots__/App.spec.tsx.snap index 32a0a8023..ed3d3415d 100644 --- a/src/core/client/admin/components/__snapshots__/App.spec.tsx.snap +++ b/src/core/client/admin/components/__snapshots__/App.spec.tsx.snap @@ -17,7 +17,9 @@ exports[`renders correctly 1`] = ` - + diff --git a/src/core/client/admin/containers/AppContainer.tsx b/src/core/client/admin/containers/AppContainer.tsx index 7693b19da..a58fd1de3 100644 --- a/src/core/client/admin/containers/AppContainer.tsx +++ b/src/core/client/admin/containers/AppContainer.tsx @@ -26,6 +26,7 @@ const enhanced = withRouteConfig({ query AppContainerQuery { viewer { ...UserMenuContainer_viewer + ...NavigationContainer_viewer } } `, diff --git a/src/core/client/admin/containers/AuthCheckContainer.tsx b/src/core/client/admin/containers/AuthCheckContainer.tsx index 6af1c6c67..4d2b4ce0d 100644 --- a/src/core/client/admin/containers/AuthCheckContainer.tsx +++ b/src/core/client/admin/containers/AuthCheckContainer.tsx @@ -6,15 +6,25 @@ import { SetRedirectPathMutation, withSetRedirectPathMutation, } from "talk-admin/mutations"; +import { AbilityType, can } from "talk-admin/permissions"; import RestrictedContainer from "talk-admin/views/restricted/containers/RestrictedContainer"; import { graphql } from "talk-framework/lib/relay"; import { withRouteConfig } from "talk-framework/lib/router"; +import { GQLUSER_ROLE } from "talk-framework/schema"; interface Props { match: Match; router: Router; setRedirectPath: SetRedirectPathMutation; - data: AuthCheckContainerQueryResponse | null; + data: + | AuthCheckContainerQueryResponse & { + route: { + // An AbilityType can be passed in as the Route data + // to perform a permission check. + data?: AbilityType; + }; + } + | null; } class AuthCheckContainer extends React.Component { @@ -47,7 +57,14 @@ class AuthCheckContainer extends React.Component { settings: { auth }, } = props.data!; if (viewer) { - if (viewer.role === "COMMENTER") { + if ( + viewer.role === GQLUSER_ROLE.COMMENTER || + viewer.role === GQLUSER_ROLE.STAFF || + (props.data && + props.data.route.data && + // Perform permission check on the ability passed in by the route data + !can(viewer, props.data.route.data)) + ) { return false; } else if ( !viewer.email || diff --git a/src/core/client/admin/containers/NavigationContainer.tsx b/src/core/client/admin/containers/NavigationContainer.tsx new file mode 100644 index 000000000..cf32a2a35 --- /dev/null +++ b/src/core/client/admin/containers/NavigationContainer.tsx @@ -0,0 +1,38 @@ +import React from "react"; + +import { NavigationContainer_viewer as ViewerData } from "talk-admin/__generated__/NavigationContainer_viewer.graphql"; +import { Ability, can } from "talk-admin/permissions"; +import { graphql, withFragmentContainer } from "talk-framework/lib/relay"; +import { SignOutMutation, withSignOutMutation } from "talk-framework/mutations"; + +import Navigation from "../components/Navigation"; + +interface Props { + signOut: SignOutMutation; + viewer: ViewerData | null; +} + +class NavigationContainer extends React.Component { + public render() { + return ( + + ); + } +} + +const enhanced = withSignOutMutation( + withFragmentContainer({ + viewer: graphql` + fragment NavigationContainer_viewer on User { + role + } + `, + })(NavigationContainer) +); + +export default enhanced; diff --git a/src/core/client/admin/containers/RedirectLoginContainer.tsx b/src/core/client/admin/containers/RedirectLoginContainer.tsx index cb5166a45..7cf0b02f5 100644 --- a/src/core/client/admin/containers/RedirectLoginContainer.tsx +++ b/src/core/client/admin/containers/RedirectLoginContainer.tsx @@ -8,6 +8,7 @@ import { } from "talk-admin/mutations"; import { graphql } from "talk-framework/lib/relay"; import { withRouteConfig } from "talk-framework/lib/router"; +import { GQLUSER_ROLE } from "talk-framework/schema"; interface Props { match: Match; @@ -35,7 +36,7 @@ class RedirectLoginContainer extends React.Component { settings: { auth }, } = props.data!; if (viewer) { - if (viewer.role === "COMMENTER") { + if (viewer.role === GQLUSER_ROLE.COMMENTER) { return "/admin/login"; } else if ( !viewer.email || diff --git a/src/core/client/admin/permissions.tsx b/src/core/client/admin/permissions.tsx index af79f43bc..b1a7da259 100644 --- a/src/core/client/admin/permissions.tsx +++ b/src/core/client/admin/permissions.tsx @@ -13,6 +13,8 @@ import { GQLUSER_ROLE, GQLUSER_ROLE_RL } from "talk-framework/schema"; * the single point of truth. */ const permissionMap = { + // Mutation.updateSettings + CHANGE_CONFIGURATION: [GQLUSER_ROLE.ADMIN], // Mutation.updateUserRole CHANGE_ROLE: [GQLUSER_ROLE.ADMIN], // Mutation.openStory diff --git a/src/core/client/admin/routeConfig.tsx b/src/core/client/admin/routeConfig.tsx index 3ec46a4fd..871689a83 100644 --- a/src/core/client/admin/routeConfig.tsx +++ b/src/core/client/admin/routeConfig.tsx @@ -3,6 +3,7 @@ import React from "react"; import AppContainer from "./containers/AppContainer"; import AuthCheckContainer from "./containers/AuthCheckContainer"; +import { Ability } from "./permissions"; import CommunityContainer from "./routes/community/containers/CommunityContainer"; import ConfigureContainer from "./routes/configure/containers/ConfigureContainer"; import ConfigureAdvancedRouteContainer from "./routes/configure/sections/advanced/containers/AdvancedConfigRouteContainer"; @@ -45,29 +46,34 @@ export default makeRouteConfig( - - - - - - - - + + + + + + + + + + diff --git a/src/core/client/admin/test/auth/__snapshots__/restricted.spec.tsx.snap b/src/core/client/admin/test/auth/__snapshots__/restricted.spec.tsx.snap index a25455f8e..09ec4d183 100644 --- a/src/core/client/admin/test/auth/__snapshots__/restricted.spec.tsx.snap +++ b/src/core/client/admin/test/auth/__snapshots__/restricted.spec.tsx.snap @@ -1,6 +1,139 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`show restricted screen for commenters 1`] = ` +exports[`show restricted screen for commenters and staff 1`] = ` +
+
+
+
+
+ + + + + + + + + + + logo mark2018 + + + + + +
+
+
+

+ + Currently signed in to + +

+

+ Talk +

+
+
+
+
+ +
+

+ You do not have permission to access this page. +

+
+
+
+ You are signed in as: +

+ Markus +

+
+
+
+ +
+

+ If you think this is an error, please contact your administrator for assistance. +

+
+
+
+
+`; + +exports[`show restricted screen for commenters and staff 2`] = `
diff --git a/src/core/client/admin/test/auth/restricted.spec.tsx b/src/core/client/admin/test/auth/restricted.spec.tsx index d4e47f63f..19dd0e061 100644 --- a/src/core/client/admin/test/auth/restricted.spec.tsx +++ b/src/core/client/admin/test/auth/restricted.spec.tsx @@ -3,6 +3,7 @@ import sinon from "sinon"; import { TalkContext } from "talk-framework/lib/bootstrap"; import { LOCAL_ID } from "talk-framework/lib/relay"; +import { GQLUSER_ROLE } from "talk-framework/schema"; import { createAccessToken, replaceHistoryLocation, @@ -47,12 +48,15 @@ function createTestRenderer( return { testRenderer, context }; } -it("show restricted screen for commenters", async () => { - const { testRenderer } = createTestRenderer({ role: "COMMENTER" }); - const authBox = await waitForElement(() => - within(testRenderer.root).getByTestID("authBox") - ); - expect(within(authBox).toJSON()).toMatchSnapshot(); +it("show restricted screen for commenters and staff", async () => { + const restrictedRoles = [GQLUSER_ROLE.COMMENTER, GQLUSER_ROLE.STAFF]; + for (const role of restrictedRoles) { + const { testRenderer } = createTestRenderer({ role }); + const authBox = await waitForElement(() => + within(testRenderer.root).getByTestID("authBox") + ); + expect(within(authBox).toJSON()).toMatchSnapshot(); + } }); it("show restricted screen when email is not set", async () => { @@ -71,7 +75,9 @@ it("show restricted screen local was not set (password)", async () => { }); it("sign out when clicking on sign in as", async () => { - const { context, testRenderer } = createTestRenderer({ role: "COMMENTER" }); + const { context, testRenderer } = createTestRenderer({ + role: GQLUSER_ROLE.COMMENTER, + }); const authBox = await waitForElement(() => within(testRenderer.root).getByTestID("authBox") ); diff --git a/src/core/client/admin/test/configure/permission.spec.tsx b/src/core/client/admin/test/configure/permission.spec.tsx new file mode 100644 index 000000000..e73fc5ca2 --- /dev/null +++ b/src/core/client/admin/test/configure/permission.spec.tsx @@ -0,0 +1,71 @@ +import { get, merge } from "lodash"; +import sinon from "sinon"; + +import { GQLUSER_ROLE } from "talk-framework/schema"; +import { + replaceHistoryLocation, + waitForElement, + within, +} from "talk-framework/testHelpers"; + +import create from "../create"; +import { settings, users } from "../fixtures"; + +beforeEach(() => { + replaceHistoryLocation("http://localhost/admin/configure/general"); +}); + +const createTestRenderer = async ( + resolver: any = {}, + options: { muteNetworkErrors?: boolean } = {} +) => { + const resolvers = { + ...resolver, + Query: { + ...resolver.Query, + settings: sinon + .stub() + .returns(merge({}, settings, get(resolver, "Query.settings"))), + }, + }; + const { testRenderer } = create({ + // Set this to true, to see graphql responses. + logNetwork: false, + muteNetworkErrors: options.muteNetworkErrors, + resolvers, + initLocalState: localRecord => { + localRecord.setValue(true, "loggedIn"); + }, + }); + return { + testRenderer, + }; +}; + +it("denies access to moderators", async () => { + const deniedRoles = [GQLUSER_ROLE.MODERATOR]; + for (const r of deniedRoles) { + const { testRenderer } = await createTestRenderer({ + Query: { + viewer: sinon.stub().returns({ ...users[0], role: r }), + }, + }); + await waitForElement(() => + within(testRenderer.root).getByText("Sign in with a different account") + ); + } +}); + +it("allows access to admins", async () => { + const deniedRoles = [GQLUSER_ROLE.ADMIN]; + for (const r of deniedRoles) { + const { testRenderer } = await createTestRenderer({ + Query: { + viewer: sinon.stub().returns({ ...users[0], role: r }), + }, + }); + await waitForElement(() => + within(testRenderer.root).getByTestID("configure-container") + ); + } +}); diff --git a/src/core/client/admin/test/fixtures.ts b/src/core/client/admin/test/fixtures.ts index 9e793aa8f..90e2672cd 100644 --- a/src/core/client/admin/test/fixtures.ts +++ b/src/core/client/admin/test/fixtures.ts @@ -1,6 +1,6 @@ import { merge } from "lodash"; -import { GQLStory, GQLSTORY_STATUS } from "talk-framework/schema"; +import { GQLStory, GQLSTORY_STATUS, GQLUSER_ROLE } from "talk-framework/schema"; export const settings = { id: "settings", @@ -259,21 +259,21 @@ export const users = [ id: "user-0", username: "Markus", email: "markus@test.com", - role: "ADMIN", + role: GQLUSER_ROLE.ADMIN, }, { ...baseUser, id: "user-1", username: "Lukas", email: "lukas@test.com", - role: "MODERATOR", + role: GQLUSER_ROLE.MODERATOR, }, { ...baseUser, id: "user-2", username: "Isabelle", email: "isabelle@test.com", - role: "COMMENTER", + role: GQLUSER_ROLE.COMMENTER, }, ]; diff --git a/src/core/client/stream/containers/TabBarContainer.tsx b/src/core/client/stream/containers/TabBarContainer.tsx index f1c304256..1de035a0a 100644 --- a/src/core/client/stream/containers/TabBarContainer.tsx +++ b/src/core/client/stream/containers/TabBarContainer.tsx @@ -10,13 +10,13 @@ import { import { TabBarContainer_story as StoryData } from "talk-stream/__generated__/TabBarContainer_story.graphql"; import { TabBarContainer_viewer as ViewerData } from "talk-stream/__generated__/TabBarContainer_viewer.graphql"; import { TabBarContainerLocal as Local } from "talk-stream/__generated__/TabBarContainerLocal.graphql"; -import { roleIsAtLeast } from "talk-stream/helpers"; import { SetActiveTabInput, SetActiveTabMutation, withSetActiveTabMutation, } from "talk-stream/mutations"; +import { Ability, can } from "talk-stream/permissions"; import TabBar from "../components/TabBar"; interface Props { @@ -45,7 +45,7 @@ export class TabBarContainer extends Component { showProfileTab={loggedIn} showConfigureTab={ !!this.props.viewer && - roleIsAtLeast(this.props.viewer.role, "MODERATOR") + can(this.props.viewer, Ability.CHANGE_STORY_CONFIGURATION) } onTabClick={this.handleSetActiveTab} /> diff --git a/src/core/client/stream/helpers/roleIsAtLeast.ts b/src/core/client/stream/helpers/roleIsAtLeast.ts index cb773082f..2036f9617 100644 --- a/src/core/client/stream/helpers/roleIsAtLeast.ts +++ b/src/core/client/stream/helpers/roleIsAtLeast.ts @@ -1,13 +1,15 @@ -// TODO: use generated schema types. -type Role = - | "ADMIN" - | "MODERATOR" - | "STAFF" - | "COMMENTER" - | "%future added value"; +import { GQLUSER_ROLE, GQLUSER_ROLE_RL } from "talk-framework/schema"; -const hierarchy: Role[] = ["COMMENTER", "STAFF", "MODERATOR", "ADMIN"]; -export default function roleIsAtLeast(role: Role, atLeast: Role) { +const hierarchy: GQLUSER_ROLE_RL[] = [ + GQLUSER_ROLE.COMMENTER, + GQLUSER_ROLE.STAFF, + GQLUSER_ROLE.MODERATOR, + GQLUSER_ROLE.ADMIN, +]; +export default function roleIsAtLeast( + role: GQLUSER_ROLE_RL, + atLeast: GQLUSER_ROLE_RL +) { [role, atLeast].forEach(r => { if (!hierarchy.includes(r)) { throw new Error(`Unknown role ${r}`); diff --git a/src/core/client/stream/mutations/CreateCommentMutation.ts b/src/core/client/stream/mutations/CreateCommentMutation.ts index 100d7577a..de69a4446 100644 --- a/src/core/client/stream/mutations/CreateCommentMutation.ts +++ b/src/core/client/stream/mutations/CreateCommentMutation.ts @@ -14,6 +14,7 @@ import { MutationInput, MutationResponsePromise, } from "talk-framework/lib/relay"; +import { GQLUSER_ROLE } from "talk-framework/schema"; import { CreateCommentMutation as MutationTypes } from "talk-stream/__generated__/CreateCommentMutation.graphql"; import { @@ -117,7 +118,8 @@ function commit( // TODO: Generate and use schema types. const expectPremoderation = - !roleIsAtLeast(me.role, "STAFF") && storySettings.moderation === "PRE"; + !roleIsAtLeast(me.role, GQLUSER_ROLE.STAFF) && + storySettings.moderation === "PRE"; return commitMutationPromiseNormalized(environment, { mutation, diff --git a/src/core/client/stream/mutations/CreateCommentReplyMutation.ts b/src/core/client/stream/mutations/CreateCommentReplyMutation.ts index dd26cd7f4..09746dfc4 100644 --- a/src/core/client/stream/mutations/CreateCommentReplyMutation.ts +++ b/src/core/client/stream/mutations/CreateCommentReplyMutation.ts @@ -14,6 +14,7 @@ import { MutationInput, MutationResponsePromise, } from "talk-framework/lib/relay"; +import { GQLUSER_ROLE } from "talk-framework/schema"; import { CreateCommentReplyMutation as MutationTypes } from "talk-stream/__generated__/CreateCommentReplyMutation.graphql"; import { @@ -144,7 +145,8 @@ function commit( // TODO: Generate and use schema types. const expectPremoderation = - !roleIsAtLeast(viewer.role, "STAFF") && storySettings.moderation === "PRE"; + !roleIsAtLeast(viewer.role, GQLUSER_ROLE.STAFF) && + storySettings.moderation === "PRE"; return commitMutationPromiseNormalized(environment, { mutation, diff --git a/src/core/client/stream/permissions.tsx b/src/core/client/stream/permissions.tsx new file mode 100644 index 000000000..561fc5433 --- /dev/null +++ b/src/core/client/stream/permissions.tsx @@ -0,0 +1,32 @@ +import { mapValues } from "lodash"; +import { GQLUSER_ROLE, GQLUSER_ROLE_RL } from "talk-framework/schema"; + +/** + * permissionMap describes what abilities certain roles have. + * + * This list is currently manually managed. We want to + * get to a point where this is generated from the schema. + * + * We currently specify in the comments which endpoints of + * the graph is important for the ability, which we can later + * used to auto generate the map making the schema become + * the single point of truth. + */ +const permissionMap = { + // Mutation.updateStorySettings + CHANGE_STORY_CONFIGURATION: [GQLUSER_ROLE.ADMIN, GQLUSER_ROLE.MODERATOR], +}; + +export type AbilityType = keyof typeof permissionMap; +export const Ability = mapValues(permissionMap, (_, key) => key) as { + [P in AbilityType]: P +}; + +/** + * can is used to check if the `viewer` has permission for `ability`. + * + * Example: `can(props.me, Ability.CHANGE_ROLE)`. + */ +export function can(viewer: { role: GQLUSER_ROLE_RL }, ability: AbilityType) { + return permissionMap[ability].includes(viewer.role as GQLUSER_ROLE); +} diff --git a/src/core/client/stream/test/fixtures.ts b/src/core/client/stream/test/fixtures.ts index b50d39030..00f1049b6 100644 --- a/src/core/client/stream/test/fixtures.ts +++ b/src/core/client/stream/test/fixtures.ts @@ -1,3 +1,4 @@ +import { GQLUSER_ROLE } from "talk-framework/schema"; import { denormalizeComment, denormalizeComments, @@ -76,17 +77,17 @@ export const users = [ { id: "user-0", username: "Markus", - role: "COMMENTER", + role: GQLUSER_ROLE.COMMENTER, }, { id: "user-1", username: "Lukas", - role: "COMMENTER", + role: GQLUSER_ROLE.COMMENTER, }, { id: "user-2", username: "Isabelle", - role: "COMMENTER", + role: GQLUSER_ROLE.COMMENTER, }, ]; @@ -399,13 +400,13 @@ export const storyWithDeepestReplies = denormalizeStory({ export const viewerAsModerator = { id: "me-as-moderator", username: "Moderator", - role: "MODERATOR", + role: GQLUSER_ROLE.MODERATOR, }; export const viewerWithComments = { id: "me-with-comments", username: "Markus", - role: "COMMENTER", + role: GQLUSER_ROLE.COMMENTER, comments: { edges: [ {