diff --git a/README-keycloak.md b/README-keycloak.md index e4df9bb0c0..da4cfc2ce1 100644 --- a/README-keycloak.md +++ b/README-keycloak.md @@ -37,19 +37,27 @@ be sure that you have the following set in your .env file: start the app without the profile, you can still start Keycloak later by specifying the profile.) +`DISABLE_APISIX_USER_MIDDLEWARE=True` is the default in `env/backend.env` +(without APISIX actually running, nothing verifies the `X-Userinfo` header +Django would otherwise trust, so this is disabled unless you opt in). Add +`DISABLE_APISIX_USER_MIDDLEWARE=False` to your `backend.local.env` file so +Django will trust the header APISIX sets after a real Keycloak login. + When you run `docker compose up`, the Keycloak and APISIX containers should start up. APISIX is on port 8065, Keycloak on port 8066. Now you should be able to log in at `https://open.odl.local:8065/login` with one of the users mentioned above, or just click "Log in" from the home page at http://open.odl.local:8062. Try logging out and back in a couple times to make sure it works. -Keycloak is enabled by default. If you do NOT want to use the Keycloak and APISIX instances, -follow these steps: +Not using Keycloak/APISIX is the default (`DISABLE_APISIX_USER_MIDDLEWARE=True`, +`COMPOSE_PROFILES=backend,frontend`) -- no extra steps needed. If you've +already switched into Keycloak/APISIX mode above and want to switch back: 1. Change the value of `MITOL_API_BASE_URL` to `http://api.open.odl.local:8063` in your `shared.local.env` file. -2. Add `DISABLE_APISIX_USER_MIDDLEWARE=True` to your `backend.local.env` file -3. Set `COMPOSE_PROFILES=backend,frontend` in your .env file +2. Set `COMPOSE_PROFILES=backend,frontend` in your .env file +3. Remove (or set to `True`) `DISABLE_APISIX_USER_MIDDLEWARE` in your + `backend.local.env` file ### Changing email and password diff --git a/RELEASE.rst b/RELEASE.rst index 7589c385aa..fa209d7ec3 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,16 @@ Release Notes ============= +Version 0.81.4 +-------------- + +- gate learner analytics on its own feature flag (#4018) +- Require B2B data consent on the contract dashboard (#4006) +- Stop the first autosave navigating out of the editor (#4016) +- fix(webhooks): stop leaking the real OCW_WEBHOOK_KEY into logs/Sentry (#3988) +- fix(auth): disable ApisixUserMiddleware by default in local dev/codespaces (#4002) +- fix(b2b): show learner email and icon avatar when name is blank (#3992) + Version 0.81.3 -------------- diff --git a/env/backend.env b/env/backend.env index e0f8cf4468..f12b5a7908 100644 --- a/env/backend.env +++ b/env/backend.env @@ -49,6 +49,13 @@ TIKA_SERVER_ENDPOINT=http://tika:9998/ TIKA_CLIENT_ONLY=True # APISIX/Keycloak settings +# The default local dev profile (COMPOSE_PROFILES=backend,frontend) doesn't +# run APISIX/Keycloak, so nothing is actually verifying the X-Userinfo header +# ApisixUserMiddleware trusts -- a request straight to Django could forge it +# to log in as anyone. Disable the middleware by default; opting into real +# Keycloak/APISIX testing (COMPOSE_PROFILES=...,keycloak,apisix) needs to set +# this back to False. See README-keycloak.md. +DISABLE_APISIX_USER_MIDDLEWARE=True APISIX_LOGOUT_URL=http://api.open.odl.local:8065/logout/ APISIX_SESSION_SECRET_KEY=supertopsecret1234 KC_SPI_THEME_WELCOME_THEME=scim diff --git a/env/codespaces.env b/env/codespaces.env index 93189f66af..34cab1633b 100644 --- a/env/codespaces.env +++ b/env/codespaces.env @@ -1,3 +1,8 @@ +# Codespaces doesn't run APISIX/Keycloak, and its ports are forwardable +# (network-reachable) by design -- so nothing verifies the X-Userinfo header +# ApisixUserMiddleware would otherwise trust. Disable it here for the same +# reason as env/backend.env. +DISABLE_APISIX_USER_MIDDLEWARE=True MITOL_SUPPORT_EMAIL=support@localhost POSTHOG_TIMEOUT_MS=1500 MAILGUN_KEY=test diff --git a/frontends/api/package.json b/frontends/api/package.json index 2f04a48002..e03eae347e 100644 --- a/frontends/api/package.json +++ b/frontends/api/package.json @@ -36,7 +36,7 @@ }, "dependencies": { "@mitodl/mit-learn-api-axios": "2026.8.17", - "@mitodl/mitxonline-api-axios": "2026.9.23", + "@mitodl/mitxonline-api-axios": "2026.9.29", "@tanstack/react-query": "^5.66.0", "axios": "^1.12.2", "tiny-invariant": "^1.3.3" diff --git a/frontends/api/src/mitxonline/hooks/organizations/index.ts b/frontends/api/src/mitxonline/hooks/organizations/index.ts index 7e7f538783..c48511d647 100644 --- a/frontends/api/src/mitxonline/hooks/organizations/index.ts +++ b/frontends/api/src/mitxonline/hooks/organizations/index.ts @@ -2,6 +2,7 @@ import { useMutation, useQueryClient } from "@tanstack/react-query" import { b2bApi } from "../../clients" import { B2bApiB2bAttachCreateRequest, + B2bApiB2bDataConsentCreateRequest, B2bApiB2bManagerOrganizationsContractsCodesBulkAssignCreateRequest, B2bApiB2bManagerOrganizationsContractsCodesReassignUpdateRequest, B2bApiB2bManagerOrganizationsContractsCodesRemindCreateRequest, @@ -9,6 +10,7 @@ import { B2bApiB2bManagerOrganizationsContractsCodesSendTestEmailCreateRequest, } from "@mitodl/mitxonline-api-axios/v2" import { managerOrganizationQueries, managerOrganizationKeys } from "./queries" +import { mitxUserQueries } from "../user" import type { MutationHookOptions } from "../../../mutations/mutationMeta" const useB2BAttachMutation = ( @@ -28,6 +30,20 @@ const useB2BAttachMutation = ( }) } +const useDataConsentMutation = ({ meta }: MutationHookOptions = {}) => { + const queryClient = useQueryClient() + return useMutation({ + mutationFn: (opts: B2bApiB2bDataConsentCreateRequest) => + b2bApi.b2bDataConsentCreate(opts), + // Returned so the mutation stays pending until users/me has the new value. + onSuccess: () => + queryClient.invalidateQueries({ + queryKey: mitxUserQueries.me().queryKey, + }), + meta, + }) +} + /** * Bulk-assign available enrollment codes to a list of email addresses. Codes are * auto-allocated by the backend (one per record); the response reports which @@ -140,6 +156,7 @@ const useSendTestEmail = ({ meta }: MutationHookOptions = {}) => export { managerOrganizationQueries, useB2BAttachMutation, + useDataConsentMutation, useBulkAssignSeats, useReassignCode, useRemindCode, diff --git a/frontends/api/src/mitxonline/test-utils/factories/contracts.ts b/frontends/api/src/mitxonline/test-utils/factories/contracts.ts index 1b1b71cb75..f5eaf7424a 100644 --- a/frontends/api/src/mitxonline/test-utils/factories/contracts.ts +++ b/frontends/api/src/mitxonline/test-utils/factories/contracts.ts @@ -2,13 +2,15 @@ import { faker } from "@faker-js/faker/locale/en" import type { BulkAssignError, BulkAssignResult, - ContractPage, ManagerEnrollmentCode, PaginatedManagerEnrollmentCodeList, + UserContractPage, } from "@mitodl/mitxonline-api-axios/v2" import { makePaginatedFactory } from "ol-test-utilities" -const contract = (overrides: Partial = {}): ContractPage => ({ +const contract = ( + overrides: Partial = {}, +): UserContractPage => ({ id: faker.number.int(), contract_end: faker.date.future().toISOString(), contract_start: faker.date.past().toISOString(), @@ -20,6 +22,7 @@ const contract = (overrides: Partial = {}): ContractPage => ({ welcome_message: faker.lorem.sentence(), welcome_message_extra: `

${faker.lorem.paragraph()}

`, programs: [], + consented_to_data_sharing: null, ...overrides, variant_options: overrides.variant_options ?? [], }) diff --git a/frontends/api/src/mitxonline/test-utils/factories/organization.ts b/frontends/api/src/mitxonline/test-utils/factories/organization.ts index 8be6c6735e..12f3c05926 100644 --- a/frontends/api/src/mitxonline/test-utils/factories/organization.ts +++ b/frontends/api/src/mitxonline/test-utils/factories/organization.ts @@ -1,10 +1,10 @@ import { faker } from "@faker-js/faker/locale/en" -import { OrganizationPage } from "@mitodl/mitxonline-api-axios/v2" +import { UserOrganizationPage } from "@mitodl/mitxonline-api-axios/v2" import { mergeOverrides } from "ol-test-utilities" const organization = ( - overrides: Partial, -): OrganizationPage => { + overrides: Partial, +): UserOrganizationPage => { const merged = mergeOverrides( { id: faker.number.int(), diff --git a/frontends/api/src/mitxonline/test-utils/urls.ts b/frontends/api/src/mitxonline/test-utils/urls.ts index 324273e9d6..45967ddb71 100644 --- a/frontends/api/src/mitxonline/test-utils/urls.ts +++ b/frontends/api/src/mitxonline/test-utils/urls.ts @@ -41,6 +41,8 @@ const programEnrollments = { const b2b = { courseEnrollment: (readableId?: string) => `${getApiBaseUrl()}/api/v0/b2b/enroll/${readableId}/`, + dataConsent: (contractId: number) => + `${getApiBaseUrl()}/api/v0/b2b/data_consent/${contractId}/`, } const programs = { diff --git a/frontends/main/package.json b/frontends/main/package.json index c5d936e91d..2eece37f14 100644 --- a/frontends/main/package.json +++ b/frontends/main/package.json @@ -18,7 +18,7 @@ "@mitodl/arithmix": "^0.2.5", "@mitodl/course-search-utils": "^3.8.1", "@mitodl/hacksnack": "^0.1.2", - "@mitodl/mitxonline-api-axios": "2026.9.23", + "@mitodl/mitxonline-api-axios": "2026.9.29", "@mitodl/smoot-design": "6.38.0", "@mui/base": "5.0.0-beta.70", "@mui/material": "^6.4.5", diff --git a/frontends/main/src/app-pages/ContractLearnersPage/ContractLearnersPage.test.tsx b/frontends/main/src/app-pages/ContractLearnersPage/ContractLearnersPage.test.tsx index 8e8e561c96..367705fc15 100644 --- a/frontends/main/src/app-pages/ContractLearnersPage/ContractLearnersPage.test.tsx +++ b/frontends/main/src/app-pages/ContractLearnersPage/ContractLearnersPage.test.tsx @@ -13,7 +13,9 @@ import { import { useFeatureFlagEnabled } from "posthog-js/react" import { allowConsoleErrors } from "ol-test-utilities" import { ForbiddenError } from "@/common/errors" +import { FeatureFlags } from "@/common/feature_flags" import { useFeatureFlagsLoaded } from "@/common/useFeatureFlagsLoaded" +import { contractAnalyticsView } from "@/common/urls" import ContractLearnersPage from "./ContractLearnersPage" jest.mock("posthog-js/react", () => ({ @@ -48,6 +50,23 @@ const setup = () => { return { org, contract, orgSlug: org.slug.replace(/^org-/, "") } } +/** + * A managed org with no analytics org ID, so the page renders its chrome + * without firing any analytics request โ€” enough for the back link. + */ +const setupUnqueryable = () => { + const contract = mitxFactories.contracts.contract() + const org = mitxFactories.organizations.organization({ + contracts: [contract], + sso_organization_id: null, + }) + setMockResponse.get( + mitxUrls.organization.managerOrganizationsList(), + paginate([org]), + ) + return { org, contract, orgSlug: org.slug.replace(/^org-/, "") } +} + /** * The page fires one list query plus one unfiltered total-count query, used * for the "X of Y enrollments" summary text. Mocking by exact URL keeps the @@ -107,7 +126,7 @@ describe("ContractLearnersPage", () => { ) }) - test("throws ForbiddenError when the analytics flag is off", () => { + test("throws ForbiddenError when the learner analytics flag is off", () => { mockedUseFeatureFlagEnabled.mockReturnValue(false) allowConsoleErrors() @@ -118,6 +137,55 @@ describe("ContractLearnersPage", () => { ).toThrow(ForbiddenError) }) + test("the aggregate analytics flag alone does not open this page", () => { + // The two dashboards roll out independently: aggregate analytics must not + // confer access to per-learner data. + mockedUseFeatureFlagEnabled.mockImplementation( + (flag) => flag === FeatureFlags.B2BAnalyticsDashboard, + ) + allowConsoleErrors() + + expect(() => + renderWithProviders( + , + ), + ).toThrow(ForbiddenError) + }) + + test("the learner flag alone does not open this page", () => { + // Learner analytics is nested inside the analytics rollout: the back link + // and framing assume the aggregate page is reachable. + mockedUseFeatureFlagEnabled.mockImplementation( + (flag) => flag === FeatureFlags.B2BLearnerAnalytics, + ) + allowConsoleErrors() + + expect(() => + renderWithProviders( + , + ), + ).toThrow(ForbiddenError) + }) + + test("opens with both analytics flags on, linking back to aggregate analytics", async () => { + mockedUseFeatureFlagEnabled.mockImplementation( + (flag) => + flag === FeatureFlags.B2BAnalyticsDashboard || + flag === FeatureFlags.B2BLearnerAnalytics, + ) + const { contract, orgSlug } = setupUnqueryable() + + renderWithProviders( + , + ) + + const back = await screen.findByRole("link", { name: /Program analytics/ }) + expect(back).toHaveAttribute( + "href", + contractAnalyticsView(orgSlug, contract.slug), + ) + }) + test("denies access when the org is not one the user manages", async () => { setMockResponse.get( mitxUrls.organization.managerOrganizationsList(), @@ -212,6 +280,67 @@ describe("ContractLearnersPage", () => { expect(within(row).getByText("Certificate")).toBeInTheDocument() }) + test("a learner row shows the email alongside the name", async () => { + const { org, contract, orgSlug } = setup() + const contractId = String(contract.id) + setMockResponse.get( + mitxUrls.organization.managerOrganizationsList(), + paginate([org]), + ) + mockTotal(contractId, 1) + mockList(contractId, [ + analyticsFactories.learnerProgress({ + full_name: "Anton Petrov", + email: "anton@example.com", + courserun_title: "Module 5", + }), + ]) + + renderWithProviders( + , + ) + + const name = await screen.findByText("Anton Petrov") + const row = rowOf(name) + expect(within(row).getByText("anton@example.com")).toBeInTheDocument() + expect(within(row).getByText("AP")).toBeInTheDocument() + }) + + test.each([ + { fullName: null, label: "null" }, + { fullName: "", label: "empty" }, + { fullName: " ", label: "whitespace-only" }, + ])( + "a learner with a $label name shows only the email and an icon avatar", + async ({ fullName }) => { + const { org, contract, orgSlug } = setup() + const contractId = String(contract.id) + setMockResponse.get( + mitxUrls.organization.managerOrganizationsList(), + paginate([org]), + ) + mockTotal(contractId, 1) + mockList(contractId, [ + analyticsFactories.learnerProgress({ + full_name: fullName, + email: "x7k2m@example.com", + courserun_title: "Module 5", + }), + ]) + + renderWithProviders( + , + ) + + const email = await screen.findByText("x7k2m@example.com") + const row = rowOf(email) + expect(within(row).getByText("Module 5")).toBeInTheDocument() + expect(within(row).queryByText("?")).not.toBeInTheDocument() + expect(within(row).queryByText("Unknown learner")).not.toBeInTheDocument() + expect(row.querySelector("[aria-hidden='true'] svg")).not.toBeNull() + }, + ) + test("a learner who withheld consent shows No consent given", async () => { const { org, contract, orgSlug } = setup() const contractId = String(contract.id) diff --git a/frontends/main/src/app-pages/ContractLearnersPage/ContractLearnersPage.tsx b/frontends/main/src/app-pages/ContractLearnersPage/ContractLearnersPage.tsx index 079126b6d2..843f271adf 100644 --- a/frontends/main/src/app-pages/ContractLearnersPage/ContractLearnersPage.tsx +++ b/frontends/main/src/app-pages/ContractLearnersPage/ContractLearnersPage.tsx @@ -898,7 +898,13 @@ const ContractLearnersPageInternal: React.FC = ({ const ContractLearnersPage: React.FC = (props) => { const flagsLoaded = useFeatureFlagsLoaded() - const enabled = useFeatureFlagEnabled(FeatureFlags.B2BAnalyticsDashboard) + const analyticsEnabled = useFeatureFlagEnabled( + FeatureFlags.B2BAnalyticsDashboard, + ) + const learnerAnalyticsEnabled = useFeatureFlagEnabled( + FeatureFlags.B2BLearnerAnalytics, + ) + const enabled = analyticsEnabled && learnerAnalyticsEnabled if (!flagsLoaded) { return ( diff --git a/frontends/main/src/app-pages/ContractLearnersPage/LearnerRow.tsx b/frontends/main/src/app-pages/ContractLearnersPage/LearnerRow.tsx index 8d9faaa0a1..729cb1bc0e 100644 --- a/frontends/main/src/app-pages/ContractLearnersPage/LearnerRow.tsx +++ b/frontends/main/src/app-pages/ContractLearnersPage/LearnerRow.tsx @@ -1,6 +1,7 @@ "use client" import React from "react" +import { RiUserLine } from "@remixicon/react" import { styled, Typography } from "ol-components" import { initials } from "ol-utilities" import type { LearnerProgress } from "api/analytics-hooks/organizations" @@ -68,8 +69,9 @@ import { COLUMN_FLEX } from "./columns" // -------------------------------------------------------------------------- /** - * Initials, not a photo: the analytics API returns no avatar image, and a name - * is the only identity it carries. + * Initials, not a photo: the analytics API returns no avatar image. Without a + * name, a generic person icon โ€” email-derived initials are wrong for addresses + * like `jdoe@` or `x7k2m@`. */ const Avatar = styled.div(({ theme }) => ({ display: "flex", @@ -97,6 +99,13 @@ const LearnerName = styled(Typography)(({ theme }) => ({ textOverflow: "ellipsis", })) as typeof Typography +const LearnerEmail = styled(Typography)(({ theme }) => ({ + ...theme.typography.body3, + color: theme.custom.colors.darkGray2, + overflow: "hidden", + textOverflow: "ellipsis", +})) as typeof Typography + const CourseTitle = styled(Typography)(({ theme }) => ({ ...theme.typography.body3, color: theme.custom.colors.silverGrayDark, @@ -184,7 +193,7 @@ const LearnerRow: React.FC = ({ row }) => { const status = getDisplayStatus(row) const statusLabel = DISPLAY_STATUS_LABEL[status] const isWithheld = status === "not-shared" - const name = row.full_name ?? row.email ?? "Unknown learner" + const name = row.full_name?.trim() || null return ( @@ -195,7 +204,7 @@ const LearnerRow: React.FC = ({ row }) => { checked={selected} onChange={() => onToggleSelect(rowId)} inputProps={{ - "aria-label": `Select ${name}, ${row.courserun_title}`, + "aria-label": `Select ${name ?? row.email ?? "learner"}, ${row.courserun_title}`, }} /> @@ -203,10 +212,13 @@ const LearnerRow: React.FC = ({ row }) => { - {name} + {name && {name}} + {row.email && ( + {row.email} + )} {row.courserun_title} diff --git a/frontends/main/src/app-pages/DashboardPage/AnalyticsContent.test.tsx b/frontends/main/src/app-pages/DashboardPage/AnalyticsContent.test.tsx index 512a3d01b4..28f22602e8 100644 --- a/frontends/main/src/app-pages/DashboardPage/AnalyticsContent.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/AnalyticsContent.test.tsx @@ -9,7 +9,7 @@ import { import { waitFor, within } from "@testing-library/react" import userEvent from "@testing-library/user-event" import type { AxiosError } from "axios" -import type { OrganizationPage } from "@mitodl/mitxonline-api-axios/v2" +import type { UserOrganizationPage } from "@mitodl/mitxonline-api-axios/v2" import { useFeatureFlagEnabled } from "posthog-js/react" import { allowConsoleErrors } from "ol-test-utilities" import { ForbiddenError } from "@/common/errors" @@ -57,7 +57,7 @@ const managerOrgsUrl = urls.organization.managerOrganizationsList() const ORG_UUID = "3fa85f64-5717-4562-b3fc-2c963f66afa6" const orgWithUuid = ( - overrides: Partial = {}, + overrides: Partial = {}, ssoOrganizationId: string | null = ORG_UUID, ) => factories.organizations.organization({ @@ -920,6 +920,29 @@ describe("AnalyticsContent, contract-scoped", () => { ) }) + test("hides 'Learner analytics' when its own flag is off", async () => { + // The learner page throws ForbiddenError without its flag, so the button + // would be a dead end. + mockedUseFeatureFlagEnabled.mockImplementation( + (flag) => flag !== FeatureFlags.B2BLearnerAnalytics, + ) + const contract = factories.contracts.contract() + const org = orgWithUuid({ contracts: [contract] }) + setManagerOrgs([org]) + + setContractAnalyticsResponses(String(contract.id)) + + const orgSlug = org.slug.replace(/^org-/, "") + renderWithProviders( + , + ) + + await screen.findByText(`Analytics ยท ${contract.name}`) + expect( + screen.queryByRole("link", { name: "Learner analytics" }), + ).not.toBeInTheDocument() + }) + test("hides 'Learner analytics' on the org-wide aggregate page", async () => { // learner-progress is contract-scoped only, so there is nowhere for this // button to point without a contract in view. diff --git a/frontends/main/src/app-pages/DashboardPage/AnalyticsContent.tsx b/frontends/main/src/app-pages/DashboardPage/AnalyticsContent.tsx index cc0bf8c6bb..9d0adffb6a 100644 --- a/frontends/main/src/app-pages/DashboardPage/AnalyticsContent.tsx +++ b/frontends/main/src/app-pages/DashboardPage/AnalyticsContent.tsx @@ -296,6 +296,9 @@ const AnalyticsContentInternal: React.FC = ({ const managerDashboardFlag = useFeatureFlagEnabled( FeatureFlags.B2BContractManagerDashboard, ) + const learnerAnalyticsFlag = useFeatureFlagEnabled( + FeatureFlags.B2BLearnerAnalytics, + ) const { data: managerOrgs, isLoading: isLoadingOrgs, @@ -495,9 +498,10 @@ const AnalyticsContentInternal: React.FC = ({ - {(contract || (manageSeatsSlug && managerDashboardFlag)) && ( + {((contract && learnerAnalyticsFlag) || + (manageSeatsSlug && managerDashboardFlag)) && ( - {contract ? ( + {contract && learnerAnalyticsFlag ? ( { @@ -1574,6 +1578,94 @@ describe("ContractContent", () => { ) }) + test("the learner analytics button follows its own flag, not the aggregate one", async () => { + // Aggregate analytics on, learner analytics off: the learner page 403s + // without its flag, so only "View analytics" may appear. + mockedUseFeatureFlagEnabled.mockImplementation( + (flag) => flag === FeatureFlags.B2BAnalyticsDashboard, + ) + const { orgX } = setupProgramsAndCourses() + + setMockResponse.get(managerOrganizationsUrl, { + count: 1, + next: null, + previous: null, + results: [orgX], + }) + renderWithProviders( + , + ) + + await screen.findByRole("link", { name: "View analytics" }) + expect( + screen.queryByRole("link", { name: "View learner analytics" }), + ).not.toBeInTheDocument() + }) + + test("the learner flag alone renders neither analytics button", async () => { + // Learner analytics is nested inside the analytics rollout, so its flag + // grants nothing on its own โ€” the page it links to 403s without both. + mockedUseFeatureFlagEnabled.mockImplementation( + (flag) => flag === FeatureFlags.B2BLearnerAnalytics, + ) + const { orgX } = setupProgramsAndCourses() + + setMockResponse.get(managerOrganizationsUrl, { + count: 1, + next: null, + previous: null, + results: [orgX], + }) + renderWithProviders( + , + ) + + await screen.findByRole("heading", { name: orgX.name }) + expect( + screen.queryByRole("link", { name: "View learner analytics" }), + ).not.toBeInTheDocument() + expect( + screen.queryByRole("link", { name: "View analytics" }), + ).not.toBeInTheDocument() + }) + + test("renders both analytics buttons when both flags are on", async () => { + mockedUseFeatureFlagEnabled.mockImplementation( + (flag) => + flag === FeatureFlags.B2BAnalyticsDashboard || + flag === FeatureFlags.B2BLearnerAnalytics, + ) + const { orgX } = setupProgramsAndCourses() + + setMockResponse.get(managerOrganizationsUrl, { + count: 1, + next: null, + previous: null, + results: [orgX], + }) + renderWithProviders( + , + ) + + const learnersLink = await screen.findByRole("link", { + name: "View learner analytics", + }) + expect(learnersLink).toHaveAttribute( + "href", + contractLearnersView(orgX.slug, orgX.contracts[0].slug), + ) + await screen.findByRole("link", { name: "View analytics" }) + }) + test("sanitizes HTML content in welcome_message_extra", async () => { const { orgX } = setupProgramsAndCourses() @@ -2549,3 +2641,170 @@ describe("ContractContent", () => { ) }) }) + +describe("ContractContent data consent", () => { + beforeEach(() => { + setMockResponse.get(urls.enrollment.enrollmentsListV3(), []) + setMockResponse.get(urls.programEnrollments.enrollmentsListV3(), []) + mockedUseFeatureFlagEnabled.mockImplementation( + (flag) => flag === FeatureFlags.B2BDataConsent, + ) + }) + + const setupConsent = (consented: boolean | null) => { + const setup = setupProgramsAndCourses() + const contract = { + ...setup.orgX.contracts[0], + consented_to_data_sharing: consented, + } + const org = { ...setup.orgX, contracts: [contract] } + const mitxOnlineUser = { ...setup.mitxOnlineUser, b2b_organizations: [org] } + setMockResponse.get(urls.userMe.get(), mitxOnlineUser) + return { org, contract, mitxOnlineUser } + } + + const renderContract = (org: { slug: string }, contractSlug: string) => + renderWithProviders( + , + ) + + const startButtons = async () => { + const programs = await screen.findAllByTestId("org-program-root") + const buttons = programs.flatMap((program) => + within(program) + .getAllByTestId("enrollment-card-desktop") + .map((card) => within(card).getByTestId("courseware-button")), + ) + expect(buttons.length).toBeGreaterThan(0) + return buttons + } + + const consentCheckbox = () => + screen.getByRole("checkbox", { + name: "I have read and consent to the data sharing described above.", + }) + + test.each([null, false])( + "shows the dialog and disables every card when consent is %s", + async (consented) => { + const { org, contract } = setupConsent(consented) + renderContract(org, contract.slug) + + const dialog = await screen.findByRole("dialog") + expect(dialog).toHaveTextContent(contract.name) + for (const button of await startButtons()) { + expect(button).toBeDisabled() + } + }, + ) + + test("shows no dialog and leaves cards enabled once consent is true", async () => { + const { org, contract } = setupConsent(true) + renderContract(org, contract.slug) + + const buttons = await startButtons() + expect(buttons.some((button) => !button.hasAttribute("disabled"))).toBe( + true, + ) + expect(screen.queryByRole("dialog")).not.toBeInTheDocument() + }) + + test("shows no dialog and leaves cards enabled when the flag is off", async () => { + mockedUseFeatureFlagEnabled.mockReturnValue(false) + const { org, contract } = setupConsent(null) + renderContract(org, contract.slug) + + const buttons = await startButtons() + expect(buttons.some((button) => !button.hasAttribute("disabled"))).toBe( + true, + ) + expect(screen.queryByRole("dialog")).not.toBeInTheDocument() + }) + + test("Decline records false and closes the dialog, but cards stay disabled", async () => { + const { org, contract, mitxOnlineUser } = setupConsent(null) + setMockResponse.post(urls.b2b.dataConsent(contract.id), undefined, { + code: 204, + }) + renderContract(org, contract.slug) + await screen.findByRole("dialog") + + // What users/me returns after the POST. + setMockResponse.get(urls.userMe.get(), { + ...mitxOnlineUser, + b2b_organizations: [ + { + ...org, + contracts: [{ ...contract, consented_to_data_sharing: false }], + }, + ], + }) + await user.click(screen.getByRole("button", { name: "Decline" })) + + await waitFor(() => { + expect(screen.queryByRole("dialog")).not.toBeInTheDocument() + }) + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "post", + url: urls.b2b.dataConsent(contract.id), + body: { consented: false }, + }), + ) + for (const button of await startButtons()) { + expect(button).toBeDisabled() + } + }) + + test("Agree records true, closes the dialog, and enables the cards", async () => { + const { org, contract, mitxOnlineUser } = setupConsent(null) + setMockResponse.post(urls.b2b.dataConsent(contract.id), undefined, { + code: 204, + }) + renderContract(org, contract.slug) + await screen.findByRole("dialog") + + setMockResponse.get(urls.userMe.get(), { + ...mitxOnlineUser, + b2b_organizations: [ + { + ...org, + contracts: [{ ...contract, consented_to_data_sharing: true }], + }, + ], + }) + await user.click(consentCheckbox()) + await user.click(screen.getByRole("button", { name: "Agree and continue" })) + + await waitFor(() => { + expect(screen.queryByRole("dialog")).not.toBeInTheDocument() + }) + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: "post", + url: urls.b2b.dataConsent(contract.id), + body: { consented: true }, + }), + ) + const buttons = await startButtons() + expect(buttons.some((button) => !button.hasAttribute("disabled"))).toBe( + true, + ) + }) + + test("keeps the dialog open with an error when the request fails", async () => { + const { org, contract } = setupConsent(null) + setMockResponse.post(urls.b2b.dataConsent(contract.id), "Server error", { + code: 500, + }) + renderContract(org, contract.slug) + await screen.findByRole("dialog") + + await user.click(screen.getByRole("button", { name: "Decline" })) + + expect(await screen.findByRole("alert")).toHaveTextContent( + "We couldn't save your response. Please try again.", + ) + expect(screen.getByRole("dialog")).toBeInTheDocument() + }) +}) diff --git a/frontends/main/src/app-pages/DashboardPage/ContractContent.tsx b/frontends/main/src/app-pages/DashboardPage/ContractContent.tsx index e9e9081636..fa73b0bcec 100644 --- a/frontends/main/src/app-pages/DashboardPage/ContractContent.tsx +++ b/frontends/main/src/app-pages/DashboardPage/ContractContent.tsx @@ -1,6 +1,6 @@ "use client" -import React, { useEffect } from "react" +import React, { useEffect, useState } from "react" import Image from "next/image" import { useQuery } from "@tanstack/react-query" import { @@ -21,7 +21,11 @@ import type { V3UserProgramEnrollment, } from "@mitodl/mitxonline-api-axios/v2" import { mitxUserQueries } from "api/mitxonline-hooks/user" -import { managerOrganizationQueries } from "api/mitxonline-hooks/organizations" +import { + managerOrganizationQueries, + useDataConsentMutation, +} from "api/mitxonline-hooks/organizations" +import { SILENCE_ERROR_TOAST } from "api/mutation-meta" import { ButtonLink } from "@mitodl/smoot-design" import { RiAwardFill } from "@remixicon/react" import { useFeatureFlagEnabled } from "posthog-js/react" @@ -39,6 +43,7 @@ import { useContractDashboardData } from "./CoursewareDisplay/hooks/useContractD import UnstyledRawHTML from "@/components/UnstyledRawHTML/UnstyledRawHTML" import { VariantPicker } from "./CoursewareDisplay/VariantPicker" import { CoursewareCard } from "./CoursewareDisplay/CoursewareCard" +import { DataConsentDialog } from "./DataConsentDialog" const HeaderRoot = styled.div(({ theme }) => ({ display: "flex", @@ -251,7 +256,8 @@ const OrgProgramCollectionDisplay: React.FC<{ collection: V2ProgramCollection entries: DashboardCourseEntry[] hideDescription?: boolean -}> = ({ collection, entries, hideDescription }) => { + cardsDisabled?: boolean +}> = ({ collection, entries, hideDescription, cardsDisabled }) => { const header = ( @@ -284,6 +290,7 @@ const OrgProgramCollectionDisplay: React.FC<{ Component="li" kind="course" entry={entry} + disabled={cardsDisabled} /> ) })} @@ -297,7 +304,14 @@ const OrgProgramDisplay: React.FC<{ entries: DashboardCourseEntry[] programEnrollment?: V3UserProgramEnrollment hideDescription?: boolean -}> = ({ program, entries, programEnrollment, hideDescription }) => { + cardsDisabled?: boolean +}> = ({ + program, + entries, + programEnrollment, + hideDescription, + cardsDisabled, +}) => { const hasValidCertificate = !!programEnrollment?.certificate if (entries.length === 0) { @@ -340,6 +354,7 @@ const OrgProgramDisplay: React.FC<{ Component="li" kind="course" entry={entry} + disabled={cardsDisabled} /> ) })} @@ -413,10 +428,12 @@ const HeaderActions = styled.div(({ theme }) => ({ type ContractContentInternalProps = { org: OrganizationPage contract: ContractPage + cardsDisabled?: boolean } const ContractContentInternal: React.FC = ({ org, contract, + cardsDisabled = false, }) => { const { isLoading, @@ -434,6 +451,9 @@ const ContractContentInternal: React.FC = ({ const analyticsEnabled = useFeatureFlagEnabled( FeatureFlags.B2BAnalyticsDashboard, ) + const learnerAnalyticsEnabled = useFeatureFlagEnabled( + FeatureFlags.B2BLearnerAnalytics, + ) const { data: managerOrgs } = useQuery({ ...managerOrganizationQueries.managerOrganizationsList(), enabled: managerDashboardFlag === true || analyticsEnabled === true, @@ -495,7 +515,7 @@ const ContractContentInternal: React.FC = ({ View analytics )} - {analyticsEnabled && ( + {analyticsEnabled && learnerAnalyticsEnabled && ( = ({ hideDescription={ selectedVariant !== null && !selectedVariant.default_variant } + cardsDisabled={cardsDisabled} /> ))} @@ -560,6 +581,7 @@ const ContractContentInternal: React.FC = ({ hideDescription={ selectedVariant !== null && !selectedVariant.default_variant } + cardsDisabled={cardsDisabled} /> ))} @@ -594,6 +616,29 @@ const ContractContent: React.FC = ({ (contract) => contract.slug === contractSlug, ) + const consentFlag = useFeatureFlagEnabled(FeatureFlags.B2BDataConsent) + const consentRequired = + consentFlag === true && + !!b2bContract && + b2bContract.consented_to_data_sharing !== true + // Declining closes the dialog for this contract until the next page load. + const [declinedContractId, setDeclinedContractId] = useState( + null, + ) + const consentMutation = useDataConsentMutation({ meta: SILENCE_ERROR_TOAST }) + const submitConsent = (consented: boolean) => { + if (!b2bContract) return + const contractId = b2bContract.id + consentMutation.mutate( + { contract_id: contractId, DataConsentRequest: { consented } }, + { + onSuccess: () => { + if (!consented) setDeclinedContractId(contractId) + }, + }, + ) + } + useEffect(() => { if (b2bOrganization) { localStorage.setItem("last-dashboard-org", orgSlug) @@ -621,7 +666,28 @@ const ContractContent: React.FC = ({ } return ( - + <> + + submitConsent(true)} + onDecline={() => submitConsent(false)} + submitting={ + consentMutation.isPending + ? consentMutation.variables?.DataConsentRequest.consented + ? "accept" + : "decline" + : null + } + isError={consentMutation.isError} + /> + ) } diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CoursewareCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CoursewareCard.tsx index 8eaf4c6a16..f8e39986c5 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CoursewareCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/CoursewareCard.tsx @@ -51,6 +51,7 @@ type CoursewareCardCourseProps = StyledComponentBaseProps & CourseDisplayProps & { kind: "course" entry: DashboardCourseEntry + disabled?: boolean } /** @@ -110,7 +111,7 @@ const CoursewareCard: React.FC = (props) => { ) } - const { entry } = props + const { entry, disabled } = props return entry.displayedEnrollment ? ( // Happens to be the same as the enrollment branch above, for now. // Will likely diverge with multiple enrollment display. @@ -125,6 +126,7 @@ const CoursewareCard: React.FC = (props) => { headingLevel={headingLevel} onUpgradeError={onUpgradeError} isModule={isModule} + disabled={disabled} Component={Component} className={className} /> @@ -137,6 +139,7 @@ const CoursewareCard: React.FC = (props) => { layout={layout} headingLevel={headingLevel} isModule={isModule} + disabled={disabled} Component={Component} className={className} /> diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx index e654b974fc..a374ba5490 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx @@ -1223,3 +1223,67 @@ describe("EnrolledCourseCard progress badge", () => { ).not.toBeInTheDocument() }) }) + +describe.each([ + { display: "desktop", testId: "enrollment-card-desktop" }, + { display: "mobile", testId: "enrollment-card-mobile" }, +])("EnrolledCourseCard disabled ($display)", ({ testId }) => { + setupLocationMock() + + test("shows a plain-text title and disables the CTA and menu, keeping the certificate link", () => { + setupUserApis() + const certUuid = faker.string.uuid() + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + run: { ...currentRunDates, courseware_url: faker.internet.url() }, + certificate: { + uuid: certUuid, + link: `https://courses.example.com/certificate/${certUuid}/`, + }, + }) + renderWithProviders() + const card = within(screen.getByTestId(testId)) + + expect( + card.getByRole("heading", { name: enrollment.run.course.title }), + ).toBeInTheDocument() + expect( + card.queryByRole("link", { name: enrollment.run.course.title }), + ).not.toBeInTheDocument() + expect(card.getByTestId("courseware-button")).toBeDisabled() + expect(card.getByRole("button", { name: "More options" })).toBeDisabled() + expect( + card.getByRole("link", { name: /View Certificate/ }), + ).toHaveAttribute( + "href", + `https://courses.example.com/certificate/course/${certUuid}/`, + ) + }) +}) + +describe("EnrolledCourseCard disabled with sibling runs", () => { + setupLocationMock() + + test("hides each run's View content link", async () => { + setupUserApis() + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + run: { ...currentRunDates, courseware_url: faker.internet.url() }, + }) + const sibling = mitxonline.factories.enrollment.courseEnrollment({ + run: { ...pastRunDates, courseware_url: faker.internet.url() }, + }) + renderWithProviders( + , + ) + const desktopCard = within(screen.getByTestId("enrollment-card-desktop")) + await user.click(desktopCard.getByText("Course runs (2)")) + + expect(desktopCard.getByText("Current run:")).toBeInTheDocument() + expect( + desktopCard.queryByRole("link", { name: /View content/ }), + ).not.toBeInTheDocument() + }) +}) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx index e6c4746bf8..8f5d5ec9f5 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx @@ -241,6 +241,7 @@ type EnrolledCourseCardProps = { headingLevel?: "h2" | "h3" | "h4" | "h5" | "h6" onUpgradeError?: (error: string) => void isModule?: boolean + disabled?: boolean Component?: React.ElementType className?: string } @@ -253,6 +254,7 @@ export const EnrolledCourseCard = ({ headingLevel, onUpgradeError, isModule, + disabled = false, Component, className, }: EnrolledCourseCardProps) => { @@ -364,7 +366,7 @@ export const EnrolledCourseCard = ({ ) : null const titleSection = ( - {coursewareUrl && coursewareOpen ? ( + {coursewareUrl && coursewareOpen && !disabled ? ( ) const courseHasEnded = run?.end_date ? isInPast(run.end_date) : false - const isDisabled = Boolean(!coursewareUrl || !coursewareOpen) + const isDisabled = disabled || !coursewareUrl || !coursewareOpen const isCompleted = enrollmentStatus === EnrollmentStatus.Completed || courseHasEnded const buttonText = isCompleted ? "View" : "Continue" @@ -455,21 +457,23 @@ export const EnrolledCourseCard = ({ menuItems.push(...getRunMenuItems({ enrollment, title, receiptResolution })) - const contextMenu = ( - - {coursewareUrl && coursewareOpen && ( + {coursewareUrl && coursewareOpen && !disabled && ( <> = ({ ) @@ -298,6 +310,7 @@ type SiblingRunsPanelProps = { id?: string /** id of the SiblingRunsToggle that labels this panel. */ labelledBy?: string + disabled?: boolean } const SiblingRunsPanel: React.FC = ({ @@ -306,6 +319,7 @@ const SiblingRunsPanel: React.FC = ({ expanded, id, labelledBy, + disabled = false, }) => { const currentRun = enrollment.run const currentStatus = getDashboardEnrollmentStatus({ @@ -351,6 +365,7 @@ const SiblingRunsPanel: React.FC = ({ labelValue={currentLabelValue} runLabel={currentRunLabel} enrollment={enrollment} + disabled={disabled} /> {siblingEnrollments.map((e) => { const startDate = e.run?.start_date @@ -402,6 +417,7 @@ const SiblingRunsPanel: React.FC = ({ labelValue={runIdentifier} runLabel={fullLabel} enrollment={e} + disabled={disabled} /> ) })} diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx index c18b921b84..a1ddbaf612 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.test.tsx @@ -1314,3 +1314,38 @@ describe("UnenrolledCourseCard progress badge", () => { ).not.toBeInTheDocument() }) }) + +describe.each([ + { display: "desktop", testId: "enrollment-card-desktop" }, + { display: "mobile", testId: "enrollment-card-mobile" }, +])("UnenrolledCourseCard disabled ($display)", ({ testId }) => { + setupLocationMock() + + test("shows a plain-text title and a disabled Start button for an enrollable run", async () => { + setupUserApis() + const b2bContractId = faker.number.int() + const run = mitxonline.factories.courses.courseRun({ + b2b_contract: b2bContractId, + is_enrollable: true, + }) + const course = mitxOnlineCourse({ + title: run.title, + courseruns: [run], + next_run_id: run.id, + }) + renderWithProviders( + , + ) + const card = within(screen.getByTestId(testId)) + + expect(card.getByRole("heading", { name: run.title })).toBeInTheDocument() + expect( + card.queryByRole("button", { name: run.title }), + ).not.toBeInTheDocument() + expect(card.getByTestId("courseware-button")).toBeDisabled() + }) +}) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx index bc672642b3..a9bc93adf3 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/UnenrolledCourseCard.tsx @@ -34,6 +34,7 @@ type UnenrolledCourseCardProps = { layout?: "default" | "compact" headingLevel?: "h2" | "h3" | "h4" | "h5" | "h6" isModule?: boolean + disabled?: boolean Component?: React.ElementType className?: string } @@ -46,6 +47,7 @@ export const UnenrolledCourseCard = ({ layout = "default", headingLevel, isModule, + disabled = false, Component, className, }: UnenrolledCourseCardProps) => { @@ -55,7 +57,8 @@ export const UnenrolledCourseCard = ({ displayedRunProp ?? getBestRun(course, { enrollableOnly: true, contractId }) const coursewareUrl = courseRun?.courseware_url || undefined const readableId = courseRun?.courseware_id - const isDisabled = !courseRun?.is_enrollable || !coursewareUrl || !readableId + const isDisabled = + disabled || !courseRun?.is_enrollable || !coursewareUrl || !readableId const title = layout === "compact" ? course.title : courseRun?.title || course.title const isContractPageResource = Boolean(contractId) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/test-utils.ts b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/test-utils.ts index 8ea17d1778..60dc571c9e 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/test-utils.ts +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/test-utils.ts @@ -11,6 +11,7 @@ import { CourseWithCourseRunsSerializerV2, OrganizationPage, User, + UserContractPage, V2Program, V2ProgramDetail, V3UserProgramEnrollment, @@ -461,7 +462,7 @@ const createTestContracts = ( orgId: number, count: number = 1, programs: number[] = [], -): ContractPage[] => +): UserContractPage[] => Array.from({ length: count }, () => makeContract({ organization: orgId, programs }), ) diff --git a/frontends/main/src/app-pages/DashboardPage/DashboardLayout.test.tsx b/frontends/main/src/app-pages/DashboardPage/DashboardLayout.test.tsx index 57f88a72d4..5cbfeb8820 100644 --- a/frontends/main/src/app-pages/DashboardPage/DashboardLayout.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/DashboardLayout.test.tsx @@ -21,14 +21,14 @@ import { } from "@/common/urls" import { faker } from "@faker-js/faker/locale/en" import invariant from "tiny-invariant" -import { OrganizationPage } from "@mitodl/mitxonline-api-axios/v2" +import { UserOrganizationPage } from "@mitodl/mitxonline-api-axios/v2" jest.mock("posthog-js/react") describe("DashboardLayout", () => { type SetupOptions = { initialUrl?: string - organizations?: OrganizationPage[] + organizations?: UserOrganizationPage[] } const setup = ({ initialUrl = DASHBOARD_HOME, diff --git a/frontends/main/src/app-pages/DashboardPage/DataConsentDialog.test.tsx b/frontends/main/src/app-pages/DashboardPage/DataConsentDialog.test.tsx new file mode 100644 index 0000000000..184f0f3fb4 --- /dev/null +++ b/frontends/main/src/app-pages/DashboardPage/DataConsentDialog.test.tsx @@ -0,0 +1,85 @@ +import React from "react" +import { renderWithProviders, screen, user } from "@/test-utils" +import { DataConsentDialog } from "./DataConsentDialog" + +const setup = ( + props: Partial> = {}, +) => { + const onAccept = jest.fn() + const onDecline = jest.fn() + renderWithProviders( + , + ) + return { onAccept, onDecline } +} + +describe("DataConsentDialog", () => { + test("names the contract in the consent text", () => { + setup() + expect( + screen.getByText( + /paid for my participation in the Horizon Digital Program \| Cohort 2\./, + ), + ).toBeInTheDocument() + }) + + test("can't be dismissed: no close button, and Escape leaves it open", async () => { + const { onAccept, onDecline } = setup() + expect( + screen.queryByRole("button", { name: "Close" }), + ).not.toBeInTheDocument() + + await user.keyboard("{Escape}") + + expect(screen.getByRole("dialog")).toBeInTheDocument() + expect(onAccept).not.toHaveBeenCalled() + expect(onDecline).not.toHaveBeenCalled() + }) + + test("Agree and continue is disabled until the consent box is checked", async () => { + const { onAccept } = setup() + const agree = screen.getByRole("button", { name: "Agree and continue" }) + expect(agree).toBeDisabled() + + await user.click( + screen.getByRole("checkbox", { + name: "I have read and consent to the data sharing described above.", + }), + ) + expect(agree).toBeEnabled() + + await user.click(agree) + expect(onAccept).toHaveBeenCalledTimes(1) + }) + + test("Decline calls onDecline without the box checked", async () => { + const { onAccept, onDecline } = setup() + await user.click(screen.getByRole("button", { name: "Decline" })) + expect(onDecline).toHaveBeenCalledTimes(1) + expect(onAccept).not.toHaveBeenCalled() + }) + + test.each(["accept", "decline"] as const)( + "disables both actions while submitting %s", + (submitting) => { + setup({ submitting }) + expect(screen.getByRole("button", { name: "Decline" })).toBeDisabled() + expect( + screen.getByRole("button", { name: "Agree and continue" }), + ).toBeDisabled() + }, + ) + + test("shows an error when the last request failed", () => { + setup({ isError: true }) + expect(screen.getByRole("alert")).toHaveTextContent( + "We couldn't save your response. Please try again.", + ) + }) +}) diff --git a/frontends/main/src/app-pages/DashboardPage/DataConsentDialog.tsx b/frontends/main/src/app-pages/DashboardPage/DataConsentDialog.tsx new file mode 100644 index 0000000000..439000f061 --- /dev/null +++ b/frontends/main/src/app-pages/DashboardPage/DataConsentDialog.tsx @@ -0,0 +1,118 @@ +import React from "react" +import { Dialog, DialogActions, LoadingSpinner, styled } from "ol-components" +import { Alert, Button, Checkbox } from "@mitodl/smoot-design" + +const Paragraph = styled.p(({ theme }) => ({ + ...theme.typography.body2, + lineHeight: theme.typography.pxToRem(22), + color: theme.custom.colors.black, + margin: 0, +})) + +const Body = styled.div({ + display: "flex", + flexDirection: "column", + gap: "28px", +}) + +// smoot-design's Checkbox has a fixed 24px height; this label wraps. +const ConsentCheckbox = styled(Checkbox)({ + "&&": { height: "auto" }, +}) + +const Actions = styled(DialogActions)({ + gap: "12px", + "> *": { + flex: 1, + }, +}) + +type DataConsentDialogProps = { + open: boolean + contractName: string + onAccept: () => void + onDecline: () => void + submitting?: "accept" | "decline" | null + isError?: boolean +} + +const DataConsentDialog: React.FC = ({ + open, + contractName, + onAccept, + onDecline, + submitting = null, + isError = false, +}) => { + const [agreed, setAgreed] = React.useState(false) + const spinner = + + return ( + {}} + showCloseButton={false} + title="Data Consent - Requirement for Enrollment" + maxWidth="sm" + fullWidth + actions={ + + + + + } + > + + + I understand that my employer has paid for my participation in the{" "} + {contractName}. I hereby consent and authorize MIT to share the + following information about my participation and progress in the + Program with my employer: (a) Program completion status; (b) module + progress and completion dates; (c) assessment scores and performance + metrics; (d) certificates earned; and (e) any other progression or + assessment data maintained by MIT. + + + I understand that the data will be used solely to evaluate program + effectiveness, track workforce development and assess return on + training investment. + + + I understand that sharing of this data is a condition of my + participation in the Program; I acknowledge that my employer is + funding the training; and I have been informed of what data will be + shared and with whom. + + setAgreed(event.target.checked)} + /> + {isError ? ( + + We couldn't save your response. Please try again. + + ) : null} + + + ) +} + +export { DataConsentDialog } +export type { DataConsentDialogProps } diff --git a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx index a1a5e299c9..f81ad618cf 100644 --- a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx +++ b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentEditPage.tsx @@ -2,7 +2,7 @@ import React from "react" import { useRouter } from "next-nprogress-bar" -import { notFound, usePathname } from "next/navigation" +import { notFound } from "next/navigation" import { Permission } from "api/hooks/user" import { useWebsiteContentDetailRetrieve } from "api/hooks/website_content" import RestrictedRoute from "@/components/RestrictedRoute/RestrictedRoute" @@ -12,6 +12,7 @@ import { NewsEditor } from "@/page-components/TiptapEditor/contentTypes/news/New import { articleView, newsView, websiteContentEditView } from "@/common/urls" import invariant from "tiny-invariant" import type { WebsiteContent } from "api/v1" +import { replaceDraftUrl } from "./draftUrl" const PageContainer = styled.div(({ theme }) => ({ color: theme.custom.colors.darkGray2, @@ -66,7 +67,6 @@ const WebsiteContentEditPage = ({ autosaveDelayMs, }: WebsiteContentEditPageProps) => { const { data: article, isLoading } = useWebsiteContentDetailRetrieve(idOrSlug) - const pathname = usePathname() const router = useRouter() const Editor = EDITORS[type] @@ -106,18 +106,13 @@ const WebsiteContentEditPage = ({ /** * Where a draft lives, which is usually where we already are -- * the exception being a URL that names the item by slug, which - * this canonicalises to the id once. + * this canonicalises to the id. * - * Guarded because a draft saves itself every couple of seconds: - * pushing the route we are on buys nothing (this page reads its - * item through React Query, which the mutation already - * invalidates) and costs a soft navigation and a run of the - * progress bar each time. + * Corrected in place rather than navigated to, because a draft + * saves itself while the author is typing and a navigation would + * take the editor down mid-sentence. See `replaceDraftUrl`. */ - const draftUrl = websiteContentEditView(type, saved.id) - if (draftUrl !== pathname) { - router.push(draftUrl) - } + replaceDraftUrl(websiteContentEditView(type, saved.id)) }} /> diff --git a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentNewPage.happydom.test.tsx b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentNewPage.happydom.test.tsx new file mode 100644 index 0000000000..c97608e6a8 --- /dev/null +++ b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentNewPage.happydom.test.tsx @@ -0,0 +1,102 @@ +/** + * @jest-environment @happy-dom/jest-environment + * + * Using the Happy DOM environment as the editor accesses DOM APIs and uses + * contenteditable elements not supported by JSDOM, the default Jest environment. + */ +import React from "react" +import { screen, waitFor } from "@testing-library/react" +import userEvent from "@testing-library/user-event" +import { setMockResponse, factories, urls, makeRequest } from "api/test-utils" +import { WebsiteContentNewPage } from "./WebsiteContentNewPage" +import { websiteContentEditView } from "@/common/urls" +import { renderWithProviders } from "@/test-utils" + +/** Mounting a real ProseMirror in happy-dom and waiting out a save. */ +jest.setTimeout(30000) + +/** + * The page navigates through `next-nprogress-bar`'s wrapper rather than + * `next/navigation` directly, so this is where a push can be observed -- + * spying on the memory router does not see it. The wrapper's own same-URL + * check only suppresses the progress bar; it still pushes. + */ +jest.mock("next-nprogress-bar", () => ({ + useRouter: () => ({ + push: (...args: unknown[]) => routerMocks.push(...args), + }), +})) + +const routerMocks = { push: jest.fn() } + +beforeEach(() => { + routerMocks.push.mockClear() + window.history.replaceState({}, "", "/website_content/article/new") +}) + +/** What production uses; this test waits it out deliberately. */ +const AUTOSAVE_DELAY_MS = 2000 + +describe("WebsiteContentNewPage autosave", () => { + /** + * The first autosave of something new is what gives it a URL, and it lands + * while the author is still typing. Navigating there would take the editor + * down mid-sentence -- the caret goes, and the next route shows a spinner + * while it fetches the item the editor is already holding. To the author the + * page reloaded under them. + * + * Only the absence of the push can be asserted here, not the absence of the + * remount: the router is mocked, so a `push` would not actually unmount + * anything in this environment. The push is the cause, so it is the thing + * worth pinning. + */ + test("the created draft's URL is adopted without navigating", async () => { + const user = factories.user.user({ + is_authenticated: true, + is_article_editor: true, + }) + setMockResponse.get(urls.userMe.get(), user) + const article = factories.websiteContent.websiteContent({ + id: 777, + content_type: "article", + is_published: false, + }) + setMockResponse.post(urls.websiteContent.list(), article) + setMockResponse.patch(urls.websiteContent.details(article.id), article) + + renderWithProviders( + , + { user, url: "/website_content/article/new" }, + ) + await screen.findByTestId("editor") + + await userEvent.type( + await screen.findByRole("heading", { level: 1 }), + "A brand new article", + ) + + await waitFor( + () => { + expect(makeRequest).toHaveBeenCalledWith( + expect.objectContaining({ method: "post" }), + ) + }, + { timeout: 12000 }, + ) + + // The address bar names the item that was created... + await waitFor(() => { + expect(window.location.pathname).toBe( + websiteContentEditView("article", article.id), + ) + }) + // ...and nothing navigated to get there. + expect(routerMocks.push).not.toHaveBeenCalled() + + // Settled, so no write lands after the test ends. + await waitFor(() => expect(screen.queryByText("Saving...")).toBe(null)) + }) +}) diff --git a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentNewPage.tsx b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentNewPage.tsx index d378cffe83..70ec771f6f 100644 --- a/frontends/main/src/app-pages/WebsiteContent/WebsiteContentNewPage.tsx +++ b/frontends/main/src/app-pages/WebsiteContent/WebsiteContentNewPage.tsx @@ -11,6 +11,7 @@ import { NewsEditor } from "@/page-components/TiptapEditor/contentTypes/news/New import { articleView, newsView, websiteContentEditView } from "@/common/urls" import invariant from "tiny-invariant" import type { WebsiteContent } from "api/v1" +import { replaceDraftUrl } from "./draftUrl" const PageContainer = styled.div(({ theme }) => ({ color: theme.custom.colors.darkGray2, @@ -29,6 +30,7 @@ const EDITORS: Record< onSave?: (savedContent: WebsiteContent) => void readOnly?: boolean contentItem?: WebsiteContent + autosaveDelayMs?: number }> > = { article: ({ contentItem, ...props }) => ( @@ -39,10 +41,17 @@ const EDITORS: Record< interface WebsiteContentNewPageProps { type: string + /** + * Passed straight to the editor; only tests set it, to keep a background + * draft save from landing in the middle of their interactions. See + * `WebsiteContentEditor`. + */ + autosaveDelayMs?: number } const WebsiteContentNewPage: React.FC = ({ type, + autosaveDelayMs, }) => { const router = useRouter() const Editor = EDITORS[type] @@ -56,13 +65,19 @@ const WebsiteContentNewPage: React.FC = ({ { if (article.is_published) { invariant(article.slug, "Published content must have a slug") return router.push(viewUrl(article.slug)) - } else { - router.push(websiteContentEditView(type, article.id)) } + /** + * The draft has just been created by an autosave, so this fires + * while the author is still typing. The URL is corrected in place + * rather than navigated to -- see `replaceDraftUrl`, which is + * where the reasoning lives. + */ + replaceDraftUrl(websiteContentEditView(type, article.id)) }} /> diff --git a/frontends/main/src/app-pages/WebsiteContent/draftUrl.test.ts b/frontends/main/src/app-pages/WebsiteContent/draftUrl.test.ts new file mode 100644 index 0000000000..051c665698 --- /dev/null +++ b/frontends/main/src/app-pages/WebsiteContent/draftUrl.test.ts @@ -0,0 +1,51 @@ +import { replaceDraftUrl } from "./draftUrl" + +const at = (pathname: string) => window.history.replaceState({}, "", pathname) + +describe("replaceDraftUrl", () => { + test("puts the draft's URL in the address bar", () => { + at("/website_content/article/new") + + replaceDraftUrl("/website_content/article/42/edit") + + expect(window.location.pathname).toBe("/website_content/article/42/edit") + }) + + test("canonicalises a URL that named the item by slug", () => { + at("/website_content/article/some-slug/edit") + + replaceDraftUrl("/website_content/article/42/edit") + + expect(window.location.pathname).toBe("/website_content/article/42/edit") + }) + + /** + * The common case, since a draft saves itself every couple of seconds and + * every save after the first is to an item whose URL we are already on. + * Writing the same entry repeatedly is what `history.replaceState` is least + * bad at, but skipping it keeps the intent legible and costs nothing. + */ + test("does nothing when the URL is already right", () => { + at("/website_content/article/42/edit") + const replaceState = jest.spyOn(window.history, "replaceState") + + replaceDraftUrl("/website_content/article/42/edit") + + expect(replaceState).not.toHaveBeenCalled() + replaceState.mockRestore() + }) + + /** + * `replace`, not `push`: Back should leave the editor, not step through an + * entry per autosave to get out of it. + */ + test("leaves no history entry behind", () => { + at("/website_content/article/new") + const pushState = jest.spyOn(window.history, "pushState") + + replaceDraftUrl("/website_content/article/42/edit") + + expect(pushState).not.toHaveBeenCalled() + pushState.mockRestore() + }) +}) diff --git a/frontends/main/src/app-pages/WebsiteContent/draftUrl.ts b/frontends/main/src/app-pages/WebsiteContent/draftUrl.ts new file mode 100644 index 0000000000..eb307a9a42 --- /dev/null +++ b/frontends/main/src/app-pages/WebsiteContent/draftUrl.ts @@ -0,0 +1,28 @@ +/** + * Point the address bar at where a saved draft lives, without navigating. + * + * A draft writes itself a couple of seconds after typing stops, and the first + * such save on a new item is what gives that item a URL. Reaching it with + * `router.push` unmounts the editor and mounts the next page in its place: the + * caret is lost mid-sentence, the new route shows a spinner while it fetches + * the item the editor is already holding, and to the author the page simply + * reloaded under them while they were writing. + * + * `history.replaceState` is Next's supported way to change the URL without + * re-running the route, so nothing unmounts and the editor keeps its state and + * its focus. That is safe because the URL is not what the editor saves + * against -- it remembers the row it created (`createdIdRef` in + * `WebsiteContentEditor`) and PATCHes that regardless of what the address bar + * says. Updating it only matters for a reload, a bookmark, or a shared link. + * + * `replace` rather than `push`, so Back does not have to step through an entry + * per autosave to get out of the editor. + * + * No-op when the URL is already right, which is the common case: every save + * after the first one is to an item whose URL we are already on. + */ +export const replaceDraftUrl = (url: string): void => { + if (typeof window === "undefined") return + if (window.location.pathname === url) return + window.history.replaceState({}, "", url) +} diff --git a/frontends/main/src/common/feature_flags.ts b/frontends/main/src/common/feature_flags.ts index b54be81533..90fa560768 100644 --- a/frontends/main/src/common/feature_flags.ts +++ b/frontends/main/src/common/feature_flags.ts @@ -15,6 +15,7 @@ export enum FeatureFlags { VideoPlaylistPage = "video-playlist-page", B2BContractManagerDashboard = "b2b-contract-manager-dashboard", B2BAnalyticsDashboard = "b2b-analytics-dashboard", + B2BLearnerAnalytics = "b2b-learner-analytics", Arithmix = "arithmix", Hacksnack = "hacksnack", AccountManagement = "account-management", @@ -23,6 +24,7 @@ export enum FeatureFlags { OrganizationalLearning = "organizational-learning", MultipleRunContextMenus = "multiple-run-context-menus", ProgramLetters = "program-letters", + B2BDataConsent = "b2b-data-consent", } /** diff --git a/frontends/ol-components/src/components/Dialog/Dialog.tsx b/frontends/ol-components/src/components/Dialog/Dialog.tsx index ebb93777c2..19f54f95dc 100644 --- a/frontends/ol-components/src/components/Dialog/Dialog.tsx +++ b/frontends/ol-components/src/components/Dialog/Dialog.tsx @@ -85,6 +85,7 @@ type DialogProps = { */ additionalLabelledBy?: string role?: MuiDialogProps["role"] + showCloseButton?: boolean } /** @@ -117,6 +118,7 @@ const Dialog: React.FC = ({ "aria-describedby": ariaDescribedBy, additionalLabelledBy, role, + showCloseButton = true, }) => { const [confirming, setConfirming] = useState(isSubmitting) const titleId = useId() @@ -162,11 +164,13 @@ const Dialog: React.FC = ({ maxWidth={maxWidth} scroll={scroll} > - - - - - + {showCloseButton && ( + + + + + + )} {title && (
diff --git a/learning_resources/views.py b/learning_resources/views.py index f88f5bb04d..1c199ca0b7 100644 --- a/learning_resources/views.py +++ b/learning_resources/views.py @@ -1293,6 +1293,30 @@ def get_queryset(self): ).order_by("child", "parent") +def _redact_webhook_key(body: bytes) -> str: + """ + Return a safe-to-log version of a JSON request body, with any + webhook_key value redacted. + + Parses the body the same way the view does and redacts based on the + decoded member name, rather than pattern-matching the raw text -- JSON + allows a member name to be spelled with \\uXXXX escapes (e.g. + "webhook\\u005fkey" decodes to "webhook_key"), so a raw-text match on + the literal key would miss a real key that's merely spelled that way, + letting the actual secret through unredacted. A body that can't be + parsed as a JSON object is redacted in full instead of assumed safe. + """ + try: + parsed = rapidjson.loads(body.decode()) + except (ValueError, UnicodeDecodeError): + return "" + if not isinstance(parsed, dict): + return "" + if "webhook_key" in parsed: + parsed["webhook_key"] = "[redacted]" + return rapidjson.dumps(parsed) + + @method_decorator(blocked_ip_exempt, name="dispatch") class WebhookOCWView(views.APIView): """ @@ -1307,9 +1331,8 @@ def handle_exception(self, exc): Raise any exception with request info instead of returning response with error status/message """ - msg = ( - f"Error ({exc}). BODY: {self.request.body or ''}, META: {self.request.META}" - ) + safe_body = _redact_webhook_key(self.request.body) + msg = f"Error ({exc}). BODY: {safe_body}, META: {self.request.META}" raise WebhookException(msg) from exc @extend_schema(exclude=True) diff --git a/learning_resources/views_test.py b/learning_resources/views_test.py index 76133cc124..694c5732fa 100644 --- a/learning_resources/views_test.py +++ b/learning_resources/views_test.py @@ -686,6 +686,84 @@ def test_ocw_webhook_endpoint_bad_key(settings, client): ) +def test_ocw_webhook_endpoint_bad_key_is_redacted_from_error(settings, client): + """The (wrong) attempted key must not appear verbatim in the raised error""" + settings.OCW_WEBHOOK_KEY = "fake_key" + with pytest.raises(WebhookException) as exc_info: + client.post( + reverse("lr:v1:ocw-next-webhook"), + data={"webhook_key": "bad_key", "prefix": "prefix", "version": "live"}, + headers={"Content-Type": "text/plain"}, + ) + assert "bad_key" not in str(exc_info.value) + assert "[redacted]" in str(exc_info.value) + + +def test_ocw_webhook_endpoint_does_not_leak_secret_on_post_auth_error(settings, client): + """ + A correctly-authenticated request that errors *after* the key check (e.g. + prefixes sent as an int, which .split(',') can't handle) must not leak the + real webhook_key into the resulting exception message. + """ + settings.OCW_WEBHOOK_KEY = "fake_key" + with pytest.raises(WebhookException) as exc_info: + client.post( + reverse("lr:v1:ocw-next-webhook"), + data={"webhook_key": "fake_key", "prefixes": 12345, "version": "live"}, + headers={"Content-Type": "text/plain"}, + ) + assert "fake_key" not in str(exc_info.value) + assert "[redacted]" in str(exc_info.value) + + +def test_ocw_webhook_endpoint_does_not_leak_secret_via_escaped_key_name( + settings, client +): + """ + JSON allows a member name to be spelled with \\uXXXX escapes (e.g. + "webhook\\u005fkey" decodes to "webhook_key"), so a request using that + spelling still authenticates. A raw-text match on the literal + "webhook_key" bytes would miss this spelling and let the real secret + through unredacted -- confirm it doesn't. + """ + settings.OCW_WEBHOOK_KEY = "fake_key" + raw_body = '{"webhook\\u005fkey": "fake_key", "prefixes": 12345, "version": "live"}' + with pytest.raises(WebhookException) as exc_info: + client.post( + reverse("lr:v1:ocw-next-webhook"), + data=raw_body, + content_type="text/plain", + ) + assert "fake_key" not in str(exc_info.value) + assert "[redacted]" in str(exc_info.value) + + +def test_ocw_webhook_endpoint_redacts_unparseable_body(settings, client): + """A body that fails to parse as JSON must not be logged verbatim""" + settings.OCW_WEBHOOK_KEY = "fake_key" + with pytest.raises(WebhookException) as exc_info: + client.post( + reverse("lr:v1:ocw-next-webhook"), + data="not valid json {{{", + content_type="text/plain", + ) + assert "not valid json" not in str(exc_info.value) + assert "redacted" in str(exc_info.value) + + +def test_ocw_webhook_endpoint_redacts_non_object_body(settings, client): + """A syntactically valid but non-object JSON body must not be logged verbatim""" + settings.OCW_WEBHOOK_KEY = "fake_key" + with pytest.raises(WebhookException) as exc_info: + client.post( + reverse("lr:v1:ocw-next-webhook"), + data='"just a json string, not an object"', + content_type="text/plain", + ) + assert "just a json string" not in str(exc_info.value) + assert "redacted" in str(exc_info.value) + + def test_topics_list_endpoint(client, django_assert_num_queries): """Test topics list endpoint""" topics = sorted( diff --git a/main/middleware/apisix_user_test.py b/main/middleware/apisix_user_test.py index f7d6df630c..46e72de23d 100644 --- a/main/middleware/apisix_user_test.py +++ b/main/middleware/apisix_user_test.py @@ -9,6 +9,7 @@ from django.contrib.auth import get_user_model from django.contrib.auth.models import AnonymousUser from django.db import close_old_connections +from django.urls import reverse from main.constants import PostHogEvents from main.factories import UserFactory @@ -37,12 +38,16 @@ def mock_login(mocker): @pytest.fixture(autouse=True) def userinfo_flag_defaults(settings): """ - Turn both userinfo create/update flags on, so the tests that exercise the full - create-and-sync behavior get it regardless of the setting defaults or of whatever - is set in backend.local.env. Tests for the disabled paths override these. + Turn both userinfo create/update flags on, and ensure the middleware itself + is enabled, so the tests that exercise the full create-and-sync behavior get + it regardless of the setting defaults or of whatever is set in + backend.local.env (which defaults DISABLE_APISIX_USER_MIDDLEWARE to True for + local dev/codespaces -- see env/backend.env). Tests for the disabled paths + override these. """ settings.MITOL_APIGATEWAY_USERINFO_CREATE = True settings.MITOL_APIGATEWAY_USERINFO_UPDATE = True + settings.DISABLE_APISIX_USER_MIDDLEWARE = False @pytest.fixture(autouse=True) @@ -358,6 +363,21 @@ def test_userinfo_update_disabled_skips_writes(mocker, settings, synced_user, ch assert get_attr(reloaded) == original +@pytest.mark.django_db(transaction=True) +def test_disabled_middleware_ignores_forged_header(client, settings): + """ + With the middleware disabled (the local-dev/codespaces default, since + nothing there actually verifies the header came from a real APISIX/ + Keycloak login), a forged X-Userinfo header must not authenticate or + create a user. + """ + settings.DISABLE_APISIX_USER_MIDDLEWARE = True + header = b64encode(json.dumps(apisix_user_info).encode()) + resp = client.get(reverse("profile:v0:users_api-me"), HTTP_X_USERINFO=header) + assert resp.json()["is_authenticated"] is False + assert not User.objects.filter(global_id=apisix_user_info["sub"]).exists() + + @pytest.mark.django_db(transaction=True) def test_userinfo_update_disabled_skips_profile_creation(mocker, mock_login, settings): """Parity with mitol-django-apigateway: known users get no profile writes at all.""" diff --git a/main/sentry.py b/main/sentry.py index 4304b3b98e..6ac546ca7e 100644 --- a/main/sentry.py +++ b/main/sentry.py @@ -5,6 +5,7 @@ import sentry_sdk from celery.exceptions import WorkerLostError +from django.conf import settings from sentry_sdk.integrations.boto3 import Boto3Integration from sentry_sdk.integrations.celery import CeleryIntegration from sentry_sdk.integrations.django import DjangoIntegration @@ -46,6 +47,11 @@ def scrub_pg_detail(text): def scrub_pg_details(event): """Truncate Postgres DETAIL lines everywhere in a Sentry event. + Despite the name, also redacts OCW_WEBHOOK_KEY wherever it appears (see + scrub_ocw_webhook_key) -- both scrubs share the same recursive walk since + they're the same problem: a sensitive value ending up in a place no SDK + privacy setting reaches. + The row echo reaches Sentry through more fields than the exception value: LoggingIntegration puts the log message in a breadcrumb (BreadcrumbHandler._breadcrumb_from_record), logger.error("...: %s", exc) @@ -61,10 +67,27 @@ def scrub_pg_details(event): return _scrub_node(event) +def scrub_ocw_webhook_key(text): + """Redact the OCW webhook shared secret if it appears verbatim in a string. + + OCW_WEBHOOK_KEY is unlike most secrets: a caller proves they know it by + literally sending it as request data (WebhookOCWView.post checks it + against content["webhook_key"]), so a genuinely authenticated request's + own body -- or a captured frame variable holding that body -- can carry + the real value into Sentry via request.data or frame locals, regardless + of what a view's own error handling redacts. + See https://github.com/mitodl/hq/issues/13470. + """ + key = settings.OCW_WEBHOOK_KEY + if key and key in text: + return text.replace(key, "[redacted]") + return text + + def _scrub_node(node): """Recurse through the serialized event, rewriting strings in place.""" if isinstance(node, str): - return scrub_pg_detail(node) + return scrub_ocw_webhook_key(scrub_pg_detail(node)) if isinstance(node, dict): for key, value in node.items(): node[key] = _scrub_node(value) diff --git a/main/sentry_test.py b/main/sentry_test.py index 96cbe47e5c..7ffde3e69a 100644 --- a/main/sentry_test.py +++ b/main/sentry_test.py @@ -5,11 +5,15 @@ import pytest import sentry_sdk +from rest_framework.reverse import reverse +from sentry_sdk.integrations.django import DjangoIntegration from sentry_sdk.integrations.logging import LoggingIntegration from sentry_sdk.transport import Transport +from learning_resources.exceptions import WebhookException from main.sentry import ( before_send, + scrub_ocw_webhook_key, scrub_pg_detail, scrub_pg_details, ) @@ -57,6 +61,31 @@ def sentry_transport(): sentry_sdk.get_global_scope().set_client(None) +@pytest.fixture +def sentry_transport_with_django(): + """ + Like sentry_transport, but with DjangoIntegration active -- needed to + reproduce request.data actually reaching the captured event, matching + what init_sentry() configures for real. The other tests in this file + don't need request context, so this stays separate rather than changing + the shared fixture they use. + """ + transport = FakeTransport() + sentry_sdk.init( + dsn="https://k@o0.ingest.sentry.io/0", + transport=transport, + before_send=before_send, + max_request_body_size="small", + default_integrations=False, + integrations=[ + LoggingIntegration(level=logging.INFO, event_level=logging.ERROR), + DjangoIntegration(), + ], + ) + yield transport + sentry_sdk.get_global_scope().set_client(None) + + def test_detail_line_is_truncated(): """The row echo goes; the primary error that names the failure stays.""" scrubbed = scrub_pg_detail(PG_INTEGRITY_ERROR) @@ -196,3 +225,84 @@ def save(): assert len(sentry_transport.events) == 2 for event in sentry_transport.events: assert "learner@example.invalid" not in json.dumps(event) + + +def test_scrub_ocw_webhook_key_redacts(settings): + """The key is replaced wherever it appears verbatim in a string.""" + settings.OCW_WEBHOOK_KEY = "supersecretvalue123" + text = '{"webhook_key": "supersecretvalue123", "prefixes": 12345}' + scrubbed = scrub_ocw_webhook_key(text) + assert "supersecretvalue123" not in scrubbed + assert "[redacted]" in scrubbed + assert "12345" in scrubbed + + +def test_scrub_ocw_webhook_key_unset(settings): + """No configured key means nothing to redact, and the text is untouched.""" + settings.OCW_WEBHOOK_KEY = None + text = "nothing secret here" + assert scrub_ocw_webhook_key(text) == text + + +def test_scrubs_ocw_webhook_key_from_frame_locals_and_request_data(settings): + """ + The key must be redacted wherever the SDK independently captures it -- + frame locals and request.data -- not just from a message the view + constructs, since fixing the view's own message doesn't stop the SDK's + own capture of the raw request from carrying the real secret separately. + """ + settings.OCW_WEBHOOK_KEY = "supersecretvalue123" + event = { + "request": {"data": {"webhook_key": "supersecretvalue123", "prefixes": 12345}}, + "exception": { + "values": [ + { + "value": "'int' object has no attribute 'split'", + "stacktrace": { + "frames": [ + { + "function": "post", + "vars": { + "content": ( + '{"webhook_key": "supersecretvalue123", ' + '"prefixes": 12345}' + ) + }, + } + ] + }, + } + ] + }, + } + scrub_pg_details(event) + assert "supersecretvalue123" not in repr(event) + + +@pytest.mark.django_db +def test_real_sdk_scrubs_ocw_webhook_key_end_to_end( + client, settings, sentry_transport_with_django +): + """ + Reproduces the exact reported scenario through the real Django view and + the real Sentry SDK (with DjangoIntegration active, so request.data is + actually attached): a correctly-authenticated request that errors after + the key check (prefixes sent as an int) must not leak the real secret via + any capture path the SDK uses, not just the view's own exception message. + """ + settings.OCW_WEBHOOK_KEY = "supersecretvalue123" + with pytest.raises(WebhookException): + client.post( + reverse("lr:v1:ocw-next-webhook"), + data={ + "webhook_key": "supersecretvalue123", + "prefixes": 12345, + "version": "live", + }, + headers={"Content-Type": "text/plain"}, + ) + sentry_sdk.flush() + + assert len(sentry_transport_with_django.events) >= 1 + for event in sentry_transport_with_django.events: + assert "supersecretvalue123" not in json.dumps(event) diff --git a/main/settings.py b/main/settings.py index db3deaa708..0add44cd13 100644 --- a/main/settings.py +++ b/main/settings.py @@ -36,7 +36,7 @@ from main.settings_pluggy import * # noqa: F403 from openapi.settings_spectacular import open_spectacular_settings -VERSION = "0.81.3" +VERSION = "0.81.4" log = logging.getLogger() diff --git a/yarn.lock b/yarn.lock index a2ae4fb94f..1c5d935704 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3631,13 +3631,13 @@ __metadata: languageName: node linkType: hard -"@mitodl/mitxonline-api-axios@npm:2026.9.23": - version: 2026.9.23 - resolution: "@mitodl/mitxonline-api-axios@npm:2026.9.23" +"@mitodl/mitxonline-api-axios@npm:2026.9.29": + version: 2026.9.29 + resolution: "@mitodl/mitxonline-api-axios@npm:2026.9.29" dependencies: "@types/node": "npm:^20.11.19" axios: "npm:^1.6.5" - checksum: 10/b0790aed3769eae57fe37034cfdb2d74da2541bfe195a219d711a08b2293fccd63c8e2fa65af9342b473978c2fc08c42171793fcf9bb47a0fea56cd5a7072c3e + checksum: 10/119780b47bb74031ce342117b92705eaedb55297fbf216cab569e003cee325f4e933773e6adec9c769609ab4d9029fa613970716b3f640a631c7ff2dd9840475 languageName: node linkType: hard @@ -9570,7 +9570,7 @@ __metadata: dependencies: "@faker-js/faker": "npm:^10.5.0" "@mitodl/mit-learn-api-axios": "npm:2026.8.17" - "@mitodl/mitxonline-api-axios": "npm:2026.9.23" + "@mitodl/mitxonline-api-axios": "npm:2026.9.29" "@tanstack/react-query": "npm:^5.66.0" "@testing-library/react": "npm:^16.3.0" axios: "npm:^1.12.2" @@ -16918,7 +16918,7 @@ __metadata: "@mitodl/arithmix": "npm:^0.2.5" "@mitodl/course-search-utils": "npm:^3.8.1" "@mitodl/hacksnack": "npm:^0.1.2" - "@mitodl/mitxonline-api-axios": "npm:2026.9.23" + "@mitodl/mitxonline-api-axios": "npm:2026.9.29" "@mitodl/smoot-design": "npm:6.38.0" "@mui/base": "npm:5.0.0-beta.70" "@mui/material": "npm:^6.4.5"