diff --git a/src/hooks/useCanConfigureModel.test.tsx b/src/hooks/useCanConfigureModel.test.tsx index bae74517c..b1612c6dc 100644 --- a/src/hooks/useCanConfigureModel.test.tsx +++ b/src/hooks/useCanConfigureModel.test.tsx @@ -1,15 +1,20 @@ -import { renderHook } from "@testing-library/react"; +import { renderHook, waitFor } from "@testing-library/react"; import type { PropsWithChildren } from "react"; import { Provider } from "react-redux"; import { BrowserRouter, Route, Routes } from "react-router"; import configureStore from "redux-mock-store"; +import { JIMMRelation } from "juju/jimm/JIMMV4"; +import { actions as jujuActions } from "store/juju"; import type { RootState } from "store/store"; import { rootStateFactory } from "testing/factories"; import { generalStateFactory, - configFactory, credentialFactory, + authUserInfoFactory, + controllerFeaturesFactory, + controllerFeaturesStateFactory, + configFactory, } from "testing/factories/general"; import { modelUserInfoFactory } from "testing/factories/juju/ModelManagerV9"; import { @@ -17,12 +22,14 @@ import { modelDataFactory, modelDataInfoFactory, modelListInfoFactory, + rebacRelationFactory, } from "testing/factories/juju/juju"; import { modelWatcherModelDataFactory } from "testing/factories/juju/model-watcher"; +import { renderWrappedHook } from "testing/utils"; import useCanConfigureModel from "./useCanConfigureModel"; -const mockStore = configureStore(); +const mockStore = configureStore([]); const generateContainer = (state: RootState, path: string, url: string) => @@ -53,12 +60,7 @@ describe("useModelStatus", () => { }), controllerConnections: { "wss://jimm.jujucharms.com/api": { - user: { - "display-name": "eggman", - identity: "user-eggman@external", - "controller-access": "", - "model-access": "", - }, + user: authUserInfoFactory.build(), }, }, credentials: { @@ -83,7 +85,10 @@ describe("useModelStatus", () => { }); }); - it("should return true when user has admin access", () => { + it("should return true when juju user has admin access", () => { + if (state.general.config) { + state.general.config.isJuju = true; + } state.juju.modelData.abc123.info = modelDataInfoFactory.build({ uuid: "abc123", name: "test1", @@ -101,7 +106,10 @@ describe("useModelStatus", () => { expect(result.current).toBe(true); }); - it("should return true when user has write access", () => { + it("should return true when juju user has write access", () => { + if (state.general.config) { + state.general.config.isJuju = true; + } state.juju.modelData.abc123.info = modelDataInfoFactory.build({ uuid: "abc123", name: "test1", @@ -119,7 +127,10 @@ describe("useModelStatus", () => { expect(result.current).toBe(true); }); - it("should return false when user has read access", () => { + it("should return false when juju user has read access", () => { + if (state.general.config) { + state.general.config.isJuju = true; + } state.juju.modelData.abc123.info = modelDataInfoFactory.build({ uuid: "abc123", name: "test1", @@ -136,4 +147,74 @@ describe("useModelStatus", () => { }); expect(result.current).toBe(false); }); + + it("should request permissions for the JAAS user", async () => { + if (state.general.config) { + state.general.config.isJuju = false; + } + state.general.controllerFeatures = controllerFeaturesStateFactory.build({ + "wss://jimm.jujucharms.com/api": controllerFeaturesFactory.build({ + rebac: true, + }), + }); + const store = mockStore(state); + renderWrappedHook(() => useCanConfigureModel(), { + store, + path, + url, + }); + const action = jujuActions.checkRelation({ + tuple: { + object: "user-eggman@external", + relation: JIMMRelation.WRITER, + target_object: "model-abc123", + }, + wsControllerURL: "wss://jimm.jujucharms.com/api", + }); + await waitFor(() => { + expect( + store.getActions().find((dispatch) => dispatch.type === action.type), + ).toMatchObject(action); + }); + }); + + it("should return true when a JAAS user has write access", () => { + if (state.general.config) { + state.general.config.isJuju = false; + } + state.juju.rebacRelations = [ + rebacRelationFactory.build({ + tuple: { + object: "user-eggman@external", + relation: JIMMRelation.WRITER, + target_object: "model-abc123", + }, + allowed: true, + }), + ]; + const { result } = renderHook(() => useCanConfigureModel(), { + wrapper: generateContainer(state, path, url), + }); + expect(result.current).toBe(true); + }); + + it("should return false when a JAAS user doesn't have write access", () => { + if (state.general.config) { + state.general.config.isJuju = false; + } + state.juju.rebacRelations = [ + rebacRelationFactory.build({ + tuple: { + object: "user-eggman@external", + relation: JIMMRelation.WRITER, + target_object: "model-abc123", + }, + allowed: false, + }), + ]; + const { result } = renderHook(() => useCanConfigureModel(), { + wrapper: generateContainer(state, path, url), + }); + expect(result.current).toBe(false); + }); }); diff --git a/src/hooks/useCanConfigureModel.ts b/src/hooks/useCanConfigureModel.ts index 0c512cdea..583bc60af 100644 --- a/src/hooks/useCanConfigureModel.ts +++ b/src/hooks/useCanConfigureModel.ts @@ -2,18 +2,49 @@ import { useParams } from "react-router"; import type { EntityDetailsRoute } from "components/Routes"; import useModelStatus from "hooks/useModelStatus"; +import { useCheckPermissions } from "juju/api-hooks"; +import { JIMMRelation } from "juju/jimm/JIMMV4"; +import { getIsJuju, getControllerUserTag } from "store/general/selectors"; import { getActiveUser, getModelUUIDFromList } from "store/juju/selectors"; import { canAdministerModel } from "store/juju/utils/models"; import { useAppSelector } from "store/store"; -const useCanConfigureModel = () => { - const { userName, modelName } = useParams(); - const modelUUID = useAppSelector(getModelUUIDFromList(modelName, userName)); +const useCheckJujuPermissions = (modelUUID: string, enabled?: boolean) => { const activeUser = useAppSelector((state) => getActiveUser(state, modelUUID)); const modelStatusData = useModelStatus(); return ( - !!activeUser && canAdministerModel(activeUser, modelStatusData?.info?.users) + enabled && + !!activeUser && + canAdministerModel(activeUser, modelStatusData?.info?.users) + ); +}; + +const useCheckJIMMPermissions = ( + modelUUID: string, + enabled?: boolean, + cleanup?: boolean, +) => { + const controllerUser = useAppSelector((state) => getControllerUserTag(state)); + const { permitted } = useCheckPermissions( + enabled && controllerUser && modelUUID + ? { + object: controllerUser, + relation: JIMMRelation.WRITER, + target_object: `model-${modelUUID}`, + } + : null, + cleanup, ); + return permitted; +}; + +const useCanConfigureModel = (cleanup?: boolean) => { + const isJuju = useAppSelector(getIsJuju); + const { userName, modelName } = useParams(); + const modelUUID = useAppSelector(getModelUUIDFromList(modelName, userName)); + const jujuPermissions = useCheckJujuPermissions(modelUUID, isJuju); + const jimmPermissions = useCheckJIMMPermissions(modelUUID, !isJuju, cleanup); + return isJuju ? jujuPermissions : jimmPermissions; }; export default useCanConfigureModel; diff --git a/src/hooks/useCanManageSecrets.test.tsx b/src/hooks/useCanManageSecrets.test.tsx index 4adbcebe0..5f8b035b8 100644 --- a/src/hooks/useCanManageSecrets.test.tsx +++ b/src/hooks/useCanManageSecrets.test.tsx @@ -51,6 +51,7 @@ describe("useCanManageSecrets", () => { state = rootStateFactory.build({ general: generalStateFactory.build({ config: configFactory.build({ + isJuju: true, controllerAPIEndpoint: "wss://jimm.jujucharms.com/api", }), controllerConnections: { diff --git a/src/juju/api-hooks/permissions.test.tsx b/src/juju/api-hooks/permissions.test.tsx index 5dfdba2a8..cb1373cd4 100644 --- a/src/juju/api-hooks/permissions.test.tsx +++ b/src/juju/api-hooks/permissions.test.tsx @@ -1,6 +1,7 @@ import { renderHook, waitFor } from "@testing-library/react"; import configureStore from "redux-mock-store"; +import type { RelationshipTuple } from "juju/jimm/JIMMV4"; import { JIMMRelation, JIMMTarget } from "juju/jimm/JIMMV4"; import { actions as jujuActions } from "store/juju"; import type { RootState } from "store/store"; @@ -109,6 +110,39 @@ describe("useCheckPermissions", () => { }); }); + it("does not try and fetch a relation if the tuple object has a new reference", async () => { + const tuple = { + object: "user-eggman@external", + relation: JIMMRelation.MEMBER, + target_object: "group-admins", + }; + const store = mockStore(state); + const { rerender } = renderHook(() => useCheckPermissions(tuple), { + wrapper: (props) => ( + + ), + }); + await waitFor(() => { + expect( + store + .getActions() + .filter( + (dispatch) => dispatch.type === jujuActions.checkRelation.type, + ), + ).toHaveLength(1); + }); + rerender({ ...tuple }); + await waitFor(() => { + expect( + store + .getActions() + .filter( + (dispatch) => dispatch.type === jujuActions.checkRelation.type, + ), + ).toHaveLength(1); + }); + }); + it("does not fetch a relation that is already loading", async () => { const tuple = { object: "user-eggman@external", @@ -163,6 +197,61 @@ describe("useCheckPermissions", () => { }); }); + it("cleans up a previous relation if the tuple changes", async () => { + const tuple = { + object: "user-eggman@external", + relation: JIMMRelation.MEMBER, + target_object: "group-admins", + }; + state.juju.rebacRelations = [ + rebacRelationFactory.build({ + tuple: tuple, + loaded: true, + }), + ]; + const store = mockStore(state); + const { rerender } = renderHook< + { + permitted: boolean; + loading: boolean; + loaded: boolean; + }, + { + tupleObject: RelationshipTuple; + cleanup: boolean; + } + >( + ({ tupleObject, cleanup } = { tupleObject: tuple, cleanup: true }) => + useCheckPermissions(tupleObject, cleanup), + { + wrapper: (props) => ( + + ), + }, + ); + await waitFor(() => { + expect( + store + .getActions() + .find((dispatch) => dispatch.type === jujuActions.checkRelation.type), + ).toBeUndefined(); + }); + rerender({ tupleObject: { ...tuple, object: "newobject" }, cleanup: true }); + const action = jujuActions.removeCheckRelation({ + tuple, + }); + await waitFor(() => { + expect( + store + .getActions() + .find( + (dispatch) => + dispatch.type === jujuActions.removeCheckRelation.type, + ), + ).toMatchObject(action); + }); + }); + it("can clean up a relation", async () => { const tuple = { object: "user-eggman@external", diff --git a/src/juju/api-hooks/permissions.ts b/src/juju/api-hooks/permissions.ts index 0cb602e70..e9ae79a8f 100644 --- a/src/juju/api-hooks/permissions.ts +++ b/src/juju/api-hooks/permissions.ts @@ -1,4 +1,6 @@ -import { useEffect } from "react"; +import { usePrevious } from "@canonical/react-components"; +import fastDeepEqual from "fast-deep-equal/es6"; +import { useEffect, useRef } from "react"; import { useDispatch } from "react-redux"; import type { RelationshipTuple } from "juju/jimm/JIMMV4"; @@ -31,6 +33,8 @@ export const useCheckPermissions = ( getReBACPermissionLoading(state, tuple), ); const rebacEnabled = useAppSelector(isReBACEnabled); + const previousTuple = usePrevious(tuple, false); + const tupleChanged = !fastDeepEqual(tuple, previousTuple); useEffect(() => { if ( @@ -39,6 +43,9 @@ export const useCheckPermissions = ( !loaded && wsControllerURL && tuple && + // Ignore changes if the object has a new reference, but the values are + // the same. + tupleChanged && // Only check the relation if the controller supports rebac. rebacEnabled ) { @@ -49,19 +56,44 @@ export const useCheckPermissions = ( }), ); } - }, [dispatch, loaded, loading, rebacEnabled, tuple, wsControllerURL]); + }, [ + dispatch, + loaded, + loading, + rebacEnabled, + tuple, + wsControllerURL, + tupleChanged, + ]); - useEffect( - () => () => { - if (tuple && cleanup) { + useEffect(() => { + if (cleanup && tupleChanged && previousTuple) { + dispatch( + jujuActions.removeCheckRelation({ + tuple: previousTuple, + }), + ); + } + }, [cleanup, dispatch, previousTuple, tupleChanged]); + + const cleanupTuple = useRef<(() => void) | null>(null); + + useEffect(() => { + if (cleanup && tupleChanged && tuple) { + cleanupTuple.current = () => dispatch( jujuActions.removeCheckRelation({ tuple, }), ); - } + } + }, [cleanup, dispatch, tuple, tupleChanged]); + + useEffect( + () => () => { + cleanupTuple.current?.(); }, - [cleanup, dispatch, tuple], + [], ); return { diff --git a/src/pages/EntityDetails/App/App.test.tsx b/src/pages/EntityDetails/App/App.test.tsx index a153e514c..c4bb972fd 100644 --- a/src/pages/EntityDetails/App/App.test.tsx +++ b/src/pages/EntityDetails/App/App.test.tsx @@ -54,6 +54,7 @@ describe("Entity Details App", () => { state = rootStateFactory.build({ general: generalStateFactory.build({ config: configFactory.build({ + isJuju: true, controllerAPIEndpoint: "wss://jimm.jujucharms.com/api", }), credentials: { diff --git a/src/pages/EntityDetails/EntityDetails.tsx b/src/pages/EntityDetails/EntityDetails.tsx index b6d753380..1f90bb8a1 100644 --- a/src/pages/EntityDetails/EntityDetails.tsx +++ b/src/pages/EntityDetails/EntityDetails.tsx @@ -11,6 +11,7 @@ import NotFound from "components/NotFound"; import type { EntityDetailsRoute } from "components/Routes"; import WebCLI from "components/WebCLI"; import { useEntityDetailsParams } from "components/hooks"; +import useCanConfigureModel from "hooks/useCanConfigureModel"; import useWindowTitle from "hooks/useWindowTitle"; import BaseLayout from "layout/BaseLayout/BaseLayout"; import { getIsJuju, getUserPass } from "store/general/selectors"; @@ -50,6 +51,12 @@ const EntityDetails = ({ modelWatcherError }: Props) => { const modelInfo = useSelector(getModelInfo(modelUUID)); const { isNestedEntityPage } = useEntityDetailsParams(); + // Cleanup is set for this hook, but not for the instances of + // useCanConfigureModel in other model components as this component wraps all + // model routes so the model permissions are removed once the user navigates + // away from the model. + useCanConfigureModel(true); + const isJuju = useSelector(getIsJuju); const [showWebCLI, setShowWebCLI] = useState(false); diff --git a/src/pages/EntityDetails/Model/ApplicationsTab/LocalAppsTable/LocalAppsTable.test.tsx b/src/pages/EntityDetails/Model/ApplicationsTab/LocalAppsTable/LocalAppsTable.test.tsx index f600bafcb..acc8a4d23 100644 --- a/src/pages/EntityDetails/Model/ApplicationsTab/LocalAppsTable/LocalAppsTable.test.tsx +++ b/src/pages/EntityDetails/Model/ApplicationsTab/LocalAppsTable/LocalAppsTable.test.tsx @@ -4,7 +4,7 @@ import userEvent from "@testing-library/user-event"; import { actions as jujuActions } from "store/juju"; import type { RootState } from "store/store"; import { jujuStateFactory, rootStateFactory } from "testing/factories"; -import { generalStateFactory } from "testing/factories/general"; +import { generalStateFactory, configFactory } from "testing/factories/general"; import { charmApplicationFactory } from "testing/factories/juju/Charms"; import { modelUserInfoFactory } from "testing/factories/juju/ModelManagerV9"; import { @@ -26,6 +26,9 @@ describe("LocalAppsTable", () => { beforeEach(() => { state = rootStateFactory.build({ general: generalStateFactory.build({ + config: configFactory.build({ + isJuju: true, + }), controllerConnections: { "wss://jimm.jujucharms.com/api": { user: { diff --git a/src/pages/EntityDetails/Model/Secrets/Secrets.test.tsx b/src/pages/EntityDetails/Model/Secrets/Secrets.test.tsx index d8a6897c3..20fb91fac 100644 --- a/src/pages/EntityDetails/Model/Secrets/Secrets.test.tsx +++ b/src/pages/EntityDetails/Model/Secrets/Secrets.test.tsx @@ -55,6 +55,7 @@ describe("Secrets", () => { "wss://example.com/api": credentialFactory.build(), }, config: configFactory.build({ + isJuju: true, controllerAPIEndpoint: "wss://example.com/api", }), controllerConnections: { diff --git a/src/pages/EntityDetails/Model/Secrets/SecretsTable/SecretsTable.test.tsx b/src/pages/EntityDetails/Model/Secrets/SecretsTable/SecretsTable.test.tsx index a6a2b5619..d61e4d52f 100644 --- a/src/pages/EntityDetails/Model/Secrets/SecretsTable/SecretsTable.test.tsx +++ b/src/pages/EntityDetails/Model/Secrets/SecretsTable/SecretsTable.test.tsx @@ -50,6 +50,7 @@ describe("SecretsTable", () => { state = rootStateFactory.build({ general: generalStateFactory.build({ config: configFactory.build({ + isJuju: true, controllerAPIEndpoint: "wss://example.com/api", }), controllerConnections: { diff --git a/src/panels/ConfigPanel/ConfigPanel.test.tsx b/src/panels/ConfigPanel/ConfigPanel.test.tsx index 7794842db..df90e7030 100644 --- a/src/panels/ConfigPanel/ConfigPanel.test.tsx +++ b/src/panels/ConfigPanel/ConfigPanel.test.tsx @@ -68,6 +68,7 @@ describe("ConfigPanel", () => { state = rootStateFactory.build({ general: generalStateFactory.build({ config: configFactory.build({ + isJuju: true, controllerAPIEndpoint: "wss://example.com/api", }), controllerConnections: { diff --git a/src/panels/ConfigPanel/SecretsPicker/SecretsPicker.test.tsx b/src/panels/ConfigPanel/SecretsPicker/SecretsPicker.test.tsx index 0df123f1d..5e70ef604 100644 --- a/src/panels/ConfigPanel/SecretsPicker/SecretsPicker.test.tsx +++ b/src/panels/ConfigPanel/SecretsPicker/SecretsPicker.test.tsx @@ -43,6 +43,7 @@ describe("SecretsPicker", () => { "wss://example.com/api": credentialFactory.build(), }, config: configFactory.build({ + isJuju: true, controllerAPIEndpoint: "wss://example.com/api", }), controllerConnections: { diff --git a/src/panels/Panels.test.tsx b/src/panels/Panels.test.tsx index e5f329d64..bd9adbe95 100644 --- a/src/panels/Panels.test.tsx +++ b/src/panels/Panels.test.tsx @@ -73,6 +73,7 @@ describe("Panels", () => { state = rootStateFactory.build({ general: generalStateFactory.build({ config: configFactory.build({ + isJuju: true, controllerAPIEndpoint: "wss://jimm.jujucharms.com/api", }), controllerConnections: {