Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions shell/header/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ const config: App = {
op: WidgetOperationTypes.APPEND,
component: MasqueradeBar,
condition: {
authenticated: true,
callback: () => isCourseBarMasqueradeRoute(),
}
}
Expand Down
17 changes: 6 additions & 11 deletions shell/header/course-bar/masquerade/MasqueradeBar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -29,11 +26,9 @@ export default function MasqueradeBar() {
<div className="bg-primary text-white">
<Container fluid size="xl">
<div className="py-3 d-md-flex justify-content-end align-items-start">
{!isUnreachable && (
<div className="align-items-center flex-grow-1 d-md-flex mx-1 my-1">
<MasqueradeWidget />
</div>
)}
<div className="align-items-center flex-grow-1 d-md-flex mx-1 my-1">
<MasqueradeWidget />
</div>
<StudioLink courseId={courseId} unitId={unitId} />
</div>
</Container>
Expand Down
11 changes: 9 additions & 2 deletions shell/header/course-bar/masquerade/data/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,15 @@ export interface MasqueradePayload {

export async function getMasqueradeOptions(courseId: string): Promise<MasqueradeStatus> {
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<MasqueradeStatus> {
Expand Down
19 changes: 4 additions & 15 deletions shell/header/course-bar/masquerade/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Comment on lines +71 to 77
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description says

The LMS's 302-to-login redirect for denied masquerade requests is detected via the final response URL and surfaced as a 403-shaped error

That doesn't seem to match up with the change to deny on all errors here. Am I looking in the wrong spot?

Copy link
Copy Markdown
Contributor Author

@arbrandes arbrandes May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Yeah, the changes in api.ts match the PR description.

I guess that changes my question to: "why is isQueryDenied changing?"

The previous logic matched the previous comment

Other failures (network, 5xx) are treated as "couldn't load" and surface as an alert.

Is that not desired behavior?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's not desired. This is because, as I just realized when fixing this, one of the error modes - "couldn't load" - also happens when simply being logged out. This is because the backend returns a 302 instead of a 403 (why it would do that for an XHR request, I don't even dare guess), and, not knowing what to do with it, React Query just treats it as a network failure, renders the bar, and then the alert.

This made me think that any error except a mutation error should result in the bar not showing, because otherwise, a regular learner would suddenly be able to see the bar because of an actual network failure.

return query.data !== undefined && !query.data.success;
}
Expand All @@ -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
Expand All @@ -127,7 +120,6 @@ export interface MasqueradeState {
isSubmitting: boolean,
isLoading: boolean,
isDenied: boolean,
isUnreachable: boolean,
}

export function useMasqueradeState(courseId: string): MasqueradeState {
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -262,6 +252,5 @@ export function useMasqueradeState(courseId: string): MasqueradeState {
isSubmitting: mutation.isPending,
isLoading,
isDenied,
isUnreachable,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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 () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ function buildContextValue(
isSubmitting: false,
isLoading: false,
isDenied: false,
isUnreachable: false,
...overrides,
};
}
Expand Down
5 changes: 0 additions & 5 deletions shell/header/course-bar/masquerade/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.',
Expand Down