-
Notifications
You must be signed in to change notification settings - Fork 169
feat: Implement querying openedx-authz for publish permissions #2685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
f57976e
feb5cee
5f64fb3
d7b1a3c
e5f10d2
03a8d7d
b21a520
35d53ab
f7c566f
129e771
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| export const CONTENT_LIBRARY_PERMISSIONS = { | ||
| DELETE_LIBRARY: 'content_libraries.delete_library', | ||
| MANAGE_LIBRARY_TAGS: 'content_libraries.manage_library_tags', | ||
| VIEW_LIBRARY: 'content_libraries.view_library', | ||
|
|
||
| EDIT_LIBRARY_CONTENT: 'content_libraries.edit_library_content', | ||
| PUBLISH_LIBRARY_CONTENT: 'content_libraries.publish_library_content', | ||
| REUSE_LIBRARY_CONTENT: 'content_libraries.reuse_library_content', | ||
|
|
||
| CREATE_LIBRARY_COLLECTION: 'content_libraries.create_library_collection', | ||
| EDIT_LIBRARY_COLLECTION: 'content_libraries.edit_library_collection', | ||
| DELETE_LIBRARY_COLLECTION: 'content_libraries.delete_library_collection', | ||
|
|
||
| MANAGE_LIBRARY_TEAM: 'content_libraries.manage_library_team', | ||
| VIEW_LIBRARY_TEAM: 'content_libraries.view_library_team', | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; | ||
| import { PermissionValidationRequest, PermissionValidationResponse } from '@src/authz/types'; | ||
| import { getApiUrl } from './utils'; | ||
|
|
||
| export const validateUserPermissions = async ( | ||
| validations: PermissionValidationRequest[], | ||
| ): Promise<PermissionValidationResponse[]> => { | ||
| const { data } = await getAuthenticatedHttpClient().post(getApiUrl('/api/authz/v1/permissions/validate/me'), validations); | ||
| return data; | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import { act, ReactNode } from 'react'; | ||
| import { renderHook, waitFor } from '@testing-library/react'; | ||
| import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; | ||
| import { getAuthenticatedHttpClient } from '@edx/frontend-platform/auth'; | ||
| import { useValidateUserPermissions } from './apiHooks'; | ||
|
|
||
| jest.mock('@edx/frontend-platform/auth', () => ({ | ||
| getAuthenticatedHttpClient: jest.fn(), | ||
| })); | ||
|
|
||
| const createWrapper = () => { | ||
| const queryClient = new QueryClient({ | ||
| defaultOptions: { | ||
| queries: { | ||
| retry: false, | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const wrapper = ({ children }: { children: ReactNode }) => ( | ||
| <QueryClientProvider client={queryClient}> | ||
| {children} | ||
| </QueryClientProvider> | ||
| ); | ||
|
|
||
| return wrapper; | ||
| }; | ||
|
|
||
| const permissions = [ | ||
| { | ||
| action: 'act:read', | ||
| object: 'lib:test-lib', | ||
| scope: 'org:OpenedX', | ||
| }, | ||
| ]; | ||
|
|
||
| const mockValidPermissions = [ | ||
| { action: 'act:read', object: 'lib:test-lib', allowed: true }, | ||
| ]; | ||
|
|
||
| const mockInvalidPermissions = [ | ||
| { action: 'act:read', object: 'lib:test-lib', allowed: false }, | ||
| ]; | ||
|
|
||
| describe('useValidateUserPermissions', () => { | ||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('returns allowed true when permissions are valid', async () => { | ||
| getAuthenticatedHttpClient.mockReturnValue({ | ||
| post: jest.fn().mockResolvedValueOnce({ data: mockValidPermissions }), | ||
| }); | ||
|
|
||
| const { result } = renderHook(() => useValidateUserPermissions(permissions), { | ||
| wrapper: createWrapper(), | ||
| }); | ||
|
|
||
| await waitFor(() => expect(result.current).toBeDefined()); | ||
| await waitFor(() => expect(result.current.data).toBeDefined()); | ||
|
|
||
| expect(getAuthenticatedHttpClient).toHaveBeenCalled(); | ||
| expect(result.current.data![0].allowed).toBe(true); | ||
| }); | ||
|
|
||
| it('returns allowed false when permissions are invalid', async () => { | ||
| getAuthenticatedHttpClient.mockReturnValue({ | ||
| post: jest.fn().mockResolvedValue({ data: mockInvalidPermissions }), | ||
| }); | ||
|
|
||
| const { result } = renderHook(() => useValidateUserPermissions(permissions), { | ||
| wrapper: createWrapper(), | ||
| }); | ||
| await waitFor(() => expect(result.current).toBeDefined()); | ||
| await waitFor(() => expect(result.current.data).toBeDefined()); | ||
|
|
||
| expect(getAuthenticatedHttpClient).toHaveBeenCalled(); | ||
| expect(result.current.data![0].allowed).toBe(false); | ||
| }); | ||
|
|
||
| it('handles error when the API call fails', async () => { | ||
| const mockError = new Error('API Error'); | ||
|
|
||
| getAuthenticatedHttpClient.mockReturnValue({ | ||
| post: jest.fn().mockRejectedValue(new Error('API Error')), | ||
| }); | ||
|
|
||
| try { | ||
| act(() => { | ||
| renderHook(() => useValidateUserPermissions(permissions), { | ||
| wrapper: createWrapper(), | ||
| }); | ||
| }); | ||
| } catch (error) { | ||
| expect(error).toEqual(mockError); // Check for the expected error | ||
| } | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { useQuery } from '@tanstack/react-query'; | ||
| import { PermissionValidationRequest, PermissionValidationResponse } from '@src/authz/types'; | ||
| import { validateUserPermissions } from './api'; | ||
|
|
||
| const adminConsoleQueryKeys = { | ||
| all: ['authz'], | ||
| permissions: (permissions: PermissionValidationRequest[]) => [...adminConsoleQueryKeys.all, 'validatePermissions', permissions] as const, | ||
| }; | ||
|
|
||
| /** | ||
| * React Query hook to validate if the current user has permissions over a certain object in the instance. | ||
| * It helps to: | ||
| * - Determine whether the current user can access certain object. | ||
| * - Provide role-based rendering logic for UI components. | ||
| * | ||
| * @param permissions - The array of objects and actions to validate. | ||
| * | ||
| * @example | ||
| * const { data } = useValidateUserPermissions([{ | ||
| "action": "act:read", | ||
| "scope": "org:OpenedX" | ||
| }]); | ||
| * if (data[0].allowed) { ... } | ||
| * | ||
| */ | ||
| export const useValidateUserPermissions = ( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is minor/optional feedback about the name of this hook: To me, "validate user permissions" sounds like an action, like it would throw an exception if the user doesn't have some permissions. But this is just fetching some data, not making an action. I think "useScopedPermissions" or just "useUserPermissions" or something like that would better reflect that this is just getting the user permissions, but you still have to validate/check that they're |
||
| permissions: PermissionValidationRequest[], | ||
| ) => useQuery<PermissionValidationResponse[], Error>({ | ||
| queryKey: adminConsoleQueryKeys.permissions(permissions), | ||
| queryFn: () => validateUserPermissions(permissions), | ||
| retry: false, | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| import { getConfig } from '@edx/frontend-platform'; | ||
|
|
||
| export const getApiUrl = (path: string) => `${getConfig().LMS_BASE_URL}${path || ''}`; | ||
| export const getStudioApiUrl = (path: string) => `${getConfig().STUDIO_BASE_URL}${path || ''}`; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| export interface PermissionValidationRequest { | ||
| action: string; | ||
| scope?: string; | ||
| } | ||
|
|
||
| export interface PermissionValidationResponse extends PermissionValidationRequest { | ||
| allowed: boolean; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,13 +7,19 @@ import { | |
| useState, | ||
| } from 'react'; | ||
| import { useParams } from 'react-router-dom'; | ||
| import { useValidateUserPermissions } from '@src/authz/data/apiHooks'; | ||
| import { CONTENT_LIBRARY_PERMISSIONS } from '@src/authz/constants'; | ||
| import { ContainerType } from '../../../generic/key-utils'; | ||
|
|
||
| import type { ComponentPicker } from '../../component-picker'; | ||
| import type { ContentLibrary, BlockTypeMetadata } from '../../data/api'; | ||
| import { useContentLibrary } from '../../data/apiHooks'; | ||
| import { useComponentPickerContext } from './ComponentPickerContext'; | ||
|
|
||
| const LIBRARY_PERMISSIONS = [ | ||
| CONTENT_LIBRARY_PERMISSIONS.PUBLISH_LIBRARY_CONTENT, | ||
|
Comment on lines
+19
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| ]; | ||
|
|
||
| export interface ComponentEditorInfo { | ||
| usageKey: string; | ||
| blockType?:string | ||
|
|
@@ -25,6 +31,7 @@ export type LibraryContextData = { | |
| libraryId: string; | ||
| libraryData?: ContentLibrary; | ||
| readOnly: boolean; | ||
| canPublish: boolean; | ||
| isLoadingLibraryData: boolean; | ||
| /** The ID of the current collection/container, on the sidebar OR page */ | ||
| collectionId: string | undefined; | ||
|
|
@@ -107,6 +114,10 @@ export const LibraryProvider = ({ | |
| componentPickerMode, | ||
| } = useComponentPickerContext(); | ||
|
|
||
| const permissions = LIBRARY_PERMISSIONS.map(action => ({ action, scope: libraryId })); | ||
|
|
||
| const { isLoading: isLoadingUserPermissions, data: userPermissions } = useValidateUserPermissions(permissions); | ||
| const canPublish = userPermissions ? userPermissions[0]?.allowed : false; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something we could improve is being explicit about the action we are requesting instead of using
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the ADR, the API guarantees that the order of the response will match the requested permissions, that's why I'm not trying to match it explicitly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't know about the order 🤔 The suggestion was more about readability and clarity regarding the permission I am requesting, and I still think it is important. Since that index depends on the order of the elements in LIBRARY_PERMISSIONS, if that list grows, I don't think it will be clear enough to use only the indexes.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. Also, hard-coding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am thinking about this issue: openedx/openedx-authz#144. I haven't refined it yet, but I would probably need to add more params to the request to see other permissions.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would suggest an API like this (no change to the REST API or the internal arrays, just implement some helper logic in the hook to support this): const possiblePermissions = {
canPublish: CONTENT_LIBRARY_PERMISSIONS.PUBLISH_LIBRARY_CONTENT,
];
const {
isLoading: isLoadingUserPermissions,
data: userPermissions,
} = useScopedUserPermissions(possiblePermissions, { scope: libraryId });
// API is useScopedUserPermissions(actions object, extra fields to mix in);
const canPublish = userPermissions?.canPublish;
// or
const canPublish = userPermissions?.canPublish.allowed; // (this is more verbose, and requiring these creates security bugs whenever users forget to include `.allowed`, but if you know there will likely be other fields besides .allowed in the future, it's better to be more verbose now) |
||
| const readOnly = !!componentPickerMode || !libraryData?.canEditLibrary; | ||
|
|
||
| // Parse the initial collectionId and/or container ID(s) from the current URL params | ||
|
|
@@ -131,7 +142,8 @@ export const LibraryProvider = ({ | |
| containerId, | ||
| setContainerId, | ||
| readOnly, | ||
| isLoadingLibraryData, | ||
| canPublish, | ||
| isLoadingLibraryData: isLoadingLibraryData || isLoadingUserPermissions, | ||
| showOnlyPublished, | ||
| extraFilter, | ||
| isCreateCollectionModalOpen, | ||
|
|
@@ -154,7 +166,9 @@ export const LibraryProvider = ({ | |
| containerId, | ||
| setContainerId, | ||
| readOnly, | ||
| canPublish, | ||
| isLoadingLibraryData, | ||
| isLoadingUserPermissions, | ||
| showOnlyPublished, | ||
| extraFilter, | ||
| isCreateCollectionModalOpen, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.