diff --git a/shell/header/app.tsx b/shell/header/app.tsx index 375fbded..a810df93 100644 --- a/shell/header/app.tsx +++ b/shell/header/app.tsx @@ -155,6 +155,7 @@ const config: App = { op: WidgetOperationTypes.APPEND, component: MasqueradeBar, condition: { + authenticated: true, callback: () => isCourseBarMasqueradeRoute(), } } diff --git a/shell/header/course-bar/masquerade/MasqueradeBar.tsx b/shell/header/course-bar/masquerade/MasqueradeBar.tsx index dfd65286..6a8d22d1 100644 --- a/shell/header/course-bar/masquerade/MasqueradeBar.tsx +++ b/shell/header/course-bar/masquerade/MasqueradeBar.tsx @@ -12,13 +12,10 @@ export default function MasqueradeBar() { const { formatMessage } = useIntl(); const { courseId = '', unitId = '' } = useParams(); const masquerade = useMasqueradeState(courseId); - const { - errorMessage, isLoading, isDenied, isUnreachable, - } = masquerade; + const { errorMessage, isLoading, isDenied } = masquerade; - /* Render nothing while we wait for the first response, and when the server - * tells us this user can't masquerade. Other failures fall through to a - * partial bar plus an alert. */ + /* Render nothing while we wait for the first response, and when the load + * fails or the server says this user can't masquerade. */ if (isLoading || isDenied) { return null; } @@ -29,11 +26,9 @@ export default function MasqueradeBar() {
- {!isUnreachable && ( -
- -
- )} +
+ +
diff --git a/shell/header/course-bar/masquerade/data/api.ts b/shell/header/course-bar/masquerade/data/api.ts index ca2ee83b..39d40fdf 100644 --- a/shell/header/course-bar/masquerade/data/api.ts +++ b/shell/header/course-bar/masquerade/data/api.ts @@ -34,8 +34,15 @@ export interface MasqueradePayload { export async function getMasqueradeOptions(courseId: string): Promise { const url = `${getSiteConfig().lmsBaseUrl}/courses/${courseId}/masquerade`; - const { data } = await getAuthenticatedHttpClient().get(url, {}); - return camelCaseObject(data); + const response = await getAuthenticatedHttpClient().get(url, {}); + /* The LMS 302s to /login instead of returning 403 when masquerade is denied. */ + if (response.request?.responseURL && response.request.responseURL !== url) { + const error = Object.assign(new Error(`Masquerade endpoint redirected to ${response.request.responseURL}`), { + customAttributes: { httpErrorStatus: 403 }, + }); + throw error; + } + return camelCaseObject(response.data); } export async function postMasqueradeOptions(courseId: string, payload: MasqueradePayload): Promise { diff --git a/shell/header/course-bar/masquerade/hooks.ts b/shell/header/course-bar/masquerade/hooks.ts index 34aeb151..dddebf9a 100644 --- a/shell/header/course-bar/masquerade/hooks.ts +++ b/shell/header/course-bar/masquerade/hooks.ts @@ -68,14 +68,12 @@ function getHttpStatus(error: unknown): number | undefined { } /* - * The server tells us "no, you can't masquerade here" via either HTTP 403 or - * a 200 with `success: false`. Both mean: hide the bar entirely. Other - * failures (network, 5xx) are treated as "couldn't load" and surface as an - * alert. + * Anything other than a successful load with `success: true` hides the bar: + * 403, 200 with `success: false`, network errors, 5xx, redirected requests. */ function isQueryDenied(query: { isError: boolean, error: unknown, data: MasqueradeStatus | undefined }): boolean { if (query.isError) { - return getHttpStatus(query.error) === 403; + return true; } return query.data !== undefined && !query.data.success; } @@ -96,13 +94,8 @@ export function formatErrorMessage( } function pickErrorMessage( - query: { isError: boolean, error: unknown, data: MasqueradeStatus | undefined }, mutation: { isError: boolean, error: unknown, data: MasqueradeStatus | undefined }, ): MasqueradeErrorMessage | null { - /* Denial is handled by hiding the bar; only surface truly-failed loads here. */ - if (query.isError && getHttpStatus(query.error) !== 403) { - return messages.failedToLoadOptions; - } if (mutation.isError) { return getHttpStatus(mutation.error) === 404 ? messages.noStudentFound @@ -127,7 +120,6 @@ export interface MasqueradeState { isSubmitting: boolean, isLoading: boolean, isDenied: boolean, - isUnreachable: boolean, } export function useMasqueradeState(courseId: string): MasqueradeState { @@ -227,10 +219,8 @@ export function useMasqueradeState(courseId: string): MasqueradeState { const showUserNameInput = active.userName !== null || pendingOption?.userName !== undefined; - const errorMessage = pickErrorMessage(query, mutation); + const errorMessage = pickErrorMessage(mutation); const isDenied = isQueryDenied(query); - /* Query landed but we couldn't load options — bar shows but widget hides. */ - const isUnreachable = query.isError && getHttpStatus(query.error) !== 403; /* Loading until the first response (success or failure) lands. */ const isLoading = query.isLoading; @@ -262,6 +252,5 @@ export function useMasqueradeState(courseId: string): MasqueradeState { isSubmitting: mutation.isPending, isLoading, isDenied, - isUnreachable, }; } diff --git a/shell/header/course-bar/masquerade/masquerade-widget/MasqueradeWidget.test.tsx b/shell/header/course-bar/masquerade/masquerade-widget/MasqueradeWidget.test.tsx index 169518cb..4c77e358 100644 --- a/shell/header/course-bar/masquerade/masquerade-widget/MasqueradeWidget.test.tsx +++ b/shell/header/course-bar/masquerade/masquerade-widget/MasqueradeWidget.test.tsx @@ -266,15 +266,13 @@ describe('MasqueradeWidget', () => { ).toBeInTheDocument(); }); - it('shows an alert when masquerade options fail to load', async () => { + it('hides the bar entirely on a generic load failure', async () => { mockGetMasqueradeOptions.mockRejectedValue(new Error('Boom')); renderWidget(); - expect( - await screen.findByText(/Unable to load masquerade options/), - ).toBeInTheDocument(); - expect(screen.queryByRole('button', { name: 'Staff' })).not.toBeInTheDocument(); + await waitFor(() => expect(mockGetMasqueradeOptions).toHaveBeenCalled()); + expect(screen.queryByRole('region', { name: /masquerade bar/i })).not.toBeInTheDocument(); }); it('hides the bar entirely when the server returns success: false', async () => { @@ -284,7 +282,6 @@ describe('MasqueradeWidget', () => { await waitFor(() => expect(mockGetMasqueradeOptions).toHaveBeenCalled()); expect(screen.queryByRole('region', { name: /masquerade bar/i })).not.toBeInTheDocument(); - expect(screen.queryByText(/Unable to load masquerade options/)).not.toBeInTheDocument(); }); it('hides the bar entirely on 403', async () => { diff --git a/shell/header/course-bar/masquerade/masquerade-widget/MasqueradeWidgetOption.test.tsx b/shell/header/course-bar/masquerade/masquerade-widget/MasqueradeWidgetOption.test.tsx index 6a36a85c..5c8c753d 100644 --- a/shell/header/course-bar/masquerade/masquerade-widget/MasqueradeWidgetOption.test.tsx +++ b/shell/header/course-bar/masquerade/masquerade-widget/MasqueradeWidgetOption.test.tsx @@ -31,7 +31,6 @@ function buildContextValue( isSubmitting: false, isLoading: false, isDenied: false, - isUnreachable: false, ...overrides, }; } diff --git a/shell/header/course-bar/masquerade/messages.ts b/shell/header/course-bar/masquerade/messages.ts index 62823820..52fae7d2 100644 --- a/shell/header/course-bar/masquerade/messages.ts +++ b/shell/header/course-bar/masquerade/messages.ts @@ -16,11 +16,6 @@ const messages = defineMessages({ defaultMessage: 'Studio', description: 'Button to view in studio', }, - failedToLoadOptions: { - id: 'masqueradeBar.error.failedToLoadOptions', - defaultMessage: 'Unable to load masquerade options.', - description: 'Error shown when masquerade options cannot be retrieved from the server.', - }, noStudentFound: { id: 'masqueradeBar.error.noStudentFound', defaultMessage: 'No student with this username or email could be found.',