From 075914ab0d325d8ec05f5aff88b8477a8171c361 Mon Sep 17 00:00:00 2001 From: Andrew Ballantyne <8126518+andrewballantyne@users.noreply.github.com> Date: Thu, 19 Dec 2024 17:54:42 -0500 Subject: [PATCH] Deprecate & Swap for Auth over DashboardConfig for group configs (#3577) * Deprecate & Swap for Auth over DashboardConfig for group configs * Added tests and adjusted logic to meet missing error logic * Fix useGroups & update ssar check to patch * Update where to look for the dropdown values in cypress * Switch to GET over SSAR to not lock cluster-admins * Fix tests --- .../api/groups-config/groupsConfigUtil.ts | 9 ++ backend/src/routes/api/groups-config/index.ts | 6 + backend/src/types.ts | 14 ++ backend/src/utils/adminUtils.ts | 38 ++++- backend/src/utils/groupsUtils.ts | 5 + backend/src/utils/resourceUtils.ts | 14 ++ frontend/src/__mocks__/mockAuth.ts | 21 +++ frontend/src/__mocks__/mockGroupConfig.ts | 2 +- .../cypress/cypress/pages/userManagement.ts | 2 +- .../cypress/cypress/support/commands/odh.ts | 2 +- frontend/src/api/index.ts | 1 + frontend/src/api/k8s/auth.ts | 34 ++++ frontend/src/api/k8s/groups.ts | 5 +- frontend/src/api/models/odh.ts | 7 + .../__tests__/useWatchGroups.spec.tsx | 75 ++++++--- .../userConfigs}/groupTypes.ts | 2 +- .../concepts/userConfigs/useWatchGroups.tsx | 147 ++++++++++++++++++ frontend/src/concepts/userConfigs/utils.ts | 104 +++++++++++++ frontend/src/k8sTypes.ts | 11 ++ .../src/pages/groupSettings/GroupSettings.tsx | 12 +- frontend/src/services/groupSettingsService.ts | 10 +- frontend/src/utilities/useWatchGroups.tsx | 91 ----------- manifests/core-bases/base/cluster-role.yaml | 6 + 23 files changed, 491 insertions(+), 127 deletions(-) create mode 100644 frontend/src/__mocks__/mockAuth.ts create mode 100644 frontend/src/api/k8s/auth.ts rename frontend/src/{utilities => concepts/userConfigs}/__tests__/useWatchGroups.spec.tsx (59%) rename frontend/src/{pages/groupSettings => concepts/userConfigs}/groupTypes.ts (92%) create mode 100644 frontend/src/concepts/userConfigs/useWatchGroups.tsx create mode 100644 frontend/src/concepts/userConfigs/utils.ts delete mode 100644 frontend/src/utilities/useWatchGroups.tsx diff --git a/backend/src/routes/api/groups-config/groupsConfigUtil.ts b/backend/src/routes/api/groups-config/groupsConfigUtil.ts index 8c125ebca4..10d2e37fe8 100644 --- a/backend/src/routes/api/groups-config/groupsConfigUtil.ts +++ b/backend/src/routes/api/groups-config/groupsConfigUtil.ts @@ -1,3 +1,9 @@ +/** + * @fileOverview + * @deprecated + * The Whole file is deprecated. The exposed functions of the module are deprecated to help. + * We are moving to direct k8s auth control and doing away with the OdhDashboardConfig group info. + */ import { FastifyRequest } from 'fastify'; import createError from 'http-errors'; import { @@ -13,6 +19,7 @@ import { isUserAdmin } from '../../../utils/adminUtils'; const SYSTEM_AUTHENTICATED = 'system:authenticated'; +/** @deprecated - see RHOAIENG-16988 */ export const getGroupsConfig = async (fastify: KubeFastifyInstance): Promise => { const { customObjectsApi } = fastify.kube; @@ -27,6 +34,7 @@ export const getGroupsConfig = async (fastify: KubeFastifyInstance): Promise groupStatus.filter((group) => group.enabled).map((group) => group.name); +/** @deprecated - see RHOAIENG-16988 */ export const updateGroupsConfig = async ( fastify: KubeFastifyInstance, request: FastifyRequest<{ Body: GroupsConfig }>, @@ -151,6 +159,7 @@ const getError = ( }; /** + * @deprecated - see RHOAIENG-16988 * Check if any selected groups has been deleted and update the configuration if so * @param fastify Fastify instance * @param groupsData Custom Resource Data for group configuration diff --git a/backend/src/routes/api/groups-config/index.ts b/backend/src/routes/api/groups-config/index.ts index 551b249eac..4da9193d89 100644 --- a/backend/src/routes/api/groups-config/index.ts +++ b/backend/src/routes/api/groups-config/index.ts @@ -1,9 +1,14 @@ +/** + * @fileOverview + * @deprecated see RHOAIENG-16988 + */ import { FastifyRequest } from 'fastify'; import { GroupsConfig, KubeFastifyInstance } from '../../../types'; import { getGroupsConfig, updateGroupsConfig } from './groupsConfigUtil'; import { secureAdminRoute } from '../../../utils/route-security'; export default async (fastify: KubeFastifyInstance): Promise => { + /** @deprecated - see RHOAIENG-16988 */ fastify.get( '/', secureAdminRoute(fastify)(async (request, reply) => { @@ -16,6 +21,7 @@ export default async (fastify: KubeFastifyInstance): Promise => { }), ); + /** @deprecated - see RHOAIENG-16988 */ fastify.put( '/', secureAdminRoute(fastify)(async (request: FastifyRequest<{ Body: GroupsConfig }>, reply) => { diff --git a/backend/src/types.ts b/backend/src/types.ts index df185bfa40..990c00f424 100644 --- a/backend/src/types.ts +++ b/backend/src/types.ts @@ -46,8 +46,11 @@ export type DashboardConfig = K8sResourceCommon & { disableStorageClasses: boolean; disableNIMModelServing: boolean; }; + /** @deprecated -- replacing this with Platform Auth resource -- remove when this is no longer in the CRD */ groupsConfig?: { + /** @deprecated -- see above */ adminGroups: string; + /** @deprecated -- see above */ allowedGroups: string; }; notebookSizes?: NotebookSize[]; @@ -1271,3 +1274,14 @@ export type ListConfigSecretsResponse = { secrets: ConfigSecretItem[]; configMaps: ConfigSecretItem[]; }; + +export type AuthKind = K8sResourceCommon & { + metadata: { + name: 'auth'; // singleton, immutable name + namespace?: never; // Cluster resource + }; + spec: { + adminGroups: string[]; + allowedGroups: string[]; + }; +}; diff --git a/backend/src/utils/adminUtils.ts b/backend/src/utils/adminUtils.ts index e42a0c7848..519d3496d1 100644 --- a/backend/src/utils/adminUtils.ts +++ b/backend/src/utils/adminUtils.ts @@ -7,6 +7,7 @@ import { KubeFastifyInstance, ResourceAccessReviewResponse } from '../types'; import { getAdminGroups, getAllGroupsByUser, getAllowedGroups, getGroup } from './groupsUtils'; import { flatten, uniq } from 'lodash'; import { getNamespaces } from '../utils/notebookUtils'; +import { getAuth } from './resourceUtils'; const SYSTEM_AUTHENTICATED = 'system:authenticated'; /** Usernames with invalid characters can start with `b64:` to keep their unwanted characters */ @@ -24,6 +25,12 @@ const getGroupUserList = async ( }; export const getAdminUserList = async (fastify: KubeFastifyInstance): Promise => { + const auth = getAuth(); + if (auth) { + return getGroupUserList(fastify, auth.spec.adminGroups); + } + + // FIXME: see RHOAIENG-16988 const adminGroups = getAdminGroups(); const adminGroupsList = adminGroups .split(',') @@ -61,6 +68,12 @@ export const getClusterAdminUserList = async (fastify: KubeFastifyInstance): Pro }; export const getAllowedUserList = async (fastify: KubeFastifyInstance): Promise => { + const auth = getAuth(); + if (auth) { + return getGroupUserList(fastify, auth.spec.allowedGroups); + } + + // FIXME: see RHOAIENG-16988 const allowedGroups = getAllowedGroups(); const allowedGroupList = allowedGroups .split(',') @@ -74,8 +87,16 @@ export const getGroupsConfig = async ( username: string, ): Promise => { try { - const adminGroups = getAdminGroups(); - const adminGroupsList = adminGroups.split(','); + const auth = getAuth(); + + let adminGroupsList: string[]; + if (auth) { + adminGroupsList = auth.spec.adminGroups; + } else { + // FIXME: see RHOAIENG-16988 + const adminGroups = getAdminGroups(); + adminGroupsList = adminGroups.split(','); + } if (adminGroupsList.includes(SYSTEM_AUTHENTICATED)) { throw new Error('It is not allowed to set system:authenticated as admin group.'); @@ -102,8 +123,17 @@ export const isUserAllowed = async ( username: string, ): Promise => { try { - const allowedGroups = getAllowedGroups(); - const allowedGroupsList = allowedGroups.split(','); + const auth = getAuth(); + + let allowedGroupsList: string[]; + if (auth) { + allowedGroupsList = auth.spec.allowedGroups; + } else { + // FIXME: see RHOAIENG-16988 + const allowedGroups = getAllowedGroups(); + allowedGroupsList = allowedGroups.split(','); + } + if (allowedGroupsList.includes(SYSTEM_AUTHENTICATED)) { return true; } else { diff --git a/backend/src/utils/groupsUtils.ts b/backend/src/utils/groupsUtils.ts index 2cfc266b7a..be738b7316 100644 --- a/backend/src/utils/groupsUtils.ts +++ b/backend/src/utils/groupsUtils.ts @@ -15,6 +15,7 @@ export class MissingGroupError extends Error { } } +/** @deprecated - see RHOAIENG-16988 */ export const getGroupsCR = (): GroupsConfigBody => { if (getDashboardConfig().spec.groupsConfig) { return getDashboardConfig().spec.groupsConfig; @@ -22,6 +23,7 @@ export const getGroupsCR = (): GroupsConfigBody => { throw new Error(`Failed to retrieve Dashboard CR groups configuration`); }; +/** @deprecated - see RHOAIENG-16988 */ export const updateGroupsCR = async ( fastify: KubeFastifyInstance, groupsConfigBody: GroupsConfigBody, @@ -38,6 +40,7 @@ export const updateGroupsCR = async ( } }; +/** @deprecated - see RHOAIENG-16988 */ export const getAdminGroups = (): string => { try { return getGroupsCR().adminGroups; @@ -46,6 +49,7 @@ export const getAdminGroups = (): string => { } }; +/** @deprecated - see RHOAIENG-16988 */ export const getAllowedGroups = (): string => { try { return getGroupsCR().allowedGroups; @@ -71,6 +75,7 @@ export const getGroup = async ( } }; +/** @deprecated - see RHOAIENG-16988 */ export const getAllGroups = async (customObjectsApi: CustomObjectsApi): Promise => { try { const adminGroupResponse = await customObjectsApi.listClusterCustomObject( diff --git a/backend/src/utils/resourceUtils.ts b/backend/src/utils/resourceUtils.ts index 2ac5ed2c63..067c31f54c 100644 --- a/backend/src/utils/resourceUtils.ts +++ b/backend/src/utils/resourceUtils.ts @@ -22,6 +22,7 @@ import { TolerationOperator, DataScienceClusterKindStatus, KnownLabels, + AuthKind, } from '../types'; import { DEFAULT_ACTIVE_TIMEOUT, @@ -50,6 +51,7 @@ const quickStartsVersion = 'v1'; const quickStartsPlural = 'odhquickstarts'; let dashboardConfigWatcher: ResourceWatcher; +let authWatcher: ResourceWatcher; let clusterStatusWatcher: ResourceWatcher; let subscriptionWatcher: ResourceWatcher; let appWatcher: ResourceWatcher; @@ -549,8 +551,16 @@ const fetchConsoleLinks = async (fastify: KubeFastifyInstance) => { }); }; +const fetchAuthKind = (fastify: KubeFastifyInstance): Promise => { + return fastify.kube.customObjectsApi + .getClusterCustomObject('services.platform.opendatahub.io', 'v1alpha1', 'auths', 'auth') + .then((response) => response.body as AuthKind) + .then((auth) => [auth]); +}; + export const initializeWatchedResources = (fastify: KubeFastifyInstance): void => { dashboardConfigWatcher = new ResourceWatcher(fastify, fetchDashboardCR); + authWatcher = new ResourceWatcher(fastify, fetchAuthKind); clusterStatusWatcher = new ResourceWatcher( fastify, fetchWatchedClusterStatus, @@ -627,6 +637,10 @@ export const getApplication = (appName: string): OdhApplication => { return apps.find((app) => app.metadata.name === appName); }; +export const getAuth = (): AuthKind => { + return authWatcher.getResources()[0]; +}; + export const getDocs = (): OdhDocument[] => { return docWatcher.getResources(); }; diff --git a/frontend/src/__mocks__/mockAuth.ts b/frontend/src/__mocks__/mockAuth.ts new file mode 100644 index 0000000000..1ac97e06ad --- /dev/null +++ b/frontend/src/__mocks__/mockAuth.ts @@ -0,0 +1,21 @@ +import { AuthKind } from '~/k8sTypes'; + +type MockAuthData = { + adminGroups?: string[]; + allowedGroups?: string[]; +}; + +export const mockAuth = ({ + adminGroups = ['odh-admins'], + allowedGroups = ['system:authenticated'], +}: MockAuthData = {}): AuthKind => ({ + apiVersion: 'services.platform.opendatahub.io/v1alpha1', + kind: 'Auth', + metadata: { + name: 'auth', + }, + spec: { + adminGroups, + allowedGroups, + }, +}); diff --git a/frontend/src/__mocks__/mockGroupConfig.ts b/frontend/src/__mocks__/mockGroupConfig.ts index aa29a91add..de533e7a25 100644 --- a/frontend/src/__mocks__/mockGroupConfig.ts +++ b/frontend/src/__mocks__/mockGroupConfig.ts @@ -1,4 +1,4 @@ -import { GroupsConfig } from '~/pages/groupSettings/groupTypes'; +import { GroupsConfig } from '~/concepts/userConfigs/groupTypes'; export const mockGroupSettings = (): GroupsConfig => ({ adminGroups: [ diff --git a/frontend/src/__tests__/cypress/cypress/pages/userManagement.ts b/frontend/src/__tests__/cypress/cypress/pages/userManagement.ts index aed2c8cef5..5213ac2932 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/userManagement.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/userManagement.ts @@ -16,7 +16,7 @@ class GroupSettingSection extends Contextual { } findMultiGroupOptions(name: string) { - return this.find().findByRole('option', { name }); + return this.find().document().findByRole('option', { name }); } private findChipGroup() { diff --git a/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts b/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts index 2b500aa58d..ba2b170fbb 100644 --- a/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts +++ b/frontend/src/__tests__/cypress/cypress/support/commands/odh.ts @@ -24,7 +24,7 @@ import type { import type { StartNotebookData } from '~/pages/projects/types'; import type { AllowedUser } from '~/pages/notebookController/screens/admin/types'; -import type { GroupsConfig } from '~/pages/groupSettings/groupTypes'; +import type { GroupsConfig } from '~/concepts/userConfigs/groupTypes'; import type { StatusResponse } from '~/redux/types'; import type { BYONImage, diff --git a/frontend/src/api/index.ts b/frontend/src/api/index.ts index 40033d7a77..e21c524694 100644 --- a/frontend/src/api/index.ts +++ b/frontend/src/api/index.ts @@ -1,5 +1,6 @@ // Normal SDK/pass-through network API calls export * from './k8s/nimAccounts'; +export * from './k8s/auth'; export * from './k8s/builds'; export * from './k8s/configMaps'; export * from './k8s/events'; diff --git a/frontend/src/api/k8s/auth.ts b/frontend/src/api/k8s/auth.ts new file mode 100644 index 0000000000..1959121b61 --- /dev/null +++ b/frontend/src/api/k8s/auth.ts @@ -0,0 +1,34 @@ +import { k8sGetResource, k8sPatchResource, Patch } from '@openshift/dynamic-plugin-sdk-utils'; +import { AuthModel } from '~/api'; +import { AuthKind } from '~/k8sTypes'; + +export const AUTH_SINGLETON_NAME = 'auth'; + +export const getAuth = (): Promise => + k8sGetResource({ model: AuthModel, queryOptions: { name: AUTH_SINGLETON_NAME } }); + +export type GroupData = { adminGroups?: string[]; allowedGroups?: string[] }; +export const patchAuth = (groupData: GroupData): Promise => { + const { adminGroups, allowedGroups } = groupData; + const patches: Patch[] = []; + if (adminGroups) { + patches.push({ + value: adminGroups, + op: 'replace', + path: '/spec/adminGroups', + }); + } + if (allowedGroups) { + patches.push({ + value: allowedGroups, + op: 'replace', + path: '/spec/allowedGroups', + }); + } + + return k8sPatchResource({ + model: AuthModel, + queryOptions: { name: AUTH_SINGLETON_NAME }, + patches, + }); +}; diff --git a/frontend/src/api/k8s/groups.ts b/frontend/src/api/k8s/groups.ts index 33a3909478..510062a89c 100644 --- a/frontend/src/api/k8s/groups.ts +++ b/frontend/src/api/k8s/groups.ts @@ -26,9 +26,12 @@ export const useGroups = (): CustomWatchK8sResult => { const [groupData, loaded, error] = useK8sWatchResourceList(initResource, GroupModel); return React.useMemo(() => { + if (!accessReviewLoaded) { + return [[], false, undefined]; + } if (!allowList) { return [[], true, undefined]; } return [groupData, loaded, error]; - }, [error, groupData, loaded, allowList]); + }, [accessReviewLoaded, allowList, groupData, loaded, error]); }; diff --git a/frontend/src/api/models/odh.ts b/frontend/src/api/models/odh.ts index 7a596f7991..4e4d5dec1a 100644 --- a/frontend/src/api/models/odh.ts +++ b/frontend/src/api/models/odh.ts @@ -48,3 +48,10 @@ export const NIMAccountModel: K8sModelCommon = { kind: 'Account', plural: 'accounts', }; + +export const AuthModel: K8sModelCommon = { + apiVersion: 'v1alpha1', + apiGroup: 'services.platform.opendatahub.io', + kind: 'Auth', + plural: 'auths', +}; diff --git a/frontend/src/utilities/__tests__/useWatchGroups.spec.tsx b/frontend/src/concepts/userConfigs/__tests__/useWatchGroups.spec.tsx similarity index 59% rename from frontend/src/utilities/__tests__/useWatchGroups.spec.tsx rename to frontend/src/concepts/userConfigs/__tests__/useWatchGroups.spec.tsx index aa487f6b03..56018ccfd9 100644 --- a/frontend/src/utilities/__tests__/useWatchGroups.spec.tsx +++ b/frontend/src/concepts/userConfigs/__tests__/useWatchGroups.spec.tsx @@ -1,16 +1,31 @@ import { act } from 'react'; import { testHook } from '~/__tests__/unit/testUtils/hooks'; -import { GroupsConfig } from '~/pages/groupSettings/groupTypes'; +import { GroupsConfig } from '~/concepts/userConfigs/groupTypes'; import { fetchGroupsSettings, updateGroupsSettings } from '~/services/groupSettingsService'; -import { useWatchGroups } from '~/utilities/useWatchGroups'; +import { useWatchGroups } from '~/concepts/userConfigs/useWatchGroups'; +import { getAuth, patchAuth, useGroups } from '~/api'; import useNotification from '~/utilities/useNotification'; +import { GroupKind } from '~/k8sTypes'; +import { mockGroup } from '~/__mocks__/mockGroup'; +import { fetchAuthGroups } from '~/concepts/userConfigs/utils'; +import { mockAuth } from '~/__mocks__/mockAuth'; jest.mock('~/services/groupSettingsService', () => ({ fetchGroupsSettings: jest.fn(), updateGroupsSettings: jest.fn(), })); +jest.mock('~/api', () => ({ + ...jest.requireActual('~/api'), + getAuth: jest.fn(), + patchAuth: jest.fn(), + useGroups: jest.fn(), +})); +jest.mock('~/concepts/userConfigs/utils', () => ({ + ...jest.requireActual('~/concepts/userConfigs/utils'), + fetchAuthGroups: jest.fn(), +})); // Mock the useNotification hook -jest.mock('../useNotification', () => { +jest.mock('../../../utilities/useNotification', () => { const mock = { success: jest.fn(), error: jest.fn(), @@ -20,8 +35,12 @@ jest.mock('../useNotification', () => { default: jest.fn(() => mock), }; }); +const getAuthMock = jest.mocked(getAuth); +const patchAuthMock = jest.mocked(patchAuth); const fetchGroupSettingsMock = jest.mocked(fetchGroupsSettings); +const fetchAuthGroupsMock = jest.mocked(fetchAuthGroups); const useNotificationMock = jest.mocked(useNotification); +const useGroupsMock = jest.mocked(useGroups); const updateGroupSettingsMock = jest.mocked(updateGroupsSettings); const mockEmptyGroupSettings = { adminGroups: [], @@ -44,18 +63,28 @@ const createResult = (r: Partial>) => ); describe('useWatchGroups', () => { + beforeEach(() => { + getAuthMock.mockImplementation(() => Promise.resolve(mockAuth())); + patchAuthMock.mockResolvedValue(mockAuth()); + const groups: GroupKind[] = [mockGroup({ name: 'odh-admins' })]; + useGroupsMock.mockImplementation(() => [groups, true, undefined]); + }); + it('should fetch groups successfully', async () => { const mockGroupSettings: GroupsConfig = { - adminGroups: [{ id: 1, name: 'odh-admins', enabled: true }], + adminGroups: [{ id: 'odh-admins', name: 'odh-admins', enabled: true }], allowedGroups: [], }; + updateGroupSettingsMock.mockImplementation((group) => Promise.resolve(group)); - fetchGroupSettingsMock.mockResolvedValue(mockGroupSettings); + fetchAuthGroupsMock.mockResolvedValue(mockGroupSettings); const renderResult = testHook(useWatchGroups)(); - expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(1); - expect(renderResult).hookToStrictEqual(createResult({ loaded: false, isLoading: true })); + expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(0); + expect(fetchAuthGroupsMock).toHaveBeenCalledTimes(0); // useFetchState takes a cycle to invoke + expect(renderResult).hookToStrictEqual(createResult({ loaded: false, isLoading: false })); await renderResult.waitForNextUpdate(); - expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(1); + expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(0); + expect(fetchAuthGroupsMock).toHaveBeenCalledTimes(1); expect(renderResult).hookToStrictEqual(createResult({ groupSettings: mockGroupSettings })); renderResult.rerender(); expect(renderResult).hookToBeStable({ @@ -68,9 +97,13 @@ describe('useWatchGroups', () => { setGroupSettings: true, setIsGroupSettingsChanged: true, }); + const mockUpdatedGroupSettings: GroupsConfig = { - adminGroups: [{ id: 2, name: 'odh-admins', enabled: false }], - allowedGroups: [], + adminGroups: [{ id: 'odh-admins', name: 'odh-admins', enabled: true }], + allowedGroups: [ + { id: 'odh-admins', name: 'odh-admins', enabled: false }, + { id: 'system:authenticated', name: 'system:authenticated', enabled: true }, + ], }; act(() => { renderResult.result.current.setIsGroupSettingsChanged(true); @@ -95,28 +128,30 @@ describe('useWatchGroups', () => { }); it('should handle error', async () => { - fetchGroupSettingsMock.mockRejectedValue(new Error(`Error updating group settings`)); + fetchAuthGroupsMock.mockRejectedValue(new Error(`Error getting group settings`)); const renderResult = testHook(useWatchGroups)(); - expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(1); - expect(renderResult).hookToStrictEqual(createResult({ loaded: false, isLoading: true })); + expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(0); + expect(fetchAuthGroupsMock).toHaveBeenCalledTimes(0); // useFetchState takes a cycle to invoke + expect(renderResult).hookToStrictEqual(createResult({ loaded: false, isLoading: false })); await renderResult.waitForNextUpdate(); - expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(1); + expect(fetchGroupSettingsMock).toHaveBeenCalledTimes(0); + expect(fetchAuthGroupsMock).toHaveBeenCalledTimes(1); expect(renderResult).hookToStrictEqual( - createResult({ loaded: false, loadError: new Error('Error updating group settings') }), + createResult({ loaded: false, loadError: new Error('Error getting group settings') }), ); expect(useNotificationMock().error).toHaveBeenCalledWith( 'Error', - 'Error updating group settings', + 'Error getting group settings', ); }); it('should test admin error', async () => { const mockGroupSettings: GroupsConfig = { - adminGroups: [{ id: 1, name: 'odh-admins', enabled: true }], + adminGroups: [{ id: 'odh-admins', name: 'odh-admins', enabled: true }], allowedGroups: [], errorAdmin: 'errorAdmin', }; - fetchGroupSettingsMock.mockResolvedValue(mockGroupSettings); + fetchAuthGroupsMock.mockResolvedValue(mockGroupSettings); const renderResult = testHook(useWatchGroups)(); await renderResult.waitForNextUpdate(); expect(useNotificationMock().error).toHaveBeenCalledWith('Group error', 'errorAdmin'); @@ -124,11 +159,11 @@ describe('useWatchGroups', () => { it('should test user error', async () => { const mockGroupSettings: GroupsConfig = { - adminGroups: [{ id: 1, name: 'odh-admins', enabled: true }], + adminGroups: [{ id: 'odh-admins', name: 'odh-admins', enabled: true }], allowedGroups: [], errorUser: 'errorUser', }; - fetchGroupSettingsMock.mockResolvedValue(mockGroupSettings); + fetchAuthGroupsMock.mockResolvedValue(mockGroupSettings); const renderResult = testHook(useWatchGroups)(); await renderResult.waitForNextUpdate(); expect(useNotificationMock().error).toHaveBeenCalledWith('Group error', 'errorUser'); diff --git a/frontend/src/pages/groupSettings/groupTypes.ts b/frontend/src/concepts/userConfigs/groupTypes.ts similarity index 92% rename from frontend/src/pages/groupSettings/groupTypes.ts rename to frontend/src/concepts/userConfigs/groupTypes.ts index e399469ffd..22566dbd35 100644 --- a/frontend/src/pages/groupSettings/groupTypes.ts +++ b/frontend/src/concepts/userConfigs/groupTypes.ts @@ -11,7 +11,7 @@ export enum GroupsConfigField { } export type GroupStatus = { - id: number; + id: number | string; name: string; enabled: boolean; }; diff --git a/frontend/src/concepts/userConfigs/useWatchGroups.tsx b/frontend/src/concepts/userConfigs/useWatchGroups.tsx new file mode 100644 index 0000000000..7b96a30ae6 --- /dev/null +++ b/frontend/src/concepts/userConfigs/useWatchGroups.tsx @@ -0,0 +1,147 @@ +import * as React from 'react'; +import { GroupsConfig } from '~/concepts/userConfigs/groupTypes'; +import { fetchGroupsSettings, updateGroupsSettings } from '~/services/groupSettingsService'; +import { + fetchAuthGroups, + updateAuthGroups, + useDoesUserHaveAuthAccess, +} from '~/concepts/userConfigs/utils'; +import useNotification from '~/utilities/useNotification'; +import { useGroups } from '~/api'; + +export const useWatchGroups = (): { + groupSettings: GroupsConfig; + loaded: boolean; + isLoading: boolean; + isGroupSettingsChanged: boolean; + loadError: Error | undefined; + updateGroups: (group: GroupsConfig) => void; + setGroupSettings: (group: GroupsConfig) => void; + setIsGroupSettingsChanged: (changed: boolean) => void; +} => { + const [loaded, setLoaded] = React.useState(false); + const [loadError, setLoadError] = React.useState(undefined); + const [isLoading, setIsLoading] = React.useState(false); + const [isGroupSettingsChanged, setIsGroupSettingsChanged] = React.useState(false); + const notification = useNotification(); + const [groupSettings, setGroupSettings] = React.useState({ + adminGroups: [], + allowedGroups: [], + }); + const { errorAdmin, errorUser } = groupSettings; + const [canUpdateAuthResource, isAuthCheckDone] = useDoesUserHaveAuthAccess(); + const [groupsData, groupsDataLoaded] = useGroups(); + + const hasDirectAccessDataLoaded = groupsDataLoaded && isAuthCheckDone; + + React.useEffect(() => { + const fetchGroups = () => { + setIsLoading(true); + if (canUpdateAuthResource) { + fetchAuthGroups(groupsData) + .then((groupConfig) => { + setGroupSettings(groupConfig); + setLoadError(undefined); + setLoaded(true); + }) + .catch((error) => { + notification.error(`Error`, `Error getting group settings`); + setLoadError(error); + setLoaded(false); + }) + .finally(() => { + setIsLoading(false); + setIsGroupSettingsChanged(false); + }); + } else { + // TODO: This path should be deprecated in RHOAI 2.17 -- remove once we fully move to Auth + // eslint-disable-next-line no-console + console.log( + 'This user does not have access to update the Auth resource -- falling back on reading OdhDashboardConfig', + ); + fetchGroupsSettings() + .then((groupsResponse) => { + setGroupSettings(groupsResponse); + setLoadError(undefined); + setLoaded(true); + }) + .catch((error) => { + notification.error(`Error`, `Error updating group settings`); + setLoadError(error); + setLoaded(false); + }) + .finally(() => { + setIsLoading(false); + setIsGroupSettingsChanged(false); + }); + } + }; + + if (hasDirectAccessDataLoaded) { + fetchGroups(); + } + }, [notification, canUpdateAuthResource, hasDirectAccessDataLoaded, groupsData]); + + React.useEffect(() => { + if (errorAdmin) { + notification.error(`Group error`, errorAdmin); + } + if (errorUser) { + notification.error(`Group error`, errorUser); + } + }, [errorAdmin, errorUser, notification]); + + const updateGroups = React.useCallback( + (group: GroupsConfig) => { + setIsLoading(true); + if (canUpdateAuthResource) { + updateAuthGroups(group, groupsData) + .then((groupsResponse) => { + setGroupSettings(groupsResponse); + }) + .catch((error) => { + setLoadError(error); + setLoaded(false); + }) + .finally(() => { + setIsLoading(false); + setIsGroupSettingsChanged(false); + }); + } else { + // TODO: This path should be deprecated in RHOAI 2.17 -- remove once we fully move to Auth + // eslint-disable-next-line no-console + console.log( + 'This user does not have access to update the Auth resource -- falling back on updating OdhDashboardConfig', + ); + updateGroupsSettings(group) + .then((response) => { + setGroupSettings(response); + notification.success( + 'Group settings changes saved', + 'It may take up to 2 minutes for configuration changes to be applied.', + ); + }) + .catch((error) => { + setLoadError(error); + setLoaded(false); + }) + .finally(() => { + setIsLoading(false); + setIsGroupSettingsChanged(false); + }); + } + }, + [canUpdateAuthResource, groupsData, notification], + ); + + return { + groupSettings, + loaded, + isLoading, + isGroupSettingsChanged, + loadError, + updateGroups, + setGroupSettings, + setIsGroupSettingsChanged, + }; +}; diff --git a/frontend/src/concepts/userConfigs/utils.ts b/frontend/src/concepts/userConfigs/utils.ts new file mode 100644 index 0000000000..798f6de00d --- /dev/null +++ b/frontend/src/concepts/userConfigs/utils.ts @@ -0,0 +1,104 @@ +import * as React from 'react'; +import { AuthKind, GroupKind } from '~/k8sTypes'; +import { getAuth, patchAuth } from '~/api'; +import useFetchState from '~/utilities/useFetchState'; +import { GroupsConfig } from './groupTypes'; + +export const useDoesUserHaveAuthAccess = (): [hasAccess: boolean, loadedAccess: boolean] => { + const [state, loaded] = useFetchState( + React.useCallback( + () => + getAuth() + .then(() => true) + .catch(() => false), + [], + ), + false, + ); + return [state, loaded]; +}; + +const ALL_USERS = 'system:authenticated'; + +const getError = (array: string[], predicate: (group: string) => boolean): string | undefined => { + let error; + if (array.length === 0) { + error = 'No group is set in the group config, please set one or more group.'; + // eslint-disable-next-line no-console + console.error(error); + return error; + } + + const missingItems = array.filter(predicate); + if (missingItems.length === 0) { + return undefined; + } + + error = `The group${missingItems.length === 1 ? '' : 's'} ${missingItems.join( + ', ', + )} no longer exists in OpenShift and has been removed from the selected group list.`; + // eslint-disable-next-line no-console + console.error(error); + return error; +}; + +const compileGroupValues = ( + adminGroups: string[], + allowedGroups: string[], + groups: GroupKind[], +): GroupsConfig => { + const groupNames = groups.map((g) => g.metadata.name); + + const data: GroupsConfig = { + allowedGroups: [ + ...groupNames.map((name) => ({ + id: name, + name, + enabled: allowedGroups.includes(name), + })), + { id: ALL_USERS, name: ALL_USERS, enabled: allowedGroups.includes(ALL_USERS) }, + ], + adminGroups: groupNames.map((name) => ({ + id: name, + name, + enabled: adminGroups.includes(name), + })), + }; + + const errorAdmin = getError(adminGroups, (group) => !groupNames.includes(group)); + if (errorAdmin) { + data.errorAdmin = errorAdmin; + } + const errorUser = getError( + allowedGroups, + (group) => !groupNames.includes(group) && group !== ALL_USERS, + ); + if (errorUser) { + data.errorUser = errorUser; + } + + return data; +}; + +const convertAuthToGroupValues = (auth: AuthKind, groups: GroupKind[]): GroupsConfig => { + const { + spec: { adminGroups, allowedGroups }, + } = auth; + + return compileGroupValues(adminGroups, allowedGroups, groups); +}; + +export const fetchAuthGroups = async (groups: GroupKind[]): Promise => + getAuth().then((auth) => convertAuthToGroupValues(auth, groups)); + +export const updateAuthGroups = async ( + groupConfigs: GroupsConfig, + groups: GroupKind[], +): Promise => { + const adminGroups = groupConfigs.adminGroups.filter((g) => g.enabled).map((g) => g.name); + const allowedGroups = groupConfigs.allowedGroups.filter((g) => g.enabled).map((g) => g.name); + + return patchAuth({ adminGroups, allowedGroups }).then((auth) => + convertAuthToGroupValues(auth, groups), + ); +}; diff --git a/frontend/src/k8sTypes.ts b/frontend/src/k8sTypes.ts index e6659ca8aa..7309dbd23e 100644 --- a/frontend/src/k8sTypes.ts +++ b/frontend/src/k8sTypes.ts @@ -1377,3 +1377,14 @@ export type ListConfigSecretsResponse = { secrets: ConfigSecretItem[]; configMaps: ConfigSecretItem[]; }; + +export type AuthKind = K8sResourceCommon & { + metadata: { + name: 'auth'; // singleton, immutable name + namespace?: never; // Cluster resource + }; + spec: { + adminGroups: string[]; + allowedGroups: string[]; + }; +}; diff --git a/frontend/src/pages/groupSettings/GroupSettings.tsx b/frontend/src/pages/groupSettings/GroupSettings.tsx index 2c05278acc..d662856b59 100644 --- a/frontend/src/pages/groupSettings/GroupSettings.tsx +++ b/frontend/src/pages/groupSettings/GroupSettings.tsx @@ -12,10 +12,10 @@ import ApplicationsPage from '~/pages/ApplicationsPage'; import { isGroupEmpty } from '~/utilities/utils'; import SettingSection from '~/components/SettingSection'; import { MultiSelection, SelectionOptions } from '~/components/MultiSelection'; -import { useWatchGroups } from '~/utilities/useWatchGroups'; +import { useWatchGroups } from '~/concepts/userConfigs/useWatchGroups'; import TitleWithIcon from '~/concepts/design/TitleWithIcon'; import { ProjectObjectType } from '~/concepts/design/utils'; -import { GroupsConfigField } from './groupTypes'; +import { GroupsConfigField } from '~/concepts/userConfigs/groupTypes'; const GroupSettings: React.FC = () => { const { @@ -45,7 +45,7 @@ const GroupSettings: React.FC = () => { setGroupSettings({ ...groupSettings, adminGroups: newState.map((opt) => ({ - id: Number(opt.id), + id: opt.id, name: opt.name, enabled: opt.selected || false, })), @@ -55,7 +55,7 @@ const GroupSettings: React.FC = () => { setGroupSettings({ ...groupSettings, allowedGroups: newState.map((opt) => ({ - id: Number(opt.id), + id: opt.id, name: opt.name, enabled: opt.selected || false, })), @@ -103,7 +103,7 @@ const GroupSettings: React.FC = () => { setValue={(newState) => handleMenuItemSelection(newState, GroupsConfigField.ADMIN)} selectionRequired noSelectedOptionsMessage="One or more group must be selected" - popperProps={{ appendTo: 'inline' }} + popperProps={{ appendTo: document.body }} /> {groupSettings.errorAdmin ? ( { setValue={(newState) => handleMenuItemSelection(newState, GroupsConfigField.USER)} selectionRequired noSelectedOptionsMessage="One or more group must be selected" - popperProps={{ appendTo: 'inline' }} + popperProps={{ appendTo: document.body }} /> {groupSettings.errorUser ? ( => { const url = '/api/groups-config'; return axios @@ -11,6 +15,10 @@ export const fetchGroupsSettings = (): Promise => { }); }; +/** + * @deprecated Use Auth Resource instead + * @see updateAuthGroups + */ export const updateGroupsSettings = (settings: GroupsConfig): Promise => { const url = '/api/groups-config'; return axios diff --git a/frontend/src/utilities/useWatchGroups.tsx b/frontend/src/utilities/useWatchGroups.tsx deleted file mode 100644 index 4b2ec57418..0000000000 --- a/frontend/src/utilities/useWatchGroups.tsx +++ /dev/null @@ -1,91 +0,0 @@ -import * as React from 'react'; -import { GroupsConfig } from '~/pages/groupSettings/groupTypes'; -import { fetchGroupsSettings, updateGroupsSettings } from '~/services/groupSettingsService'; -import useNotification from './useNotification'; - -export const useWatchGroups = (): { - groupSettings: GroupsConfig; - loaded: boolean; - isLoading: boolean; - isGroupSettingsChanged: boolean; - loadError: Error | undefined; - updateGroups: (group: GroupsConfig) => void; - setGroupSettings: (group: GroupsConfig) => void; - setIsGroupSettingsChanged: (changed: boolean) => void; -} => { - const [loaded, setLoaded] = React.useState(false); - const [loadError, setLoadError] = React.useState(undefined); - const [isLoading, setIsLoading] = React.useState(false); - const [isGroupSettingsChanged, setIsGroupSettingsChanged] = React.useState(false); - const notification = useNotification(); - const [groupSettings, setGroupSettings] = React.useState({ - adminGroups: [], - allowedGroups: [], - }); - const { errorAdmin, errorUser } = groupSettings; - - React.useEffect(() => { - const fetchGroups = () => { - setIsLoading(true); - fetchGroupsSettings() - .then((groupsResponse) => { - setGroupSettings(groupsResponse); - setLoadError(undefined); - setLoaded(true); - }) - .catch((error) => { - notification.error(`Error`, `Error updating group settings`); - setLoadError(error); - setLoaded(false); - }) - .finally(() => { - setIsLoading(false); - setIsGroupSettingsChanged(false); - }); - }; - fetchGroups(); - }, [notification]); - - React.useEffect(() => { - if (errorAdmin) { - notification.error(`Group error`, errorAdmin); - } - if (errorUser) { - notification.error(`Group error`, errorUser); - } - }, [errorAdmin, errorUser, notification]); - - const updateGroups = React.useCallback( - (group: GroupsConfig) => { - setIsLoading(true); - updateGroupsSettings(group) - .then((response) => { - setGroupSettings(response); - notification.success( - 'Group settings changes saved', - 'It may take up to 2 minutes for configuration changes to be applied.', - ); - }) - .catch((error) => { - setLoadError(error); - setLoaded(false); - }) - .finally(() => { - setIsLoading(false); - setIsGroupSettingsChanged(false); - }); - }, - [notification], - ); - - return { - groupSettings, - loaded, - isLoading, - isGroupSettingsChanged, - loadError, - updateGroups, - setGroupSettings, - setIsGroupSettingsChanged, - }; -}; diff --git a/manifests/core-bases/base/cluster-role.yaml b/manifests/core-bases/base/cluster-role.yaml index d2c172e765..2ec2c00b34 100644 --- a/manifests/core-bases/base/cluster-role.yaml +++ b/manifests/core-bases/base/cluster-role.yaml @@ -219,3 +219,9 @@ rules: - '' resources: - endpoints + - verbs: + - get + apiGroups: + - 'services.platform.opendatahub.io' + resources: + - auths