From 2ba484fe43880ee09d6e61d778ad467ab7b0e459 Mon Sep 17 00:00:00 2001 From: Ville Brofeldt <33317356+villebro@users.noreply.github.com> Date: Wed, 13 Apr 2022 16:04:04 +0300 Subject: [PATCH] fix: login button does not render (#19685) * fix: login button does not render * add type guard --- .../src/dashboard/util/findPermission.test.ts | 100 +++++++++++------- .../src/dashboard/util/findPermission.ts | 25 +++-- superset-frontend/src/types/bootstrapTypes.ts | 13 +++ 3 files changed, 90 insertions(+), 48 deletions(-) diff --git a/superset-frontend/src/dashboard/util/findPermission.test.ts b/superset-frontend/src/dashboard/util/findPermission.test.ts index 8930549f4a7e..1c80770f5001 100644 --- a/superset-frontend/src/dashboard/util/findPermission.test.ts +++ b/superset-frontend/src/dashboard/util/findPermission.test.ts @@ -16,10 +16,54 @@ * specific language governing permissions and limitations * under the License. */ -import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { + UndefinedUser, + UserWithPermissionsAndRoles, +} from 'src/types/bootstrapTypes'; import Dashboard from 'src/types/Dashboard'; import Owner from 'src/types/Owner'; -import findPermission, { canUserEditDashboard } from './findPermission'; +import findPermission, { + canUserEditDashboard, + isUserAdmin, +} from './findPermission'; + +const ownerUser: UserWithPermissionsAndRoles = { + createdOn: '2021-05-12T16:56:22.116839', + email: 'user@example.com', + firstName: 'Test', + isActive: true, + isAnonymous: false, + lastName: 'User', + userId: 1, + username: 'owner', + permissions: {}, + roles: { Alpha: [['can_write', 'Dashboard']] }, +}; + +const adminUser: UserWithPermissionsAndRoles = { + ...ownerUser, + roles: { + ...(ownerUser?.roles || {}), + Admin: [['can_write', 'Dashboard']], + }, + userId: 2, + username: 'admin', +}; + +const outsiderUser: UserWithPermissionsAndRoles = { + ...ownerUser, + userId: 3, + username: 'outsider', +}; + +const owner: Owner = { + first_name: 'Test', + id: ownerUser.userId, + last_name: 'User', + username: ownerUser.username, +}; + +const undefinedUser: UndefinedUser = {}; describe('findPermission', () => { it('findPermission for single role', () => { @@ -70,42 +114,6 @@ describe('findPermission', () => { }); describe('canUserEditDashboard', () => { - const ownerUser: UserWithPermissionsAndRoles = { - createdOn: '2021-05-12T16:56:22.116839', - email: 'user@example.com', - firstName: 'Test', - isActive: true, - isAnonymous: false, - lastName: 'User', - userId: 1, - username: 'owner', - permissions: {}, - roles: { Alpha: [['can_write', 'Dashboard']] }, - }; - - const adminUser: UserWithPermissionsAndRoles = { - ...ownerUser, - roles: { - ...ownerUser.roles, - Admin: [['can_write', 'Dashboard']], - }, - userId: 2, - username: 'admin', - }; - - const outsiderUser: UserWithPermissionsAndRoles = { - ...ownerUser, - userId: 3, - username: 'outsider', - }; - - const owner: Owner = { - first_name: 'Test', - id: ownerUser.userId, - last_name: 'User', - username: ownerUser.username, - }; - const dashboard: Dashboard = { id: 1, dashboard_title: 'Test Dash', @@ -136,9 +144,7 @@ describe('canUserEditDashboard', () => { it('rejects missing roles', () => { // in redux, when there is no user, the user is actually set to an empty object, // so we need to handle missing roles as well as a missing user.s - expect( - canUserEditDashboard(dashboard, {} as UserWithPermissionsAndRoles), - ).toEqual(false); + expect(canUserEditDashboard(dashboard, {})).toEqual(false); }); it('rejects "admins" if the admin role does not have edit rights for some reason', () => { expect( @@ -149,3 +155,15 @@ describe('canUserEditDashboard', () => { ).toEqual(false); }); }); + +test('isUserAdmin returns true for admin user', () => { + expect(isUserAdmin(adminUser)).toEqual(true); +}); + +test('isUserAdmin returns false for undefined user', () => { + expect(isUserAdmin(undefinedUser)).toEqual(false); +}); + +test('isUserAdmin returns false for non-admin user', () => { + expect(isUserAdmin(ownerUser)).toEqual(false); +}); diff --git a/superset-frontend/src/dashboard/util/findPermission.ts b/superset-frontend/src/dashboard/util/findPermission.ts index d3a8b61eca94..496f993bdf80 100644 --- a/superset-frontend/src/dashboard/util/findPermission.ts +++ b/superset-frontend/src/dashboard/util/findPermission.ts @@ -17,7 +17,11 @@ * under the License. */ import memoizeOne from 'memoize-one'; -import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; +import { + isUserWithPermissionsAndRoles, + UndefinedUser, + UserWithPermissionsAndRoles, +} from 'src/types/bootstrapTypes'; import Dashboard from 'src/types/Dashboard'; type UserRoles = Record; @@ -36,18 +40,25 @@ export default findPermission; // but is hardcoded in backend logic already, so... const ADMIN_ROLE_NAME = 'admin'; -export const isUserAdmin = (user: UserWithPermissionsAndRoles) => - Object.keys(user.roles).some(role => role.toLowerCase() === ADMIN_ROLE_NAME); +export const isUserAdmin = ( + user: UserWithPermissionsAndRoles | UndefinedUser, +) => + isUserWithPermissionsAndRoles(user) && + Object.keys(user.roles || {}).some( + role => role.toLowerCase() === ADMIN_ROLE_NAME, + ); const isUserDashboardOwner = ( dashboard: Dashboard, - user: UserWithPermissionsAndRoles, -) => dashboard.owners.some(owner => owner.username === user.username); + user: UserWithPermissionsAndRoles | UndefinedUser, +) => + isUserWithPermissionsAndRoles(user) && + dashboard.owners.some(owner => owner.username === user.username); export const canUserEditDashboard = ( dashboard: Dashboard, - user?: UserWithPermissionsAndRoles | null, + user?: UserWithPermissionsAndRoles | UndefinedUser | null, ) => - !!user?.roles && + isUserWithPermissionsAndRoles(user) && (isUserAdmin(user) || isUserDashboardOwner(dashboard, user)) && findPermission('can_write', 'Dashboard', user.roles); diff --git a/superset-frontend/src/types/bootstrapTypes.ts b/superset-frontend/src/types/bootstrapTypes.ts index 33314e7e4690..8918ea848904 100644 --- a/superset-frontend/src/types/bootstrapTypes.ts +++ b/superset-frontend/src/types/bootstrapTypes.ts @@ -1,4 +1,5 @@ import { JsonObject, Locale } from '@superset-ui/core'; +import { isPlainObject } from 'lodash'; /** * Licensed to the Apache Software Foundation (ASF) under one @@ -37,6 +38,8 @@ export interface UserWithPermissionsAndRoles extends User { roles: Record; } +export type UndefinedUser = {}; + export type Dashboard = { dttm: number; id: number; @@ -62,3 +65,13 @@ export interface CommonBootstrapData { locale: Locale; feature_flags: Record; } + +export function isUser(user: any): user is User { + return isPlainObject(user) && 'username' in user; +} + +export function isUserWithPermissionsAndRoles( + user: any, +): user is UserWithPermissionsAndRoles { + return isUser(user) && 'permissions' in user && 'roles' in user; +}