From 608a1cf05c5b217b80a8d5c7439afd31521eec48 Mon Sep 17 00:00:00 2001 From: Ahtesham Quraish Date: Wed, 19 Aug 2026 14:40:59 +0500 Subject: [PATCH 01/10] feat(settings): change email and password via Keycloak (#3726) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(settings): change email and password via Keycloak Adds "Change Email" and "Change Password" to the dashboard Settings page, behind the PostHog flag `account-management`. Both hand the user off to Keycloak using its "application initiated actions" (kc_action), then return them to Settings with a success alert. Follows the flow MITx Online already uses. Backend - AccountActionStartView redirects to Keycloak's authorization endpoint with kc_action=UPDATE_EMAIL / UPDATE_PASSWORD. - AccountActionCompleteView reads the kc_action_status Keycloak appends and passes the outcome back to the frontend as query params, which the alert consumes once and strips. - Both legs build the callback URL through one helper, because the token exchange below requires a byte-identical redirect_uri. - `email` is requested explicitly in the authorization scope. It is a Keycloak default client scope, but relying on that being true of every environment's client would fail silently and confusingly. Keeping the new address The API gateway caches userinfo in its session and only writes it at initial authentication — refreshing the access token does not refresh it — so for the life of that session (14 days in deployed environments) the X-UserInfo header keeps serving the pre-change email. Two changes make the new address stick: - On a successful email change, the callback exchanges the authorization code Keycloak sends (previously discarded) for the fresh `email` claim. The user is identified from the exchange's own `sub` rather than request.user, because ApisixUserMiddleware logs the Django user out on any request without an X-UserInfo header — which is the case on this callback when the gateway is not in front of Django. - ApisixUserMiddleware no longer re-applies the header's email once a session is established. mitxonline avoids this by skipping all field syncing for an already authenticated user, relying on Keycloak's SCIM push as a backchannel; Learn has no SCIM provisioning, so only email is held back and every other field still syncs. SSO users Users who authenticate through an external identity provider cannot change these credentials here, so the section is hidden from them and the start view independently re-checks. Detection reads federated identities from the Keycloak Admin API, cached per user. It needs MITOL_KEYCLOAK_ADMIN_CLIENT_* which no deployed environment sets yet (see the companion ol-infrastructure PR); until then it fails open and Keycloak remains the only gate. Admin API timeouts is_sso_user runs while serializing the current user, so it is on every page load, and pushing the email opt-in runs on a profile PATCH and on one-click unsubscribe. python-keycloak defaults to a 60 second timeout and mitol.keycloak.api.get_admin_client does not expose one, so main/keycloak.py builds the client with a 5 second timeout and both call sites use it. This also means profiles.api.sync_email_optin_to_keycloak — which has never run in any deployed environment because the admin client was never configured — becomes safe to enable. Local development - Keycloak needs the `update-email` feature flag; UPDATE_EMAIL is a preview action, and without it Keycloak fails with a generic error page whose only clue is a NullPointerException in its logs. - The realm export registers UPDATE_EMAIL and sets the ol-learn theme so local login pages match production. `--import-realm` skips realms that already exist, so pre-existing setups need it applied once by hand. - scripts/fetch_keycloak_theme.sh lifts the ol-learn theme jar out of the mitodl/keycloak image rather than running that image locally, which tracks a newer Keycloak than docker-compose pins and would irreversibly migrate the local realm database. - Fixes the themes volume mount, which pointed at the WildFly path and was silently ignored by Keycloak 26. Co-Authored-By: Claude Opus 5 (1M context) * fix: regenerate OpenAPI spec after get_email docstring change The CurrentUserSerializer.get_email docstring was reworded to satisfy ruff's imperative-mood rule (D401) after the spec had been generated, so the committed spec and TS client still carried the old wording. drf-spectacular derives the field description from that docstring, so openapi_spec_check.sh failed. Regenerated with ./scripts/generate_openapi.sh; the only change is that one description line in both the spec and the generated client. Co-Authored-By: Claude Opus 5 (1M context) * fix: don't 500 when KEYCLOAK_CLIENT_ID is unset, and pin it in tests CI surfaced two problems that local runs hid, because env/backend.env supplies KEYCLOAK_CLIENT_ID and MITOL_API_BASE_URL locally while CI leaves both unset. The first is a real bug. KEYCLOAK_CLIENT_ID has no default, and urlencode raises TypeError on a None value, so in any environment that hasn't configured the client every click on Change Email or Change Password would have been a server error. AccountActionStartView now checks for it and returns the user to Settings with an error alert, matching how it already handles an unrecognized action. The second was the tests' fault: they asserted against settings.KEYCLOAK_CLIENT_ID and settings.MITOL_API_BASE_URL, so they described whatever the environment happened to provide rather than fixed expectations. An autouse fixture now pins both, plus KEYCLOAK_BASE_URL and the realm name. Verified by running the suites with those variables explicitly emptied to reproduce CI: 319 passed. Co-Authored-By: Claude Opus 5 (1M context) * refactor: parse the account action explicitly instead of enum membership Review feedback flagged `action not in AccountAction` as unable to match a plain query-string value, which would make the callback skip both status reporting and the email sync. That isn't the case here — Python 3.12 extended enum membership to accept values, and test_account_action_complete plus test_account_action_complete_syncs_email cover the reporting and sync branches. The concern is fair regardless: the behaviour is version-dependent (before 3.12 the same expression raises TypeError rather than returning False) and it isn't obvious to a reader, as this review shows. Replaced with an explicit parse_account_action() lookup that returns the member or None, and kept the raw value for the log message so an unrecognised action is still visible. parse_account_action is covered directly, including the empty string and None, so the branch this feedback was about can't regress silently. AccountActionStartView is left alone: it tests membership against the KEYCLOAK_ACTIONS dict, and dict lookup by string key against StrEnum keys works on every version because StrEnum members hash equal to their values. Co-Authored-By: Claude Opus 5 (1M context) * fix: don't claim the email changed when it is only pending confirmation All deployed realms have verify_email enabled, which makes Keycloak's kc_action_status=success mean "confirmation email sent", not "address changed". The address only changes when the user clicks the link, and that leg reaches our callback with just the two params we put in the redirect URI ourselves — no authorization code, no status — so the exchange cannot run there. The result was an alert saying "Your email address has been updated" at a point where nothing had changed, followed by Settings continuing to show the old address. Reported as the feature being broken, and fairly so. The callback now derives the message from the authoritative address instead of from Keycloak's status: it only reports success if reading the email back proves it changed, and otherwise downgrades to a new `pending` status whose copy tells the user to check their inbox. This is correct whether or not verify_email is on, so it also covers the verification-off and already-verified-bypass paths where the change does apply immediately. Also logs which params a callback carried when they are unusable. That is what identified the confirmation leg as carrying no code; names only, since these params carry authorization codes and addresses. This does not close the data gap: when the user confirms via the link, nothing notifies us, so the stored email stays stale until the gateway session turns over. Closing that needs Keycloak to push the change over SCIM, which is configured outside this repo. Tracked separately. Co-Authored-By: Claude Opus 5 (1M context) * fix: read the new email after the confirmation link is clicked Closes the data gap left by the previous commit. With verify_email on — all deployed environments — the address changes when the user clicks the link in Keycloak's confirmation email, and that leg returns them to our callback through a plain hyperlink carrying only the params we put in the redirect URI ourselves. No authorization code, so there was nothing to exchange, and the change never reached us. Confirmed by logging which params that leg carries: account_action and next, nothing from Keycloak. The visible symptom was Settings showing the previous address indefinitely. In deployed environments it would have persisted until the gateway session turned over, which is 14 days. The callback now recognises that leg and makes one silent authorization round trip — no kc_action, prompt=none — purely to obtain claims it can read. Keycloak answers immediately from the existing SSO session, and the resulting code is exchanged for the current address. A marker param on the redirect URI stops it repeating if no code comes back, which is what happens when prompt=none finds no session. The marker has to be part of the redirect URI passed to the token exchange too, since Keycloak requires that to match the authorization request byte for byte. Verified against local Keycloak with verify_email enabled and Mailpit capturing the mail: the confirmation leg redirects to the authorization endpoint with prompt=none and the marker, and a marked callback without a code returns to Settings rather than bouncing again. SCIM would remove the need for this by having Keycloak push the change, and is still worth doing — it would also cover changes made outside this flow. It is configured outside this repo, so it is tracked separately rather than blocking. Co-Authored-By: Claude Opus 5 (1M context) * Revert "fix: read the new email after the confirmation link is clicked" The silent re-authorization does not work. It assumed the user still has a Keycloak session when they return from the confirmation link, so that prompt=none would answer immediately with a code. They do not: clicking an action-token link authenticates only for that action and leaves no browser SSO session, and the update-email form's "Sign out from other devices" is checked by default. Observed on the refreshed callback: params present: account_action,account_action_refreshed,error,next Keycloak returns an error rather than a code, the guard stops the retry, and the address is still not read. So the approach added a redirect that reliably fails on the one leg it existed for. Reverting rather than iterating: with no code, no session and no gateway header, that leg carries no way to identify the user at all, so nothing can be read from Keycloak there without new inputs. The messaging fix in the preceding commit stands — it is what stops the flow claiming the address changed when it has not. Co-Authored-By: Claude Opus 5 (1M context) * feat(local-dev): capture Keycloak email locally with Mailpit Deployed realms have verify_email enabled, which changes the change-email flow materially: submitting the form does not change the address, Keycloak emails a confirmation link and only applies it once that link is clicked. The local realm had verification off and no SMTP, so local testing exercised a flow that does not exist in any deployed environment — which is how the flow shipped for review with a success message that fires before anything has changed. Adds Mailpit to the keycloak profile and points the realm's SMTP at it, with verifyEmail on to match deployed realms. Nothing leaves the machine, so any address works and the confirmation link is read at http://localhost:8025. Also fixes the themes volume mount, which pointed at /opt/jboss — the WildFly distribution's path, silently ignored by Keycloak 26. Note for existing setups: --import-realm skips realms that already exist, so the SMTP settings and verifyEmail need applying by hand once. Documented, along with the confusing 500 from Keycloak's "Test connection" button when the admin user has no email address of its own. Co-Authored-By: Claude Opus 5 (1M context) * docs: record the Keycloak SCIM request for Learn Changing an email in a realm with verify_email on — all deployed environments — applies when the user clicks the confirmation link, and that leg returns to Learn with no authorization code and no session, so Learn cannot observe it. The stored address then lags until the gateway session turns over, which is 14 days. Keycloak already pushes user changes to MITx Online over SCIM. Learn has the receiving side but no push configured, and the plugin's targets live in its own admin backend rather than in ol-infrastructure, so this is a configuration request rather than a pull request. Writing it down here so it doesn't live in a chat log. Includes what was verified locally rather than assumed: pushing a replace operation to /scim/v2/Users/ returns 200, stores the address, and survives a subsequent request carrying a stale gateway header. Also records that mitol.scim only accepts scim-for-keycloak's non-compliant payload shape — path absent, value as a JSON-encoded string — because the spec-compliant form returns a 500, and that is easy to lose an hour to. Co-Authored-By: Claude Opus 5 (1M context) * feat: provision_scim_client command for inbound SCIM credentials Keycloak needs credentials before it can push user changes into Learn, and Learn had no way to create them — no OAuth2 application provisioning anywhere in the repo, and nothing that mints a service token. This adds a command so it is repeatable across environments instead of clicks in Django admin. It creates a staff service user, an OAuth2 application, and a bearer token bound to that user, then prints the values whoever configures the Keycloak SCIM provider needs. Idempotent, with --rotate-token for rotation or a lost token, since the token is only displayed when issued. The token has to be bound to a user. Learn's SCIM endpoints are guarded by Learn's own OAuth2 provider: OAuth2TokenMiddleware resolves the token to a user and mitol.scim.utils.is_authenticated_predicate then requires that user to be active and staff. A client_credentials token belongs to the application rather than a person, so AccessToken.user is null and the push is rejected with a 401 — verified by trying it, which is why the earlier suggestion of CLIENT_CREDENTIALS_GRANT in the request doc was wrong and is corrected here. The plugin's auth type must be BEARER. Verified locally end to end: the provisioned token authenticates a replace operation against /scim/v2/Users/, returns 200, and the pushed address is stored. Co-Authored-By: Claude Opus 5 (1M context) * docs: rollout runbook for change email / change password The work spans three pull requests across two repositories, a management command run per environment, and one piece of Keycloak configuration owned by another team, with a hard ordering constraint in the middle. That is too much to carry in a chat log or a PR comment, so it lives here. Records the ordering rules and why each exists, most importantly that the mit_learn app stack must not be applied before the Keycloak substructure stack: the secret it reads would not exist yet, and secrets are attached with envFrom.secretRef without optional: true, so new pods fail with CreateContainerConfigError and the rollout stalls. Also corrects the provisioning command's own output. It claimed the bearer token is shown only once, which is untrue: AccessToken.token is stored unhashed, unlike Application.client_secret, so a mislaid token can be read back rather than rotated. Rotating breaks whatever Keycloak config still holds the old value, so the distinction matters. Co-Authored-By: Claude Opus 5 (1M context) * refactor: use the library's Keycloak admin client directly The Admin API timeout moves upstream to mitol-django-keycloak (mitodl/ol-django#535), which adds a timeout argument and a MITOL_KEYCLOAK_ADMIN_TIMEOUT setting to get_admin_client(). That removes the reason main/keycloak.py existed. Its update_user_attributes was a verbatim copy of the library's update_user, differing only in using the locally-built client, so both call sites now use mitol.keycloak.api directly. Until that release lands, is_sso_user falls back to python-keycloak's 60 second default. It short-circuits before making a call whenever the admin client is unconfigured, which is every environment today. Co-Authored-By: Claude Opus 5 (1M context) * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * refactor: narrow to the settings UI and the backend it needs Reduces this to the change-email/change-password UI plus the endpoints and serializer fields behind it, per review. Keeping a changed address in step with Keycloak is dropped here in favour of SCIM. That removes: - the ApisixUserMiddleware change, superseded by #3747, which makes userinfo updates togglable so deployed environments stop overwriting SCIM - fetch_keycloak_userinfo/sync_email_from_keycloak, which read the new address back from the authorization code because the gateway header was stale - KEYCLOAK_CLIENT_SECRET, used only by that token exchange Without the exchange the callback can't tell an applied change from one still awaiting its confirmation link, so the AccountActionStatus.PENDING state goes and the update-email success copy now points at the inbox rather than claiming the address already changed. Also moved out: the SCIM client provisioning command, which belongs with enabling SCIM rather than with this UI, and the rollout runbook, which described a sequence this no longer follows. The local Keycloak setup keeps only what the flow cannot run without — the update-email preview feature and the UPDATE_EMAIL required action. Co-Authored-By: Claude Opus 5 (1M context) * address feedback * feat: store is_sso_user on the User model instead of in the cache Adds a nullable User.is_sso_user, filled in from Keycloak the first time it is needed — in practice on the first GET of users/me — and read straight from the row afterwards. A field rather than a cache entry because the value effectively never changes on its own, because how we decide it may change, and because it can then be overridden: clearing the flag grants someone local credentials, so they can keep an account after leaving the organization that provided their identity. Exposed in the user admin for that reason. Null means undetermined rather than false, so a Keycloak failure or an unconfigured admin client leaves it null and a later read retries instead of recording a guess. Co-Authored-By: Claude Opus 5 (1M context) * address the feedback * refactor: expose the current user's email as a plain read-only field Replaces the SerializerMethodField with a field declaration, per review. The default is load-bearing: users/me serializes AnonymousUser as well as a real user, AnonymousUser has no email attribute, and a read-only field whose attribute is missing is dropped from the output rather than rendered empty — which is why first_name and last_name are already absent for anonymous users. Without the default, `email` disappeared from that response. Regenerates the spec and client: `email` stays required, so its TypeScript type is unchanged; only the method docstring that served as the field description goes away. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Ahtesham Quraish Co-authored-by: Claude Opus 5 (1M context) Co-authored-by: Ahtesham Quraish Co-authored-by: Ahtesham Quraish Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Ahtesham Quraish Co-authored-by: Ahtesham Quraish --- README-keycloak.md | 27 +++ authentication/api.py | 51 +++++ authentication/api_test.py | 119 +++++++++++ authentication/constants.py | 61 ++++++ authentication/urls.py | 19 +- authentication/views.py | 177 +++++++++++++++++ authentication/views_test.py | 184 +++++++++++++++++- config/keycloak/realms/ol-local-realm.json | 9 + docker-compose.services.yml | 2 +- drf_lint_baseline.json | 4 +- frontends/api/src/generated/v0/api.ts | 79 +++++++- frontends/api/src/hooks/user/index.ts | 4 +- .../api/src/test-utils/factories/user.ts | 10 +- .../DashboardPage/AccountActionAlert.test.tsx | 75 +++++++ .../DashboardPage/AccountActionAlert.tsx | 107 ++++++++++ .../DashboardPage/SettingsContent.test.tsx | 82 ++++++++ .../DashboardPage/SettingsContent.tsx | 81 +++++++- frontends/main/src/common/feature_flags.ts | 1 + frontends/main/src/common/urls.test.ts | 29 +++ frontends/main/src/common/urls.ts | 36 ++++ frontends/main/src/test-utils/index.tsx | 8 +- main/settings.py | 10 + openapi/specs/v0.yaml | 59 +++++- profiles/serializers.py | 27 +++ profiles/views.py | 3 +- profiles/views_test.py | 4 + users/admin.py | 12 ++ users/migrations/0012_user_is_sso_user.py | 21 ++ users/models.py | 17 +- 29 files changed, 1297 insertions(+), 21 deletions(-) create mode 100644 authentication/constants.py create mode 100644 frontends/main/src/app-pages/DashboardPage/AccountActionAlert.test.tsx create mode 100644 frontends/main/src/app-pages/DashboardPage/AccountActionAlert.tsx create mode 100644 users/migrations/0012_user_is_sso_user.py diff --git a/README-keycloak.md b/README-keycloak.md index 559ab10e03..994eac0527 100644 --- a/README-keycloak.md +++ b/README-keycloak.md @@ -51,6 +51,33 @@ follow these steps: 2. Add `DISABLE_APISIX_USER_MIDDLEWARE=True` to your `backend.local.env` file 3. Set `COMPOSE_PROFILES=backend,frontend` in your .env file +### Changing email and password + +The settings page at `/dashboard/settings` lets users change their email and +password by handing off to Keycloak ("application initiated actions"). Two +things have to be in place for `kc_action=UPDATE_EMAIL` to work: + +1. Keycloak must be started with the `update-email` feature. `UPDATE_EMAIL` is + still a preview feature, so it is listed explicitly in the `keycloak` + service's `--features` flag in `docker-compose.services.yml`. +2. The `UPDATE_EMAIL` required action must be registered in the realm. It is in + `config/keycloak/realms/ol-local-realm.json`, but `--import-realm` skips + realms that already exist in the database. If you set Keycloak up before this + was added, register it once via Keycloak admin + (Authentication → Required actions → Register → Update Email), or reset the + Keycloak database so the realm re-imports. + +Without both, Keycloak fails the request with a generic "Unexpected error when +handling authentication request to identity provider" page, and its logs show +`NullPointerException ... "requiredActionProvider" is null`. + +`UPDATE_PASSWORD` is a built-in action and needs neither step. + +Note that deployed realms have `verify_email` enabled, so submitting the form +there emails a confirmation link rather than changing the address immediately, +and the new address reaches Learn when Keycloak pushes it over SCIM. The local +realm has verification off, so the change applies straight away. + ### MITx Online integration The user dashboard at `/dashboard` includes some integration with the MITx Online diff --git a/authentication/api.py b/authentication/api.py index 396eb0dd8f..75b3e596cb 100644 --- a/authentication/api.py +++ b/authentication/api.py @@ -4,6 +4,9 @@ from django.contrib.auth import get_user_model from django.db import transaction +from keycloak.exceptions import KeycloakError +from mitol.keycloak import api as keycloak_api +from requests.exceptions import RequestException from authentication.hooks import get_plugin_manager from profiles import api as profile_api @@ -39,6 +42,54 @@ def create_user(username, email, profile_data=None, user_extra=None): return user +def is_sso_user(user) -> bool: + """ + Return True if the user signs in through an external identity provider. + + Such users have no local Keycloak credentials, so they can't change their + email or password here — that lives with their institution. + + The answer lives on `User.is_sso_user`, which starts null and is filled in + the first time it is needed. Once set it is authoritative and Keycloak is + not consulted again: the value effectively never changes on its own, and + keeping it editable means someone can be granted local credentials — for + instance to keep their account after leaving the organization that provided + their identity — by clearing the flag. + + Determining it requires the Keycloak admin client. When that isn't + configured (e.g. local development) we can't tell, so we report False + without storing anything and let Keycloak be the final arbiter. + """ + stored = getattr(user, "is_sso_user", None) + if stored is not None: + return stored + + global_id = getattr(user, "global_id", None) + if not global_id: + return False + + if not keycloak_api.is_admin_client_configured(): + log.debug( + "Keycloak admin client is not configured; cannot determine whether " + "user %s is an SSO user", + user.id, + ) + return False + + try: + admin_client = keycloak_api.get_admin_client() + is_sso = bool(admin_client.get_user_social_logins(global_id)) + except (KeycloakError, RequestException): + # Leave the field null so the next read tries again, rather than + # recording a guess. + log.exception("Failed to fetch federated identities for user %s", user.id) + return False + + user.is_sso_user = is_sso + user.save(update_fields=["is_sso_user", "updated_on"]) + return is_sso + + def user_created_actions(*, user, details, **kwargs): """ Trigger plugins when a user is created diff --git a/authentication/api_test.py b/authentication/api_test.py index aef0f9fafe..ffd8a86ea4 100644 --- a/authentication/api_test.py +++ b/authentication/api_test.py @@ -1,9 +1,13 @@ """API tests""" +from uuid import uuid4 + import pytest from django.contrib.auth import get_user_model +from keycloak.exceptions import KeycloakError from authentication import api +from authentication.constants import AccountAction, parse_account_action from main.factories import UserFactory from profiles.models import Profile @@ -70,3 +74,118 @@ def test_user_created_actions(mocker, is_new): api.user_created_actions(**kwargs) assert user.user_lists.count() == (1 if is_new else 0) + + +@pytest.fixture +def mock_keycloak_admin(mocker): + """Mock the Keycloak admin client used for federated identity lookups""" + mocker.patch( + "authentication.api.keycloak_api.is_admin_client_configured", + return_value=True, + ) + return mocker.patch("authentication.api.keycloak_api.get_admin_client").return_value + + +@pytest.mark.parametrize( + ("social_logins", "expected"), + [ + ([{"identityProvider": "touchstone"}], True), + ([], False), + ], +) +def test_is_sso_user(mock_keycloak_admin, social_logins, expected): + """Users with a federated identity in Keycloak are SSO users""" + user = UserFactory.create(global_id=uuid4().hex, is_sso_user=None) + mock_keycloak_admin.get_user_social_logins.return_value = social_logins + + assert api.is_sso_user(user) is expected + mock_keycloak_admin.get_user_social_logins.assert_called_once_with(user.global_id) + + # the answer is recorded, not just returned + user.refresh_from_db() + assert user.is_sso_user is expected + + +def test_is_sso_user_only_asks_keycloak_once(mock_keycloak_admin): + """Once stored on the user, Keycloak isn't consulted again""" + user = UserFactory.create(global_id=uuid4().hex, is_sso_user=None) + mock_keycloak_admin.get_user_social_logins.return_value = [ + {"identityProvider": "touchstone"} + ] + + assert api.is_sso_user(user) is True + assert api.is_sso_user(user) is True + assert mock_keycloak_admin.get_user_social_logins.call_count == 1 + + +@pytest.mark.parametrize("stored", [True, False]) +def test_is_sso_user_stored_value_wins(mock_keycloak_admin, stored): + """ + An explicitly set flag overrides Keycloak. + + This is what lets someone keep an account after leaving the organization + that provided their identity: clear the flag and they can manage their own + email and password, whatever Keycloak still reports. + """ + user = UserFactory.create(global_id=uuid4().hex, is_sso_user=stored) + mock_keycloak_admin.get_user_social_logins.return_value = [ + {"identityProvider": "touchstone"} + ] + + assert api.is_sso_user(user) is stored + mock_keycloak_admin.get_user_social_logins.assert_not_called() + + +def test_is_sso_user_no_global_id(mock_keycloak_admin): + """Users who have never been through Keycloak can't be SSO users""" + user = UserFactory.create(global_id=None, is_sso_user=None) + + assert api.is_sso_user(user) is False + mock_keycloak_admin.get_user_social_logins.assert_not_called() + + +def test_is_sso_user_admin_client_unconfigured(mocker): + """Without an admin client we can't tell, so we don't block the user""" + mocker.patch( + "authentication.api.keycloak_api.is_admin_client_configured", + return_value=False, + ) + get_admin_client = mocker.patch("authentication.api.keycloak_api.get_admin_client") + user = UserFactory.create(global_id=uuid4().hex, is_sso_user=None) + + assert api.is_sso_user(user) is False + get_admin_client.assert_not_called() + user.refresh_from_db() + assert user.is_sso_user is None + + +def test_is_sso_user_keycloak_error(mock_keycloak_admin): + """A Keycloak failure shouldn't take the settings page down with it""" + user = UserFactory.create(global_id=uuid4().hex, is_sso_user=None) + mock_keycloak_admin.get_user_social_logins.side_effect = KeycloakError("boom") + + assert api.is_sso_user(user) is False + # left null so a later read retries rather than recording a guess + user.refresh_from_db() + assert user.is_sso_user is None + + +@pytest.mark.parametrize( + ("value", "expected"), + [ + ("update-email", AccountAction.UPDATE_EMAIL), + ("update-password", AccountAction.UPDATE_PASSWORD), + ("delete-account", None), + ("", None), + (None, None), + ], +) +def test_parse_account_action(value, expected): + """ + A raw query-string value maps to its AccountAction, or None. + + Pins that a plain string is recognised: `value in AccountAction` only + accepts values from Python 3.12 onwards and raises TypeError before that, + so the callback's reporting branch depends on this. + """ + assert parse_account_action(value) is expected diff --git a/authentication/constants.py b/authentication/constants.py new file mode 100644 index 0000000000..2625522d32 --- /dev/null +++ b/authentication/constants.py @@ -0,0 +1,61 @@ +"""Authentication constants""" + +from enum import StrEnum + +# Query params appended to the frontend URL the user lands back on after a +# Keycloak account action completes. The frontend consumes these once to show a +# success/error alert and then strips them from the URL. +ACCOUNT_ACTION_PARAM = "account_action" +ACCOUNT_ACTION_STATUS_PARAM = "account_action_status" + +# Frontend path an account action starts from and returns to. +ACCOUNT_SETTINGS_PATH = "/dashboard/settings" + + +class AccountAction(StrEnum): + """Account actions a user can start from the settings page""" + + UPDATE_EMAIL = "update-email" + UPDATE_PASSWORD = "update-password" # noqa: S105 + + +def parse_account_action(value: str | None) -> "AccountAction | None": + """ + Return the AccountAction matching a raw query-string value, or None. + + Prefer this over `value in AccountAction`: membership tests against an enum + only accept plain values from Python 3.12 onwards, and raise TypeError + before that, so the explicit lookup keeps the intent obvious and pins the + behaviour regardless of interpreter version. + """ + try: + return AccountAction(value) + except ValueError: + return None + + +class AccountActionStatus(StrEnum): + """Outcome of an account action, as reported back to the frontend""" + + SUCCESS = "success" + CANCELLED = "cancelled" + ERROR = "error" + # The user authenticates through an external identity provider, so the + # action isn't theirs to perform. + UNAVAILABLE = "unavailable" + + +# Maps our URL slugs onto Keycloak's `kc_action` values. +KEYCLOAK_ACTIONS = { + AccountAction.UPDATE_EMAIL: "UPDATE_EMAIL", + AccountAction.UPDATE_PASSWORD: "UPDATE_PASSWORD", +} + +# Keycloak appends this to the redirect URI when an application initiated action +# finishes, with one of the values below. +KEYCLOAK_ACTION_STATUS_PARAM = "kc_action_status" +KEYCLOAK_ACTION_STATUSES = { + "success": AccountActionStatus.SUCCESS, + "cancelled": AccountActionStatus.CANCELLED, + "error": AccountActionStatus.ERROR, +} diff --git a/authentication/urls.py b/authentication/urls.py index 5be5f98353..28852cb6f6 100644 --- a/authentication/urls.py +++ b/authentication/urls.py @@ -1,10 +1,25 @@ """URL configurations for authentication""" -from django.urls import re_path +from django.urls import path, re_path -from authentication.views import CustomLoginView, CustomLogoutView +from authentication.views import ( + AccountActionCompleteView, + AccountActionStartView, + CustomLoginView, + CustomLogoutView, +) urlpatterns = [ re_path(r"^logout", CustomLogoutView.as_view(), name="logout"), re_path(r"^login", CustomLoginView.as_view(), name="login"), + path( + "account/action/start//", + AccountActionStartView.as_view(), + name="account-action-start", + ), + path( + "account/action/complete", + AccountActionCompleteView.as_view(), + name="account-action-complete", + ), ] diff --git a/authentication/views.py b/authentication/views.py index ccd3b34015..e34a3e35c3 100644 --- a/authentication/views.py +++ b/authentication/views.py @@ -1,13 +1,27 @@ """Authentication views""" import logging +from urllib.parse import urlparse, urlunparse from django.conf import settings from django.contrib.auth import logout from django.shortcuts import redirect +from django.urls import reverse from django.utils.http import url_has_allowed_host_and_scheme, urlencode from django.views import View +from django.views.generic.base import RedirectView +from authentication.api import is_sso_user +from authentication.constants import ( + ACCOUNT_ACTION_PARAM, + ACCOUNT_ACTION_STATUS_PARAM, + ACCOUNT_SETTINGS_PATH, + KEYCLOAK_ACTION_STATUS_PARAM, + KEYCLOAK_ACTION_STATUSES, + KEYCLOAK_ACTIONS, + AccountActionStatus, + parse_account_action, +) from main.middleware.apisix_user import ApisixUserMiddleware, decode_apisix_headers from profiles.tasks import send_welcome_email @@ -106,3 +120,166 @@ def get( profile.save() return redirect(redirect_url) + + +def get_account_action_redirect_url(request): + """ + Get the frontend URL to return the user to once an account action finishes. + + Defaults to the dashboard settings page, where the action was started from. + """ + next_url = request.GET.get("next") + if next_url and url_has_allowed_host_and_scheme( + next_url, allowed_hosts=settings.ALLOWED_REDIRECT_HOSTS + ): + return next_url + + return f"{settings.APP_BASE_URL.removesuffix('/')}{ACCOUNT_SETTINGS_PATH}" + + +def with_query_params(url, params): + """Return url with params merged into its query string""" + parsed = urlparse(url) + query = f"{parsed.query}&{urlencode(params)}" if parsed.query else urlencode(params) + return urlunparse(parsed._replace(query=query)) + + +def build_account_action_callback_url(request, *, next_url, action): + """ + Build the callback URL Keycloak returns the user to. + + Both legs of the flow must produce a byte-identical string: it is sent as + `redirect_uri` in the authorization request, and again when exchanging the + authorization code, where Keycloak requires an exact match. + + Keycloak returns the user to the API domain, which is where APISIX (and so + the OIDC client's registered redirect URIs) lives. + """ + callback_path = reverse("account-action-complete") + base_url = ( + f"{settings.MITOL_API_BASE_URL.removesuffix('/')}{callback_path}" + if settings.MITOL_API_BASE_URL + else request.build_absolute_uri(callback_path) + ) + return with_query_params(base_url, {"next": next_url, ACCOUNT_ACTION_PARAM: action}) + + +class AccountActionStartView(RedirectView): + """ + Send the user to Keycloak to update their email or password. + + Keycloak calls these "application initiated actions": we start a normal + authorization code flow with a `kc_action` param, Keycloak walks the user + through the relevant form, and then returns them to our callback. + """ + + def get_redirect_url(self, *args, **kwargs): # noqa: ARG002 + """Get the Keycloak URL for the requested action""" + action = kwargs["action"] + next_url = get_account_action_redirect_url(self.request) + + if action not in KEYCLOAK_ACTIONS: + log.error("Received unexpected account action: %s", action) + return with_query_params( + next_url, + { + ACCOUNT_ACTION_PARAM: action, + ACCOUNT_ACTION_STATUS_PARAM: AccountActionStatus.ERROR, + }, + ) + + if not settings.KEYCLOAK_CLIENT_ID: + # Nothing to build an authorization request with. Send the user back + # with an error rather than raising: KEYCLOAK_CLIENT_ID has no + # default, so an environment that hasn't set it would otherwise + # 500 on every click. + log.error( + "KEYCLOAK_CLIENT_ID is not configured; cannot start account action %s", + action, + ) + return with_query_params( + next_url, + { + ACCOUNT_ACTION_PARAM: action, + ACCOUNT_ACTION_STATUS_PARAM: AccountActionStatus.ERROR, + }, + ) + + if self.request.user.is_authenticated and is_sso_user(self.request.user): + # SSO users have no local Keycloak credentials to change. The UI + # hides these actions from them, so this is the backstop for a + # hand-crafted or stale URL. + return with_query_params( + next_url, + { + ACCOUNT_ACTION_PARAM: action, + ACCOUNT_ACTION_STATUS_PARAM: AccountActionStatus.UNAVAILABLE, + }, + ) + + callback_url = build_account_action_callback_url( + self.request, next_url=next_url, action=action + ) + + qs = { + "client_id": settings.KEYCLOAK_CLIENT_ID, + "response_type": "code", + "redirect_uri": callback_url, + # `email` is requested explicitly so the code we exchange on the + # callback can read the updated address. It is one of Keycloak's + # default client scopes, but relying on that being true of every + # environment's client would fail silently and confusingly. + "scope": "openid email", + "kc_action": KEYCLOAK_ACTIONS[action], + } + + return "".join( + [ + settings.KEYCLOAK_BASE_URL.removesuffix("/"), + "/realms/", + settings.KEYCLOAK_REALM_NAME, + "/protocol/openid-connect/auth?", + urlencode(qs), + ] + ) + + +class AccountActionCompleteView(RedirectView): + """ + Land the user back on the frontend after a Keycloak account action. + + Keycloak appends `kc_action_status`, which tells us whether the action + succeeded so the frontend can show an alert. Getting a changed address into + Learn is Keycloak's job, pushed over SCIM. + """ + + def get_redirect_url(self, *args, **kwargs): # noqa: ARG002 + """Get the frontend URL, annotated with the outcome of the action""" + next_url = get_account_action_redirect_url(self.request) + raw_action = self.request.GET.get(ACCOUNT_ACTION_PARAM) + keycloak_status = self.request.GET.get(KEYCLOAK_ACTION_STATUS_PARAM) + + action = parse_account_action(raw_action) + status = KEYCLOAK_ACTION_STATUSES.get(keycloak_status) + + if action is None or status is None: + # Not something we can report on, so redirect without an alert + # rather than guess at an outcome. + log.warning( + "Account action callback with unusable params: action=%s, %s=%s " + "(params present: %s)", + raw_action, + KEYCLOAK_ACTION_STATUS_PARAM, + keycloak_status, + # Names only — these carry authorization codes and addresses. + ",".join(sorted(self.request.GET.keys())), + ) + return next_url + + return with_query_params( + next_url, + { + ACCOUNT_ACTION_PARAM: action, + ACCOUNT_ACTION_STATUS_PARAM: status, + }, + ) diff --git a/authentication/views_test.py b/authentication/views_test.py index 92401d9aef..0476a2a3e0 100644 --- a/authentication/views_test.py +++ b/authentication/views_test.py @@ -4,7 +4,7 @@ from base64 import b64encode from typing import NamedTuple from unittest.mock import MagicMock -from urllib.parse import urljoin +from urllib.parse import parse_qs, quote, urljoin, urlparse import pytest from django.test import RequestFactory @@ -339,3 +339,185 @@ def test_login_org_user_redirect( mock_send_welcome_email.assert_not_called() else: mock_send_welcome_email.assert_called_once_with(user.id) + + +@pytest.fixture +def mock_is_sso_user(mocker): + """Mock the SSO lookup, which otherwise talks to the Keycloak admin API""" + return mocker.patch("authentication.views.is_sso_user", return_value=False) + + +@pytest.fixture(autouse=True) +def account_action_settings(settings): + """ + Pin the settings the account action flow reads. + + KEYCLOAK_CLIENT_ID has no default and MITOL_API_BASE_URL defaults to empty, + so without this these tests assert against whatever the environment + happens to provide — passing locally and failing in CI. + """ + settings.KEYCLOAK_BASE_URL = "https://sso.example.edu" + settings.KEYCLOAK_REALM_NAME = "olapps" + settings.KEYCLOAK_CLIENT_ID = "ol-mitlearn-client" + settings.KEYCLOAK_CLIENT_SECRET = "a-secret" # noqa: S105 + settings.MITOL_API_BASE_URL = "https://api.example.edu" + return settings + + +@pytest.mark.parametrize( + ("action", "kc_action"), + [ + ("update-email", "UPDATE_EMAIL"), + ("update-password", "UPDATE_PASSWORD"), + ], +) +@pytest.mark.usefixtures("mock_is_sso_user") +def test_account_action_start(settings, client, action, kc_action): + """The start view should send the user to Keycloak with the right kc_action""" + next_url = urljoin(settings.APP_BASE_URL, "/dashboard/settings") + resp = client.get( + f"{reverse('account-action-start', kwargs={'action': action})}" + f"?next={quote(next_url)}" + ) + + assert resp.status_code == 302 + parsed = urlparse(resp.headers["Location"]) + + assert ( + f"{parsed.scheme}://{parsed.netloc}{parsed.path}" + == f"{settings.KEYCLOAK_BASE_URL.removesuffix('/')}/realms/" + f"{settings.KEYCLOAK_REALM_NAME}/protocol/openid-connect/auth" + ) + + expected_callback = ( + f"{settings.MITOL_API_BASE_URL.removesuffix('/')}" + f"{reverse('account-action-complete')}" + f"?{urlencode({'next': next_url, 'account_action': action})}" + ) + assert parse_qs(parsed.query) == { + "kc_action": [kc_action], + "scope": ["openid email"], + "response_type": ["code"], + "client_id": [settings.KEYCLOAK_CLIENT_ID], + "redirect_uri": [expected_callback], + } + + +@pytest.mark.usefixtures("mock_is_sso_user") +def test_account_action_start_unknown_action(settings, client): + """An unrecognized action should bounce back to settings with an error""" + resp = client.get( + reverse("account-action-start", kwargs={"action": "delete-account"}) + ) + + assert resp.status_code == 302 + expected_params = urlencode( + {"account_action": "delete-account", "account_action_status": "error"} + ) + settings_url = f"{settings.APP_BASE_URL.removesuffix('/')}/dashboard/settings" + assert resp.headers["Location"] == f"{settings_url}?{expected_params}" + + +def test_account_action_start_sso_user(settings, client, user, mock_is_sso_user): + """SSO users should never be sent to Keycloak to change their credentials""" + mock_is_sso_user.return_value = True + client.force_login(user) + + resp = client.get( + reverse("account-action-start", kwargs={"action": "update-email"}) + ) + + assert resp.status_code == 302 + expected_params = urlencode( + {"account_action": "update-email", "account_action_status": "unavailable"} + ) + settings_url = f"{settings.APP_BASE_URL.removesuffix('/')}/dashboard/settings" + assert resp.headers["Location"] == f"{settings_url}?{expected_params}" + + +@pytest.mark.parametrize("kc_action_status", ["success", "cancelled", "error"]) +def test_account_action_complete(settings, client, kc_action_status): + """The callback should hand the action's outcome back to the frontend""" + next_url = urljoin(settings.APP_BASE_URL, "/dashboard/settings") + callback_params = urlencode( + { + "next": next_url, + "account_action": "update-email", + "kc_action_status": kc_action_status, + } + ) + resp = client.get(f"{reverse('account-action-complete')}?{callback_params}") + + expected_params = urlencode( + { + "account_action": "update-email", + "account_action_status": kc_action_status, + } + ) + assert resp.status_code == 302 + assert resp.headers["Location"] == f"{next_url}?{expected_params}" + + +@pytest.mark.parametrize( + ("action", "kc_action_status"), + [ + # Keycloak didn't report an outcome + ("update-email", None), + ("update-email", "who-knows"), + # Not an action we started + ("delete-account", "success"), + (None, "success"), + ], +) +def test_account_action_complete_unusable_params( + settings, client, action, kc_action_status +): + """Without a usable outcome the user is returned without an alert""" + next_url = urljoin(settings.APP_BASE_URL, "/dashboard/settings") + params = {"next": next_url} + if action is not None: + params["account_action"] = action + if kc_action_status is not None: + params["kc_action_status"] = kc_action_status + + resp = client.get(f"{reverse('account-action-complete')}?{urlencode(params)}") + + assert resp.status_code == 302 + assert resp.headers["Location"] == next_url + + +@pytest.mark.usefixtures("mock_is_sso_user") +def test_account_action_disallowed_next(settings, client): + """A `next` pointing off-site falls back to the settings page""" + resp = client.get( + f"{reverse('account-action-complete')}?" + f"{urlencode({'next': 'https://malicious.com/phish'})}" + ) + + assert resp.status_code == 302 + assert ( + resp.headers["Location"] + == f"{settings.APP_BASE_URL.removesuffix('/')}/dashboard/settings" + ) + + +@pytest.mark.usefixtures("mock_is_sso_user") +def test_account_action_start_unconfigured_client(settings, client): + """ + An environment without KEYCLOAK_CLIENT_ID must not 500. + + The setting has no default, and urlencode raises TypeError on None, so + without a guard every click on Change Email would be a server error. + """ + settings.KEYCLOAK_CLIENT_ID = None + + resp = client.get( + reverse("account-action-start", kwargs={"action": "update-email"}) + ) + + assert resp.status_code == 302 + expected_params = urlencode( + {"account_action": "update-email", "account_action_status": "error"} + ) + settings_url = f"{settings.APP_BASE_URL.removesuffix('/')}/dashboard/settings" + assert resp.headers["Location"] == f"{settings_url}?{expected_params}" diff --git a/config/keycloak/realms/ol-local-realm.json b/config/keycloak/realms/ol-local-realm.json index 489881beb9..b6525b2f83 100644 --- a/config/keycloak/realms/ol-local-realm.json +++ b/config/keycloak/realms/ol-local-realm.json @@ -2428,6 +2428,15 @@ "priority": 30, "config": {} }, + { + "alias": "UPDATE_EMAIL", + "name": "Update Email", + "providerId": "UPDATE_EMAIL", + "enabled": true, + "defaultAction": false, + "priority": 1001, + "config": {} + }, { "alias": "UPDATE_PROFILE", "name": "Update Profile", diff --git a/docker-compose.services.yml b/docker-compose.services.yml index 7bfdd53a3f..ebebdf48cf 100644 --- a/docker-compose.services.yml +++ b/docker-compose.services.yml @@ -112,7 +112,7 @@ services: default: aliases: - ${KEYCLOAK_SVC_HOSTNAME:-kc.ol.local} - command: start --verbose --features scripts --import-realm --hostname=${KEYCLOAK_SVC_HOSTNAME:-kc.ol.local} --hostname-strict=false --hostname-debug=true --https-port=${KEYCLOAK_SSL_PORT} --https-certificate-file=/etc/x509/https/tls.crt --https-certificate-key-file=/etc/x509/https/tls.key --http-enabled=true --http-port=${KEYCLOAK_PORT} --config-keystore=/etc/keycloak-store --config-keystore-password=${KEYCLOAK_SVC_KEYSTORE_PASSWORD} --db=postgres --db-url-database=keycloak --db-url-host=db --db-schema=public --db-password=${POSTGRES_PASSWORD:-postgres} --db-username=postgres --db-url-port=${PGPORT:-5432} + command: start --verbose --features scripts,update-email --import-realm --hostname=${KEYCLOAK_SVC_HOSTNAME:-kc.ol.local} --hostname-strict=false --hostname-debug=true --https-port=${KEYCLOAK_SSL_PORT} --https-certificate-file=/etc/x509/https/tls.crt --https-certificate-key-file=/etc/x509/https/tls.key --http-enabled=true --http-port=${KEYCLOAK_PORT} --config-keystore=/etc/keycloak-store --config-keystore-password=${KEYCLOAK_SVC_KEYSTORE_PASSWORD} --db=postgres --db-url-database=keycloak --db-url-host=db --db-schema=public --db-password=${POSTGRES_PASSWORD:-postgres} --db-username=postgres --db-url-port=${PGPORT:-5432} volumes: - keycloak-store:/etc/keycloak-store - ./config/keycloak/tls:/etc/x509/https diff --git a/drf_lint_baseline.json b/drf_lint_baseline.json index 0a12f4f9ad..2657099b78 100644 --- a/drf_lint_baseline.json +++ b/drf_lint_baseline.json @@ -4,6 +4,6 @@ "channels/serializers.py:136:24:ORM002", "profiles/serializers.py:136:31:ORM002", "profiles/serializers.py:196:16:ORM002", - "profiles/serializers.py:419:15:ORM001", - "profiles/serializers.py:420:27:ORM001" + "profiles/serializers.py:446:15:ORM001", + "profiles/serializers.py:447:27:ORM001" ] diff --git a/frontends/api/src/generated/v0/api.ts b/frontends/api/src/generated/v0/api.ts index 62a7cd6fcf..38558e0dc1 100644 --- a/frontends/api/src/generated/v0/api.ts +++ b/frontends/api/src/generated/v0/api.ts @@ -1420,6 +1420,79 @@ export const CurrentEducationEnum = { export type CurrentEducationEnum = (typeof CurrentEducationEnum)[keyof typeof CurrentEducationEnum] +/** + * Serializer for the requesting user. Unlike UserSerializer this exposes the user\'s own email plus whether they can manage their credentials, both of which the settings page needs. It is read-only: users change their email through Keycloak, not through us. + * @export + * @interface CurrentUser + */ +export interface CurrentUser { + /** + * + * @type {number} + * @memberof CurrentUser + */ + id: number + /** + * + * @type {string} + * @memberof CurrentUser + */ + username: string + /** + * + * @type {string} + * @memberof CurrentUser + */ + global_id: string | null + /** + * + * @type {Profile} + * @memberof CurrentUser + */ + profile?: Profile + /** + * + * @type {string} + * @memberof CurrentUser + */ + email: string + /** + * + * @type {string} + * @memberof CurrentUser + */ + first_name: string + /** + * + * @type {string} + * @memberof CurrentUser + */ + last_name: string + /** + * + * @type {boolean} + * @memberof CurrentUser + */ + is_article_editor: boolean + /** + * + * @type {boolean} + * @memberof CurrentUser + */ + is_learning_path_editor: boolean + /** + * + * @type {boolean} + * @memberof CurrentUser + */ + is_authenticated: boolean + /** + * Whether the user signs in through an external identity provider, and so cannot change their email or password through us. + * @type {boolean} + * @memberof CurrentUser + */ + is_sso_user: boolean +} /** * * `online` - Online * `hybrid` - Hybrid * `in_person` - In-Person * `offline` - Offline * @export @@ -10459,7 +10532,7 @@ export const UsersApiFp = function (configuration?: Configuration) { async usersMeRetrieve( options?: RawAxiosRequestConfig, ): Promise< - (axios?: AxiosInstance, basePath?: string) => AxiosPromise + (axios?: AxiosInstance, basePath?: string) => AxiosPromise > { const localVarAxiosArgs = await localVarAxiosParamCreator.usersMeRetrieve(options) @@ -10619,7 +10692,9 @@ export const UsersApiFactory = function ( * @param {*} [options] Override http request option. * @throws {RequiredError} */ - usersMeRetrieve(options?: RawAxiosRequestConfig): AxiosPromise { + usersMeRetrieve( + options?: RawAxiosRequestConfig, + ): AxiosPromise { return localVarFp .usersMeRetrieve(options) .then((request) => request(axios, basePath)) diff --git a/frontends/api/src/hooks/user/index.ts b/frontends/api/src/hooks/user/index.ts index be8c99e3ee..0533595ebd 100644 --- a/frontends/api/src/hooks/user/index.ts +++ b/frontends/api/src/hooks/user/index.ts @@ -1,5 +1,5 @@ import { useQuery } from "@tanstack/react-query" -import type { User } from "../../generated/v0/api" +import type { CurrentUser, User } from "../../generated/v0/api" import { userQueries } from "./queries" enum Permission { @@ -31,4 +31,4 @@ export { useUserHasPermission, Permission, } -export type { User } +export type { CurrentUser, User } diff --git a/frontends/api/src/test-utils/factories/user.ts b/frontends/api/src/test-utils/factories/user.ts index 7e903b1f2b..e4a9ee88fb 100644 --- a/frontends/api/src/test-utils/factories/user.ts +++ b/frontends/api/src/test-utils/factories/user.ts @@ -1,6 +1,6 @@ import { faker } from "@faker-js/faker/locale/en" import type { PartialFactory } from "ol-test-utilities" -import type { Profile, User } from "../../generated/v0" +import type { CurrentUser, Profile } from "../../generated/v0" import { UniqueEnforcer } from "enforce-unique" const profile: PartialFactory = (overrides = {}): Profile => ({ @@ -24,12 +24,14 @@ const profile: PartialFactory = (overrides = {}): Profile => ({ const enforcerId = new UniqueEnforcer() -const user: PartialFactory = (overrides = {}): User => { +const user: PartialFactory = (overrides = {}): CurrentUser => { if (overrides?.is_authenticated === false) { return { is_authenticated: false, is_article_editor: false, is_learning_path_editor: false, + is_sso_user: false, + email: "", // @ts-expect-error API Response can include anonymous user id: null, username: "", @@ -37,12 +39,14 @@ const user: PartialFactory = (overrides = {}): User => { } } - const result: User = { + const result: CurrentUser = { id: enforcerId.enforce(faker.number.int), first_name: faker.person.firstName(), last_name: faker.person.lastName(), + email: faker.internet.email(), is_article_editor: false, is_learning_path_editor: false, + is_sso_user: false, username: faker.internet.username(), is_authenticated: true, global_id: faker.string.uuid(), diff --git a/frontends/main/src/app-pages/DashboardPage/AccountActionAlert.test.tsx b/frontends/main/src/app-pages/DashboardPage/AccountActionAlert.test.tsx new file mode 100644 index 0000000000..c21d840bc8 --- /dev/null +++ b/frontends/main/src/app-pages/DashboardPage/AccountActionAlert.test.tsx @@ -0,0 +1,75 @@ +import React from "react" +import AccountActionAlert from "./AccountActionAlert" +import { renderWithProviders, screen } from "@/test-utils" + +const renderAtUrl = (search: string) => + renderWithProviders(, { + url: `/dashboard/settings${search}`, + }) + +describe("AccountActionAlert", () => { + test.each([ + { + search: "?account_action=update-email&account_action_status=success", + message: + "Check your inbox for a confirmation link to finish updating your email address.", + }, + { + search: "?account_action=update-password&account_action_status=success", + message: "Your password has been updated.", + }, + { + search: "?account_action=update-email&account_action_status=error", + message: "We couldn't update your email address. Please try again.", + }, + { + search: + "?account_action=update-password&account_action_status=unavailable", + message: + "Your password is managed by your organization's single sign-on provider.", + }, + ])("Shows the outcome of $search", async ({ search, message }) => { + renderAtUrl(search) + expect(await screen.findByText(message)).toBeInTheDocument() + }) + + test.each([ + // Cancelling is a deliberate choice; nothing to report. + { search: "?account_action=update-email&account_action_status=cancelled" }, + // No account action in play at all. + { search: "" }, + ])("Shows nothing for $search", ({ search }) => { + const { view } = renderAtUrl(search) + expect(view.container).toBeEmptyDOMElement() + }) + + test.each([ + // Not an action we know about. + { search: "?account_action=delete-account&account_action_status=success" }, + { search: "?account_action=update-email&account_action_status=whoops" }, + // Half a payload. + { search: "?account_action=update-email" }, + ])("Warns and shows nothing for $search", ({ search }) => { + const warn = jest.spyOn(console, "warn").mockImplementation() + const { view } = renderAtUrl(search) + + expect(view.container).toBeEmptyDOMElement() + expect(warn).toHaveBeenCalled() + warn.mockRestore() + }) + + test("Strips the params so the alert doesn't reappear on refresh", async () => { + const { location } = renderAtUrl( + "?account_action=update-email&account_action_status=success&keep=me", + ) + await screen.findByText( + "Check your inbox for a confirmation link to finish updating your email address.", + ) + + expect(location.current.searchParams.get("account_action")).toBe(null) + expect(location.current.searchParams.get("account_action_status")).toBe( + null, + ) + expect(location.current.searchParams.get("keep")).toBe("me") + }) +}) diff --git a/frontends/main/src/app-pages/DashboardPage/AccountActionAlert.tsx b/frontends/main/src/app-pages/DashboardPage/AccountActionAlert.tsx new file mode 100644 index 0000000000..e3d03bc5e7 --- /dev/null +++ b/frontends/main/src/app-pages/DashboardPage/AccountActionAlert.tsx @@ -0,0 +1,107 @@ +"use client" + +import React from "react" +import { Alert } from "@mitodl/smoot-design" +import { + ACCOUNT_ACTION_PARAM, + ACCOUNT_ACTION_STATUS_PARAM, + AccountAction, + AccountActionStatus, +} from "@/common/urls" +import { + useConsumeSearchParamsOnce, + type ConsumedSearchParamsResult, +} from "@/common/useConsumeSearchParamsOnce" + +type AccountActionResult = { + action: AccountAction + status: AccountActionStatus +} + +const KEYS_TO_REMOVE = [ + ACCOUNT_ACTION_PARAM, + ACCOUNT_ACTION_STATUS_PARAM, +] as const + +const isAccountAction = (value: string | null): value is AccountAction => + Object.values(AccountAction).includes(value as AccountAction) + +const isAccountActionStatus = ( + value: string | null, +): value is AccountActionStatus => + Object.values(AccountActionStatus).includes(value as AccountActionStatus) + +const parseAccountActionResult = ( + searchParams: URLSearchParams, +): ConsumedSearchParamsResult | null => { + const action = searchParams.get(ACCOUNT_ACTION_PARAM) + const status = searchParams.get(ACCOUNT_ACTION_STATUS_PARAM) + + if (action === null && status === null) { + return null + } + + if (!isAccountAction(action) || !isAccountActionStatus(status)) { + console.warn("Unrecognized account action redirect params", action, status) + return { value: undefined, keysToRemove: KEYS_TO_REMOVE } + } + + return { value: { action, status }, keysToRemove: KEYS_TO_REMOVE } +} + +const SUCCESS_MESSAGES: Record = { + [AccountAction.UpdateEmail]: + "Check your inbox for a confirmation link to finish updating your email address.", + [AccountAction.UpdatePassword]: "Your password has been updated.", +} + +const ERROR_MESSAGES: Record = { + [AccountAction.UpdateEmail]: + "We couldn't update your email address. Please try again.", + [AccountAction.UpdatePassword]: + "We couldn't update your password. Please try again.", +} + +const UNAVAILABLE_MESSAGES: Record = { + [AccountAction.UpdateEmail]: + "Your email address is managed by your organization's single sign-on provider.", + [AccountAction.UpdatePassword]: + "Your password is managed by your organization's single sign-on provider.", +} + +/** + * Shows the outcome of a Keycloak account action once, on returning to the + * settings page. Cancelling the Keycloak form is a deliberate choice by the + * user, so it passes without an alert. + */ +const AccountActionAlert: React.FC = () => { + const result = useConsumeSearchParamsOnce(parseAccountActionResult) + + if (!result) return null + + switch (result.status) { + case AccountActionStatus.Success: + return ( + + {SUCCESS_MESSAGES[result.action]} + + ) + case AccountActionStatus.Unavailable: + return ( + + {UNAVAILABLE_MESSAGES[result.action]} + + ) + case AccountActionStatus.Error: + return ( + + {ERROR_MESSAGES[result.action]} + + ) + default: + return null + } +} + +export default AccountActionAlert +export { parseAccountActionResult } diff --git a/frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx b/frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx index 88ba57eebe..7ef414543f 100644 --- a/frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/SettingsContent.test.tsx @@ -3,21 +3,42 @@ import { SettingsContent } from "./SettingsContent" import { renderWithProviders, screen, within, user } from "@/test-utils" import { urls, setMockResponse, factories, makeRequest } from "api/test-utils" import type { LearningResourcesUserSubscriptionApiLearningResourcesUserSubscriptionCheckListRequest as CheckSubscriptionRequest } from "api" +import { useFeatureFlagEnabled } from "posthog-js/react" +import { FeatureFlags } from "@/common/feature_flags" +import { AccountAction, accountAction } from "@/common/urls" + +jest.mock("posthog-js/react", () => ({ + ...jest.requireActual("posthog-js/react"), + useFeatureFlagEnabled: jest.fn(), +})) +const mockedUseFeatureFlagEnabled = jest.mocked(useFeatureFlagEnabled) + +const enableAccountManagement = (enabled: boolean) => { + mockedUseFeatureFlagEnabled.mockImplementation((flag) => + flag === FeatureFlags.AccountManagement ? enabled : false, + ) +} type SetupApisOptions = { isAuthenticated?: boolean isSubscribed?: boolean subscriptionRequest?: CheckSubscriptionRequest emailOptin?: boolean | null + email?: string + isSsoUser?: boolean } const setupApis = ({ isAuthenticated = false, isSubscribed = false, subscriptionRequest = {}, emailOptin = null, + email = "learner@mit.edu", + isSsoUser = false, }: SetupApisOptions = {}) => { setMockResponse.get(urls.userMe.get(), { is_authenticated: isAuthenticated, + email, + is_sso_user: isSsoUser, }) setMockResponse.get(urls.profileMe.get(), { email_optin: emailOptin }) @@ -150,3 +171,64 @@ describe("SettingsPage email preferences", () => { ) }) }) + +describe("SettingsPage email & password", () => { + const CHANGE_EMAIL = /change email/i + const CHANGE_PASSWORD = /change password/i + + test("Links hand the user off to Django, which redirects to Keycloak", async () => { + enableAccountManagement(true) + setupApis({ isAuthenticated: true, email: "learner@mit.edu" }) + renderWithProviders() + + const next = { pathname: "/dashboard/settings", searchParams: null } + + const changeEmail = await screen.findByRole("link", { name: CHANGE_EMAIL }) + expect(changeEmail).toHaveAttribute( + "href", + accountAction(AccountAction.UpdateEmail, next), + ) + + const changePassword = await screen.findByRole("link", { + name: CHANGE_PASSWORD, + }) + expect(changePassword).toHaveAttribute( + "href", + accountAction(AccountAction.UpdatePassword, next), + ) + + expect(await screen.findByText("learner@mit.edu")).toBeInTheDocument() + }) + + test("Section is hidden when the feature flag is off", async () => { + enableAccountManagement(false) + setupApis({ isAuthenticated: true }) + renderWithProviders() + + await screen.findByRole("checkbox", { + name: /receive emails from mit learn/i, + }) + expect( + screen.queryByRole("link", { name: CHANGE_EMAIL }), + ).not.toBeInTheDocument() + expect( + screen.queryByRole("link", { name: CHANGE_PASSWORD }), + ).not.toBeInTheDocument() + }) + + test("Section is hidden for SSO users, whose credentials we don't own", async () => { + enableAccountManagement(true) + setupApis({ isAuthenticated: true, isSsoUser: true }) + renderWithProviders() + + await screen.findByRole("checkbox", { + name: /receive emails from mit learn/i, + }) + expect( + screen.queryByRole("link", { name: CHANGE_EMAIL }), + ).not.toBeInTheDocument() + expect( + screen.queryByRole("link", { name: CHANGE_PASSWORD }), + ).not.toBeInTheDocument() + }) +}) diff --git a/frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx b/frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx index 1fa5f096a1..7741d6093b 100644 --- a/frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx +++ b/frontends/main/src/app-pages/DashboardPage/SettingsContent.tsx @@ -10,7 +10,8 @@ import { DialogActions, Skeleton, } from "ol-components" -import { Button, Checkbox } from "@mitodl/smoot-design" +import { Button, ButtonLink, Checkbox } from "@mitodl/smoot-design" +import { useFeatureFlagEnabled } from "posthog-js/react" import { useUserMe } from "api/hooks/user" import { useProfileMeMutation, useProfileMeQuery } from "api/hooks/profile" import { @@ -18,6 +19,9 @@ import { useSearchSubscriptionList, } from "api/hooks/searchSubscription" import * as NiceModal from "@ebay/nice-modal-react" +import { FeatureFlags } from "@/common/feature_flags" +import { AccountAction, SETTINGS, accountAction } from "@/common/urls" +import AccountActionAlert from "./AccountActionAlert" import { TitleText } from "./HomeContent" const SOURCE_LABEL_DISPLAY = { topic: "Topic", @@ -119,6 +123,72 @@ const ListItemBody: React.FC = ({ ) } +const AccountActionRow = styled.div(({ theme }) => ({ + display: "flex", + alignItems: "center", + gap: "16px", + marginBottom: "16px", + [theme.breakpoints.down("sm")]: { + alignItems: "flex-start", + flexDirection: "column", + gap: "8px", + }, +})) + +const AccountActionLabel = styled.span(({ theme }) => ({ + ...theme.typography.body2, + color: theme.custom.colors.darkGray2, +})) + +const AccountActionValue = styled.span(({ theme }) => ({ + ...theme.typography.body2, + color: theme.custom.colors.silverGrayDark, +})) + +type AccountManagementProps = { + email: string +} + +/** + * Email and password management. Both actions hand the user off to Keycloak, + * which owns the credentials, and return them here with a confirmation. + * + * Hidden from SSO users, whose credentials belong to their institution. + */ +const AccountManagement: React.FC = ({ email }) => { + const next = { pathname: SETTINGS, searchParams: null } + + return ( + <> + Email & Password + + + Email: {email} + + + Change Email + + + + + Password + + + Change Password + + + + ) +} + type UnfollowDialogProps = { subscriptionIds?: number[] subscriptionName?: string @@ -178,6 +248,10 @@ const SettingsContent: React.FC = () => { const { data: profile } = useProfileMeQuery() const { mutateAsync: updateProfile } = useProfileMeMutation() + const accountManagementEnabled = useFeatureFlagEnabled( + FeatureFlags.AccountManagement, + ) + const subscriptionList = useSearchSubscriptionList({ enabled: !!user?.is_authenticated, }) @@ -186,9 +260,14 @@ const SettingsContent: React.FC = () => { return } + const showAccountManagement = + accountManagementEnabled && user.is_authenticated && !user.is_sso_user + return (
Settings + + {showAccountManagement ? : null} Email Preferences { + expect( + accountAction(action, { + pathname: "/dashboard/settings", + searchParams: null, + }), + ).toBe(expected) + }, +) + test.each([ { readableId: "course-v1:MITxT+10.50x", diff --git a/frontends/main/src/common/urls.ts b/frontends/main/src/common/urls.ts index 2b61c9ba61..20e44b6644 100644 --- a/frontends/main/src/common/urls.ts +++ b/frontends/main/src/common/urls.ts @@ -313,6 +313,42 @@ export const auth = (opts: LoginUrlOpts) => { return url.toString() } +/** + * Keycloak account actions the user can start from the settings page. + * + * Must stay in sync with `AccountAction` in authentication/constants.py. + */ +export enum AccountAction { + UpdateEmail = "update-email", + UpdatePassword = "update-password", +} + +/** + * Outcome of an account action, reported back on the URL we're returned to. + * + * Must stay in sync with `AccountActionStatus` in authentication/constants.py. + */ +export enum AccountActionStatus { + Success = "success", + Cancelled = "cancelled", + Error = "error", + Unavailable = "unavailable", +} + +export const ACCOUNT_ACTION_PARAM = "account_action" +export const ACCOUNT_ACTION_STATUS_PARAM = "account_action_status" + +/** + * Returns the URL that hands the user off to Keycloak to change their email or + * password. Django owns the handoff — it holds the OIDC client config and + * validates the user is allowed to perform the action. + */ +export const accountAction = (action: AccountAction, next: UrlDescriptor) => { + const url = new URL(`${MITOL_API_BASE_URL}/account/action/start/${action}/`) + url.searchParams.set("next", stringifyUrlDescriptor(next)) + return url.toString() +} + export const ECOMMERCE_CART = "/cart/" as const export const B2B_ATTACH_VIEW = "/enrollmentcode/[code]" diff --git a/frontends/main/src/test-utils/index.tsx b/frontends/main/src/test-utils/index.tsx index e13f853304..e9e3ad6908 100644 --- a/frontends/main/src/test-utils/index.tsx +++ b/frontends/main/src/test-utils/index.tsx @@ -8,7 +8,7 @@ import { ComplianceGateProvider } from "@/common/mitxonline/useComplianceGate" import { makeBrowserQueryClient } from "@/app/getQueryClient" import { render } from "@testing-library/react" import { factories, setMockResponse } from "api/test-utils" -import type { User } from "api/hooks/user" +import type { CurrentUser, User } from "api/hooks/user" import { userQueries } from "api/hooks/user" import { mockRouter, @@ -36,13 +36,13 @@ setupRoutes() interface TestAppOptions { url: string - user: Partial + user: Partial } const defaultTestAppOptions = { url: "/", } -const defaultUser: User = factories.user.user() +const defaultUser: CurrentUser = factories.user.user() const TestProviders: React.FC<{ children: React.ReactNode @@ -319,4 +319,4 @@ export { } from "@testing-library/react" export { default as user } from "@testing-library/user-event" -export type { TestAppOptions, User } +export type { TestAppOptions, CurrentUser, User } diff --git a/main/settings.py b/main/settings.py index 334b1015c0..72daa97d37 100644 --- a/main/settings.py +++ b/main/settings.py @@ -705,6 +705,16 @@ def get_all_config_keys(): name="KEYCLOAK_REALM_NAME", default="olapps", ) +# The OIDC client used to start Keycloak "application initiated actions" +# (update email / update password) and to exchange the resulting authorization +# code. Deliberately has no default: the account action callback URL has to be a +# registered redirect URI on this client, so guessing a client here fails at +# Keycloak with an opaque error. Deployed environments must set it (mitxonline +# does the same with `ol-mitxonline-client`). +KEYCLOAK_CLIENT_ID = get_string( + name="KEYCLOAK_CLIENT_ID", + default=None, +) MICROMASTERS_CMS_API_URL = get_string("MICROMASTERS_CMS_API_URL", None) diff --git a/openapi/specs/v0.yaml b/openapi/specs/v0.yaml index 384a27716b..bb5f536a63 100644 --- a/openapi/specs/v0.yaml +++ b/openapi/specs/v0.yaml @@ -691,7 +691,7 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/User' + $ref: '#/components/schemas/CurrentUser' description: '' /api/v0/vector_content_files_search/: get: @@ -2374,6 +2374,63 @@ components: - Junior secondary/junior high/middle school - No formal education - Other education + CurrentUser: + type: object + description: |- + Serializer for the requesting user. + + Unlike UserSerializer this exposes the user's own email plus whether they + can manage their credentials, both of which the settings page needs. It is + read-only: users change their email through Keycloak, not through us. + properties: + id: + type: integer + readOnly: true + username: + type: string + readOnly: true + global_id: + type: string + readOnly: true + nullable: true + profile: + $ref: '#/components/schemas/Profile' + email: + type: string + readOnly: true + default: '' + first_name: + type: string + readOnly: true + last_name: + type: string + readOnly: true + is_article_editor: + type: boolean + readOnly: true + is_learning_path_editor: + type: boolean + readOnly: true + is_authenticated: + type: boolean + readOnly: true + is_sso_user: + type: boolean + description: |- + Whether the user signs in through an external identity provider, and so + cannot change their email or password through us. + readOnly: true + required: + - email + - first_name + - global_id + - id + - is_article_editor + - is_authenticated + - is_learning_path_editor + - is_sso_user + - last_name + - username DeliveryEnum: enum: - online diff --git a/profiles/serializers.py b/profiles/serializers.py index 4a763c318a..5ef0186be2 100644 --- a/profiles/serializers.py +++ b/profiles/serializers.py @@ -395,6 +395,33 @@ class Meta: read_only_fields = ("id", "username", "global_id", "is_authenticated") +class CurrentUserSerializer(UserSerializer): + """ + Serializer for the requesting user. + + Unlike UserSerializer this exposes the user's own email plus whether they + can manage their credentials, both of which the settings page needs. It is + read-only: users change their email through Keycloak, not through us. + """ + + # AnonymousUser has no email attribute, and a read-only field whose + # attribute is missing is dropped from the output entirely (the same reason + # first_name/last_name don't appear for anonymous users). The default keeps + # the key present and blank instead. + email = serializers.CharField(read_only=True, default="") + is_sso_user = serializers.SerializerMethodField() + + def get_is_sso_user(self, instance) -> bool: + """ + Whether the user signs in through an external identity provider, and so + cannot change their email or password through us. + """ + return auth_api.is_sso_user(instance) + + class Meta(UserSerializer.Meta): + fields = (*UserSerializer.Meta.fields, "is_sso_user") + + class ProgramCertificateSerializer(serializers.ModelSerializer): """ Serializer for Program Certificates diff --git a/profiles/views.py b/profiles/views.py index 177e3137b7..ab443cf814 100644 --- a/profiles/views.py +++ b/profiles/views.py @@ -22,6 +22,7 @@ from profiles.models import Profile, ProgramCertificate, ProgramLetter, UserWebsite from profiles.permissions import HasEditPermission, HasSiteEditPermission from profiles.serializers import ( + CurrentUserSerializer, ProfileSerializer, ProgramCertificateSerializer, ProgramLetterSerializer, @@ -49,7 +50,7 @@ class UserViewSet(viewsets.ModelViewSet): class CurrentUserRetrieveViewSet(mixins.RetrieveModelMixin, viewsets.GenericViewSet): """User retrieve and update viewsets for the current user""" - serializer_class = UserSerializer + serializer_class = CurrentUserSerializer permission_classes = ( AnonymousAccessReadonlyPermission, HasEditPermission, diff --git a/profiles/views_test.py b/profiles/views_test.py index 864e2716d7..0852ab9406 100644 --- a/profiles/views_test.py +++ b/profiles/views_test.py @@ -394,21 +394,25 @@ def test_get_user_by_me(mocker, client, user, is_anonymous): "id": None, "username": "", "global_id": None, + "email": "", "is_learning_path_editor": False, "is_article_editor": False, "is_authenticated": False, + "is_sso_user": False, } else: assert resp.json() == { "id": user.id, "username": user.username, "global_id": user.global_id, + "email": user.email, "first_name": user.first_name, "last_name": user.last_name, "is_learning_path_editor": False, "is_article_editor": False, "profile": ProfileSerializer(user.profile).data, "is_authenticated": True, + "is_sso_user": False, } diff --git a/users/admin.py b/users/admin.py index 467788fe77..b59b3be346 100644 --- a/users/admin.py +++ b/users/admin.py @@ -19,5 +19,17 @@ class UserAdmin(ContribUserAdmin): fieldsets = ( *ContribUserAdmin.fieldsets, + ( + "Identity provider", + { + "fields": ("is_sso_user",), + "description": ( + "Blank until it is first determined from Keycloak. Clear it " + "to have it re-read, or set it explicitly to override — " + "unsetting it lets a user manage their own email and " + "password here." + ), + }, + ), ("SCIM", {"fields": ("scim_id", "scim_username", "scim_external_id")}), ) diff --git a/users/migrations/0012_user_is_sso_user.py b/users/migrations/0012_user_is_sso_user.py new file mode 100644 index 0000000000..bad22649b0 --- /dev/null +++ b/users/migrations/0012_user_is_sso_user.py @@ -0,0 +1,21 @@ +# Generated by Django 4.2.30 on 2026-08-11 08:02 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("users", "0011_user_unsubscribe_uuid"), + ] + + operations = [ + migrations.AddField( + model_name="user", + name="is_sso_user", + field=models.BooleanField( + default=None, + help_text="Authenticates via an external identity provider, so cannot change their own email or password. Blank until determined from Keycloak.", # noqa: E501 + null=True, + ), + ), + ] diff --git a/users/models.py b/users/models.py index 8df9588d4f..ff7556a3a6 100644 --- a/users/models.py +++ b/users/models.py @@ -3,7 +3,7 @@ import uuid from django.contrib.auth.models import AbstractUser -from django.db.models import CharField, UUIDField +from django.db.models import BooleanField, CharField, UUIDField from django_scim.models import AbstractSCIMUserMixin from main.models import TimestampedModel @@ -20,6 +20,21 @@ class User(AbstractUser, AbstractSCIMUserMixin, TimestampedModel): default=None, ) + # Null until first needed, then filled in from Keycloak's federated identity + # links (see authentication.api.is_sso_user). Stored rather than derived on + # each read because it effectively never changes on its own, and because + # making it editable is useful: clearing the flag grants someone local + # credentials, so they can keep an account after leaving the organization + # that provided their identity. + is_sso_user = BooleanField( + null=True, + default=None, + help_text=( + "Authenticates via an external identity provider, so cannot change " + "their own email or password. Blank until determined from Keycloak." + ), + ) + def get_or_generate_unsubscribe_uuid(self) -> uuid.UUID: """Get the existing unsubscribe_uuid or generate a new one""" if self.unsubscribe_uuid is None: From 2867a68fdcf0bde34ce79c9aba94377b56e5e086 Mon Sep 17 00:00:00 2001 From: Zaman Afzal Date: Wed, 19 Aug 2026 18:43:30 +0500 Subject: [PATCH 02/10] Update Terms of Service (MicroMasters bundle, AI Tutor, date) (#3767) * Update Terms of Service (MicroMasters bundle, AI Tutor, date) --- .../src/app-pages/TermsPage/TermsPage.tsx | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/frontends/main/src/app-pages/TermsPage/TermsPage.tsx b/frontends/main/src/app-pages/TermsPage/TermsPage.tsx index 01578baf04..516d3f1bf9 100644 --- a/frontends/main/src/app-pages/TermsPage/TermsPage.tsx +++ b/frontends/main/src/app-pages/TermsPage/TermsPage.tsx @@ -343,6 +343,20 @@ const TermsPage: React.FC = () => { not responsible for unauthorized access or misuse of personal information you voluntarily provide in violation of this policy. + + AI Tutor Chatbot: + + + MIT Learn provides an AI Tutor Chatbot as a learning tool in + conjunction with certain Offerings. It is intended to help you + review course concepts, ask questions, and receive study guidance. + As with all AI technology, the use is not error-free and you remain + responsible for verifying all information against official course + materials, assignments, and instructor guidance. Use of the AI Tutor + Chatbot does not guarantee improved grades, assignment credit or + course completion. You remain fully responsible for all Course work + and for compliance with MIT Learn's Honor Code. +
Certificates of Completion @@ -509,6 +523,18 @@ const TermsPage: React.FC = () => { applied after the purchase request has been submitted. Offers cannot be combined for additional discounts. + + Bundled MicroMasters Purchases. + + + All MicroMasters bundle purchases grant access to register + ("Entitlement") for the certificate tracks of included courses for + twenty-four (24) months from the date of purchase ("Expiration + Date"). Upon the Expiration Date, your ability to register for any + included course certificate track using the bundle will + automatically terminate, and any unused Entitlements will expire and + cannot be redeemed thereafter. + Transfers / Substitutions / Deferments. @@ -784,7 +810,7 @@ const TermsPage: React.FC = () => { inconvenience of forum). - These terms of service were last updated on June 1, 2026. + These terms of service were last updated on August 17, 2026. From 28960ae2a8097266a06fa174787087a25cfb3cdc Mon Sep 17 00:00:00 2001 From: Tobias Macey Date: Wed, 19 Aug 2026 12:09:06 -0400 Subject: [PATCH 03/10] fix(otel): continue the edge trace by extracting W3C traceparent (#3787) * fix(otel): continue the edge trace by extracting W3C traceparent Every learn-nextjs trace roots at learn-nextjs, never at traefik, even though Traefik has been emitting edge spans since ol-infrastructure#5480 shipped. `{traefik} && {learn-nextjs}` matches zero traces. Sentry's propagator is write-only for W3C. SentryPropagator.extract() reads sentry-trace and baggage and never traceparent, while inject() does write one and fields() advertises it. Traefik sends only traceparent, so the incoming context is discarded and the SSR render starts a new trace. Keycloak, behind the same Traefik, joins fine because Quarkus uses the standard W3C propagator -- which is how we know the gateway is not at fault. Verified against @sentry/opentelemetry 10.70.0, the current release, so this is not something a version bump fixes. Register a CompositePropagator before Sentry.init(). It has to be before: setGlobalPropagator refuses a second registration and returns false, so doing it afterwards silently does nothing. Sentry goes last in the composite so it still wins when sentry-trace is present, leaving browser-originated traces exactly as they are. Its extract() returns the context untouched when that header is absent, so W3C's extraction survives for edge traffic. Both directions are pinned by tests, along with a test that fails the day Sentry learns to read traceparent and this composite can be removed. @sentry/opentelemetry and @opentelemetry/core become direct dependencies because SentryPropagator is not exported from @sentry/nextjs or @sentry/node. The Sentry one needs to stay in lockstep with @sentry/nextjs. * fix(otel): dedupe the Sentry packages, and let the test pin the real ordering The caret on @sentry/opentelemetry resolved to 10.70.0 while @sentry/nextjs stayed at 10.50.0, so the lockfile carried two copies with two @sentry/core versions. The composite imported SentryPropagator from a different Sentry runtime than the SDK initialised -- precisely the split this change warned about. Pin both to 10.50.0 exactly; the lockfile now has one entry. Move the composition into otel-setup.ts and use it from both startup and the test. The test previously rebuilt the composite itself, so reordering or dropping a propagator in production would have left every assertion passing. The "if this ever starts passing" comment was backwards: the assertion passes today, and would start *failing* if Sentry learned to read traceparent. --- frontends/main/package.json | 4 +- frontends/main/src/instrumentation-node.ts | 20 ++++++ frontends/main/src/otel-propagator.test.ts | 71 ++++++++++++++++++++++ frontends/main/src/otel-setup.ts | 39 ++++++++++++ yarn.lock | 17 +++++- 5 files changed, 148 insertions(+), 3 deletions(-) create mode 100644 frontends/main/src/otel-propagator.test.ts create mode 100644 frontends/main/src/otel-setup.ts diff --git a/frontends/main/package.json b/frontends/main/package.json index d4fe006d6b..d378f865df 100644 --- a/frontends/main/package.json +++ b/frontends/main/package.json @@ -25,6 +25,7 @@ "@mui/material-nextjs": "^6.4.3", "@mui/x-charts": "^8.29.2", "@opentelemetry/api": "^1.9.1", + "@opentelemetry/core": "^2.0.1", "@opentelemetry/exporter-trace-otlp-http": "^0.214.0", "@opentelemetry/resources": "^2.6.1", "@opentelemetry/sdk-trace-base": "^2.6.1", @@ -32,7 +33,8 @@ "@radix-ui/react-popover": "^1.1.15", "@react-pdf/renderer": "^4.3.0", "@remixicon/react": "^4.2.0", - "@sentry/nextjs": "^10.50.0", + "@sentry/nextjs": "10.50.0", + "@sentry/opentelemetry": "10.50.0", "@tanstack/react-query": "^5.66.0", "@tiptap/core": "^3.13.0", "@tiptap/extension-document": "^3.13.0", diff --git a/frontends/main/src/instrumentation-node.ts b/frontends/main/src/instrumentation-node.ts index 94ad99bd1f..6aad79b0e6 100644 --- a/frontends/main/src/instrumentation-node.ts +++ b/frontends/main/src/instrumentation-node.ts @@ -5,6 +5,7 @@ import * as Sentry from "@sentry/nextjs" import type { Context, Span } from "@opentelemetry/api" +import { propagation } from "@opentelemetry/api" import { BatchSpanProcessor, ConsoleSpanExporter, @@ -21,6 +22,7 @@ import { detectResourceOverrides, hasOtlpEndpointConfig, } from "./otel-utils" +import { buildPropagator } from "./otel-setup" import { parseSampleRate } from "./sentry-utils" import { env } from "@/env" import { validateEnv } from "../validateEnv" @@ -215,6 +217,24 @@ const sentrySampleRate = parseSampleRate( 1, ) +// MUST run before Sentry.init(). Sentry's SDK installs SentryPropagator as the +// global propagator (initOtel.ts), and its extract() reads only sentry-trace +// and baggage -- never traceparent -- while its inject() *does* write one. That +// asymmetry means every W3C-only caller is dropped: Traefik fronts +// next.learn.mit.edu and sends traceparent, so the SSR render started a fresh +// trace instead of continuing the edge's. Keycloak, behind the same Traefik, +// joins fine because Quarkus uses the standard W3C propagator. +// +// setGlobalPropagator refuses a second registration and returns false, so this +// cannot be applied afterwards -- registering here means Sentry's own call is +// the one that no-ops, and this composite stays. +// +// Order matters. Sentry goes last so it still wins when sentry-trace is +// present, which keeps today's browser-originated traces intact; its extract() +// returns the context untouched when that header is absent, so W3C's +// extraction survives for edge traffic. See otel-setup.ts. +propagation.setGlobalPropagator(buildPropagator()) + Sentry.init({ dsn: env("NEXT_PUBLIC_SENTRY_DSN"), release: env("NEXT_PUBLIC_VERSION"), diff --git a/frontends/main/src/otel-propagator.test.ts b/frontends/main/src/otel-propagator.test.ts new file mode 100644 index 0000000000..6d47503b4f --- /dev/null +++ b/frontends/main/src/otel-propagator.test.ts @@ -0,0 +1,71 @@ +/** + * Pins the propagator composition that lets the SSR runtime continue a trace + * started at the edge. + * + * Sentry's SentryPropagator.extract() reads only sentry-trace and baggage, so + * on its own every W3C-only caller is dropped. Traefik fronts + * next.learn.mit.edu and sends traceparent, which is exactly that case. + */ + +import { propagation, trace, ROOT_CONTEXT } from "@opentelemetry/api" +import { SentryPropagator } from "@sentry/opentelemetry" +import { buildPropagator } from "./otel-setup" + +const TRACE_ID = "0af7651916cd43dd8448eb211c80319c" +const SPAN_ID = "b7ad6b7169203331" + +// The production composition, not a copy of it -- so reordering or dropping a +// propagator in otel-setup.ts fails these tests rather than sailing past them. +const composite = buildPropagator + +const extractedSpanContext = ( + propagator: { extract: typeof propagation.extract }, + carrier: Record, +) => { + const ctx = propagator.extract(ROOT_CONTEXT, carrier, { + get: (c, k) => c[k], + keys: (c) => Object.keys(c), + }) + return trace.getSpanContext(ctx) +} + +describe("OTel propagator composition", () => { + it("drops a W3C traceparent when only Sentry's propagator is used", () => { + // The bug this composition exists to fix. If this ever starts failing, + // Sentry has learned to read traceparent and the composite can go. + const spanContext = extractedSpanContext(new SentryPropagator(), { + traceparent: `00-${TRACE_ID}-${SPAN_ID}-01`, + }) + + expect(spanContext).toBeUndefined() + }) + + it("continues a trace from a W3C traceparent", () => { + const spanContext = extractedSpanContext(composite(), { + traceparent: `00-${TRACE_ID}-${SPAN_ID}-01`, + }) + + expect(spanContext?.traceId).toBe(TRACE_ID) + expect(spanContext?.spanId).toBe(SPAN_ID) + }) + + it("still continues a trace from sentry-trace", () => { + const spanContext = extractedSpanContext(composite(), { + "sentry-trace": `${TRACE_ID}-${SPAN_ID}-1`, + }) + + expect(spanContext?.traceId).toBe(TRACE_ID) + }) + + it("lets sentry-trace win when both headers are present", () => { + // Sentry is last in the composite deliberately, so browser-originated + // traces keep the behaviour they have today. + const sentryTraceId = "11111111111111111111111111111111" + const spanContext = extractedSpanContext(composite(), { + traceparent: `00-${TRACE_ID}-${SPAN_ID}-01`, + "sentry-trace": `${sentryTraceId}-${SPAN_ID}-1`, + }) + + expect(spanContext?.traceId).toBe(sentryTraceId) + }) +}) diff --git a/frontends/main/src/otel-setup.ts b/frontends/main/src/otel-setup.ts new file mode 100644 index 0000000000..33e4430515 --- /dev/null +++ b/frontends/main/src/otel-setup.ts @@ -0,0 +1,39 @@ +/** + * OpenTelemetry wiring shared between server startup and its tests. + * + * Exists so the composition below has exactly one definition. A test that + * rebuilt the composite itself would keep passing after someone reordered or + * dropped a propagator in production, which is the failure it is meant to + * catch. + */ + +import { + CompositePropagator, + W3CBaggagePropagator, + W3CTraceContextPropagator, +} from "@opentelemetry/core" +import { SentryPropagator } from "@sentry/opentelemetry" + +/** + * Propagator that reads W3C *and* Sentry headers. + * + * Sentry's SentryPropagator.extract() reads only sentry-trace and baggage, + * never traceparent, while its inject() does write one. That asymmetry drops + * every W3C-only caller -- Traefik fronts next.learn.mit.edu and sends + * traceparent, so the SSR render started a fresh trace instead of continuing + * the edge's. + * + * Order matters. Sentry goes last so it still wins when sentry-trace is + * present, keeping browser-originated traces as they are; its extract() + * returns the context untouched when that header is absent, so W3C's + * extraction survives for edge traffic. + */ +export function buildPropagator(): CompositePropagator { + return new CompositePropagator({ + propagators: [ + new W3CTraceContextPropagator(), + new W3CBaggagePropagator(), + new SentryPropagator(), + ], + }) +} diff --git a/yarn.lock b/yarn.lock index 6a60e76cf0..e27e31089a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4525,6 +4525,17 @@ __metadata: languageName: node linkType: hard +"@opentelemetry/core@npm:^2.0.1": + version: 2.10.0 + resolution: "@opentelemetry/core@npm:2.10.0" + dependencies: + "@opentelemetry/semantic-conventions": "npm:^1.29.0" + peerDependencies: + "@opentelemetry/api": ">=1.0.0 <1.10.0" + checksum: 10/b95d912461d878a23e092317a98eb593b2316e0e63bb0618f351aba8fcb92253553ba8d4ceac3f575b4152325a85da2cf39f4e68123088ef47429f7328c971e5 + languageName: node + linkType: hard + "@opentelemetry/exporter-trace-otlp-http@npm:^0.214.0": version: 0.214.0 resolution: "@opentelemetry/exporter-trace-otlp-http@npm:0.214.0" @@ -6288,7 +6299,7 @@ __metadata: languageName: node linkType: hard -"@sentry/nextjs@npm:^10.50.0": +"@sentry/nextjs@npm:10.50.0": version: 10.50.0 resolution: "@sentry/nextjs@npm:10.50.0" dependencies: @@ -16905,6 +16916,7 @@ __metadata: "@mui/material-nextjs": "npm:^6.4.3" "@mui/x-charts": "npm:^8.29.2" "@opentelemetry/api": "npm:^1.9.1" + "@opentelemetry/core": "npm:^2.0.1" "@opentelemetry/exporter-trace-otlp-http": "npm:^0.214.0" "@opentelemetry/resources": "npm:^2.6.1" "@opentelemetry/sdk-trace-base": "npm:^2.6.1" @@ -16912,7 +16924,8 @@ __metadata: "@radix-ui/react-popover": "npm:^1.1.15" "@react-pdf/renderer": "npm:^4.3.0" "@remixicon/react": "npm:^4.2.0" - "@sentry/nextjs": "npm:^10.50.0" + "@sentry/nextjs": "npm:10.50.0" + "@sentry/opentelemetry": "npm:10.50.0" "@tanstack/react-query": "npm:^5.66.0" "@testing-library/jest-dom": "npm:^6.4.8" "@testing-library/react": "npm:^16.3.0" From 2a95244ca3ce2ec81279ea89569aaca584969c56 Mon Sep 17 00:00:00 2001 From: Anastasia Beglova Date: Wed, 19 Aug 2026 13:20:38 -0400 Subject: [PATCH 04/10] make canvas etl resistant to pod culling (#3779) --- learning_resources/etl/canvas.py | 33 +++- learning_resources/etl/canvas_test.py | 106 ++++++++++++ learning_resources/etl/canvas_utils.py | 14 ++ .../commands/backpopulate_canvas_courses.py | 13 +- learning_resources/tasks.py | 85 +++++++-- learning_resources/tasks_test.py | 163 ++++++++++++++---- 6 files changed, 352 insertions(+), 62 deletions(-) diff --git a/learning_resources/etl/canvas.py b/learning_resources/etl/canvas.py index d470dd2d2c..5bc667371b 100644 --- a/learning_resources/etl/canvas.py +++ b/learning_resources/etl/canvas.py @@ -16,6 +16,7 @@ ) from learning_resources.etl.canvas_utils import ( canvas_course_checksum, + canvas_course_folder, canvas_course_url, canvas_url_config, get_published_items, @@ -31,7 +32,10 @@ LearningResourcePlatform, LearningResourceRun, ) -from learning_resources.utils import bulk_resources_unpublished_actions +from learning_resources.utils import ( + bulk_resources_unpublished_actions, + resource_unpublished_actions, +) from learning_resources_search.constants import ( CONTENT_FILE_TYPE, ) @@ -46,7 +50,7 @@ def sync_canvas_archive(bucket, key: str, overwrite): """ from learning_resources.etl.loaders import load_content_files, load_problem_files - course_folder = key.lstrip(settings.CANVAS_COURSE_BUCKET_PREFIX).split("/")[0] + course_folder = canvas_course_folder(key) url_config_file = f"{key.split('.imscc', maxsplit=1)[0]}.metadata.json" with TemporaryDirectory() as export_tempdir: course_archive_path = Path(export_tempdir, key.rsplit("/", maxsplit=1)[-1]) @@ -130,7 +134,8 @@ def run_for_canvas_archive(course_archive_path, course_folder, checksum, overwri except (ValueError, TypeError): log.warning("Invalid end_at date format: %s", end_at) - readable_id = f"{course_folder}-{course_info.get('course_code')}" + course_code = course_info.get("course_code") + readable_id = f"{course_folder}-{course_code}" # create placeholder learning resource resource, _ = LearningResource.objects.update_or_create( readable_id=readable_id, @@ -147,6 +152,28 @@ def run_for_canvas_archive(course_archive_path, course_folder, checksum, overwri "resource_category": LearningResourceType.course.value, }, ) + + if course_code: + orphaned_resources = LearningResource.objects.filter( + etl_source=ETLSource.canvas.name, + readable_id__istartswith=f"{course_folder}-", + ).exclude(id=resource.id) + # the update doubles as the guard so the common (no orphans) case is one query + if orphaned_resources.update(test_mode=False, published=False): + orphans = list(orphaned_resources) + log.info( + "Deleting %d resources orphaned by a course code change in folder %s", + len(orphans), + course_folder, + ) + for orphan in orphans: + resource_unpublished_actions(orphan) + else: + log.warning( + "Canvas archive in folder %s has no course code; skipping orphan cleanup", + course_folder, + ) + if resource.runs.count() == 0: LearningResourceRun.objects.create( run_id=f"{readable_id}+canvas", diff --git a/learning_resources/etl/canvas_test.py b/learning_resources/etl/canvas_test.py index 0c42fb6e01..373f7b2aee 100644 --- a/learning_resources/etl/canvas_test.py +++ b/learning_resources/etl/canvas_test.py @@ -20,6 +20,7 @@ from learning_resources.etl.canvas_utils import ( _compact_element, canvas_course_checksum, + canvas_course_folder, get_published_items, is_file_published, parse_canvas_files, @@ -167,6 +168,111 @@ def test_parse_canvas_settings_handles_namespaces(tmp_path): assert attrs["course_code"] == "NS-101" +@pytest.mark.parametrize( + ("prefix", "key", "expected"), + [ + ("canvas/course_content", "canvas/course_content/12345/a.imscc", "12345"), + ("canvas/course_content/", "canvas/course_content/12345/a.imscc", "12345"), + ("canvas/", "canvas/1/a.imscc", "1"), + # a folder whose name is spelled with characters from the prefix must + # survive - str.lstrip would eat it, str.removeprefix does not + ("canvas/course_content", "canvas/course_content/course/a.imscc", "course"), + ], +) +def test_canvas_course_folder(settings, prefix, key, expected): + """canvas_course_folder should strip the bucket prefix, not a character set""" + settings.CANVAS_COURSE_BUCKET_PREFIX = prefix + assert canvas_course_folder(key) == expected + + +@pytest.mark.django_db +def test_run_for_canvas_archive_unpublishes_course_code_orphans(tmp_path, mocker): + """ + A course whose code changed leaves a resource behind under its old readable + id. The S3 listing can't see that, so the course's own sync must clean it up. + """ + mocker.patch( + "learning_resources.etl.canvas.parse_canvas_settings", + return_value={"title": "Test Course", "course_code": "NEW101"}, + ) + mocker.patch( + "learning_resources.etl.canvas_utils.parse_context_xml", + return_value={"course_id": "123", "canvas_domain": "mit.edu"}, + ) + mock_unpublished_actions = mocker.patch( + "learning_resources.etl.canvas.resource_unpublished_actions" + ) + orphan = LearningResourceFactory.create( + readable_id="123-OLD101", + etl_source=ETLSource.canvas.name, + resource_type=LearningResourceType.course.name, + published=True, + test_mode=True, + ) + # another folder's course, and another source sharing the prefix, both stay + other_course = LearningResourceFactory.create( + readable_id="1234-OTHER", + etl_source=ETLSource.canvas.name, + resource_type=LearningResourceType.course.name, + ) + other_source = LearningResourceFactory.create( + readable_id="123-EDX", + etl_source=ETLSource.mit_edx.name, + resource_type=LearningResourceType.course.name, + ) + + run_for_canvas_archive( + tmp_path / "archive.zip", + course_folder="123", + checksum="abc123", + overwrite=True, + ) + + assert not LearningResource.objects.get(id=orphan.id).published + assert LearningResource.objects.filter(readable_id="123-NEW101").exists() + assert LearningResource.objects.filter(id=other_course.id).exists() + assert LearningResource.objects.filter(id=other_source.id).exists() + assert mock_unpublished_actions.call_count == 1 + + +@pytest.mark.django_db +def test_run_for_canvas_archive_keeps_orphans_without_course_code(tmp_path, mocker): + """ + An archive missing course_settings.xml has no course code to compare against, + so it must not treat the folder's real resource as an orphan and delete it. + """ + mocker.patch( + "learning_resources.etl.canvas.parse_canvas_settings", + return_value={}, + ) + mocker.patch( + "learning_resources.etl.canvas_utils.parse_context_xml", + return_value={"course_id": "123", "canvas_domain": "mit.edu"}, + ) + mock_unpublished_actions = mocker.patch( + "learning_resources.etl.canvas.resource_unpublished_actions" + ) + existing = LearningResourceFactory.create( + readable_id="123-REAL101", + etl_source=ETLSource.canvas.name, + resource_type=LearningResourceType.course.name, + published=True, + test_mode=True, + ) + + run_for_canvas_archive( + tmp_path / "archive.zip", + course_folder="123", + checksum="abc123", + overwrite=True, + ) + + existing.refresh_from_db() + assert existing.published is True + assert existing.test_mode is True + assert mock_unpublished_actions.call_count == 0 + + @pytest.mark.django_db def test_run_for_canvas_archive_creates_resource_and_run(tmp_path, mocker): """ diff --git a/learning_resources/etl/canvas_utils.py b/learning_resources/etl/canvas_utils.py index 4132011053..6ddc47b5c1 100644 --- a/learning_resources/etl/canvas_utils.py +++ b/learning_resources/etl/canvas_utils.py @@ -518,6 +518,20 @@ def canvas_course_url(course_archive_path) -> str: return f"https://{context_info.get('canvas_domain')}/courses/{context_info.get('course_id')}/" +def canvas_course_folder(key: str) -> str: + """ + Return the folder a canvas archive sits in, which is its canvas course id. + + Args: + key (str): the archive's S3 key + Returns: + str: the archive's course folder + """ + return ( + key.removeprefix(settings.CANVAS_COURSE_BUCKET_PREFIX).strip("/").split("/")[0] + ) + + def _url_config_key(item): """ Get the key to look up an item from the url_config dictionary diff --git a/learning_resources/management/commands/backpopulate_canvas_courses.py b/learning_resources/management/commands/backpopulate_canvas_courses.py index 36fb04b686..d77603a272 100644 --- a/learning_resources/management/commands/backpopulate_canvas_courses.py +++ b/learning_resources/management/commands/backpopulate_canvas_courses.py @@ -3,7 +3,6 @@ from django.core.management import BaseCommand from learning_resources.tasks import sync_canvas_courses -from main.utils import now_in_utc class Command(BaseCommand): @@ -42,10 +41,12 @@ def handle(self, *args, **options): # noqa: ARG002 overwrite=options["force_overwrite"], ) self.stdout.write(f"Started task {task} to get courses from Canvas") - self.stdout.write("Waiting on task...") - start = now_in_utc() - task.get() - total_seconds = (now_in_utc() - start).total_seconds() + self.stdout.write("Waiting for the archive listing...") + queued = task.get() + if queued is None: + self.stdout.write(self.style.ERROR("No Canvas archives found")) + return self.stdout.write( - f"Population of Canvas file data finished, took {total_seconds} seconds" + f"Queued {queued} Canvas course(s) for ingestion. Each course is " + "imported by its own task; check the celery logs for progress." ) diff --git a/learning_resources/tasks.py b/learning_resources/tasks.py index 7e13feaa29..4882fbe132 100644 --- a/learning_resources/tasks.py +++ b/learning_resources/tasks.py @@ -19,6 +19,7 @@ from learning_resources.etl.canvas import ( sync_canvas_archive, ) +from learning_resources.etl.canvas_utils import canvas_course_folder from learning_resources.etl.constants import ( MARKETING_PAGE_FILE_TYPE, RESOURCE_FILE_ETL_SOURCES, @@ -703,7 +704,7 @@ def summarize_unprocessed_content( return self.replace(summarizer_tasks) -@app.task(acks_late=True) +@app.task(acks_late=True, reject_on_worker_lost=True) def ingest_canvas_course(archive_path, overwrite): bucket = get_bucket_by_name(settings.COURSE_ARCHIVE_BUCKET_NAME) return sync_canvas_archive(bucket, archive_path, overwrite=overwrite) @@ -722,15 +723,62 @@ def ingest_edx_run_archive( ) -@app.task(acks_late=True) +def unpublish_removed_canvas_courses(course_folders: list[str]) -> int: + """ + Unpublish and delete canvas courses that no longer have an archive in S3. + + A canvas readable id is f"{course_folder}-{course_code}", so the archive's + S3 folder identifies its resource on its own - the same mapping the canvas + delete webhook uses to find a resource from a canvas course id. That means + the S3 listing already knows which courses are still offered and the sweep + doesn't have to wait on the imports to report back. + + Args: + course_folders (list of str): folders of every archive currently in S3 + + Returns: + int: the number of stale courses deleted + """ + if not course_folders: + log.error("No canvas archives listed, skipping stale course cleanup") + return 0 + + current_folders = set(course_folders) + stale_ids = [ + resource_id + for resource_id, readable_id in LearningResource.objects.filter( + etl_source=ETLSource.canvas.name + ).values_list("id", "readable_id") + if readable_id.split("-", 1)[0] not in current_folders + ] + if not stale_ids: + log.info("No stale canvas courses to delete") + return 0 + + stale_courses = LearningResource.objects.filter(id__in=stale_ids) + stale_courses.update(test_mode=False, published=False) + + for resource in stale_courses: + resource_unpublished_actions(resource) + log.info("Unpublished %d stale canvas courses", len(stale_ids)) + return len(stale_ids) + + +@app.task(acks_late=True, reject_on_worker_lost=True) def sync_canvas_courses(canvas_course_ids=None, overwrite=False): # noqa: FBT002 """ - Sync all canvas course files + Sync all canvas courses from the S3 bucket, queuing an ingestion task per course. + + Each course is ingested by its own independent task; nothing waits on them, + so this returns as soon as the archives are queued. Args: canvas_course_ids (list or None): If set, sync only these canvas course ids. If None, sync every course and unpublish stale ones. overwrite (bool): Whether to overwrite existing content files + + Returns: + int or None: the number of courses queued, or None if no archives were found """ bucket = get_bucket_by_name(settings.COURSE_ARCHIVE_BUCKET_NAME) @@ -741,7 +789,7 @@ def sync_canvas_courses(canvas_course_ids=None, overwrite=False): # noqa: FBT00 for archive in exports: key = archive.key - course_folder = key.lstrip(settings.CANVAS_COURSE_BUCKET_PREFIX).split("/")[0] + course_folder = canvas_course_folder(key) log.info("processing course folder %s", course_folder) if ( @@ -756,24 +804,23 @@ def sync_canvas_courses(canvas_course_ids=None, overwrite=False): # noqa: FBT00 ) ): latest_archives[course_folder] = archive - canvas_readable_ids = [] - for archive in latest_archives.values(): - key = archive.key - log.info("Ingesting canvas course %s", key) - resource_readable_id = ingest_canvas_course( - key, - overwrite=overwrite, - ) - canvas_readable_ids.append(resource_readable_id) + if not latest_archives: + # an empty listing would sweep every canvas course away, so treat it as + # a failed run rather than as "canvas offers nothing" + log.error("No canvas archives found under %s", s3_prefix) + return None if not canvas_course_ids: - stale_courses = LearningResource.objects.filter( - etl_source=ETLSource.canvas.name - ).exclude(readable_id__in=canvas_readable_ids) - stale_courses.update(test_mode=False, published=False) - [resource_unpublished_actions(resource) for resource in stale_courses] - stale_courses.delete() + # only a full run lists every archive; a run filtered to specific + # courses must not unpublish the rest. Sweeping before the fan-out + # means a culled import can't hold up (or lose) the cleanup. + unpublish_removed_canvas_courses(list(latest_archives.keys())) + + log.info("Queueing %d canvas course archives", len(latest_archives)) + for archive in latest_archives.values(): + ingest_canvas_course.delay(archive.key, overwrite) + return len(latest_archives) @app.task(bind=True) diff --git a/learning_resources/tasks_test.py b/learning_resources/tasks_test.py index 0a23c93899..ac299def5e 100644 --- a/learning_resources/tasks_test.py +++ b/learning_resources/tasks_test.py @@ -31,6 +31,7 @@ marketing_page_for_resources, scrape_marketing_pages, sync_canvas_courses, + unpublish_removed_canvas_courses, update_next_start_date_and_prices, update_ocw_learning_material_resources, ) @@ -1149,13 +1150,10 @@ def test_scrape_marketing_pages_queues_healable_programs( assert course.id not in queued_ids -@pytest.mark.parametrize("canvas_ids", [["1"], None]) -def test_sync_canvas_courses(settings, mocker, django_assert_num_queries, canvas_ids): - """ - sync_canvas_courses should unpublish and delete stale canvas LearningResources - """ +@pytest.fixture +def canvas_archive_bucket(settings, mocker): + """Mock an S3 bucket holding one archive each for canvas folders 1 and 2""" settings.CANVAS_COURSE_BUCKET_PREFIX = "canvas/" - mocker.patch("learning_resources.tasks.resource_unpublished_actions") mock_bucket = mocker.Mock() mock_archive1 = mocker.Mock() mock_archive1.key = "canvas/1/archive1.imscc" @@ -1167,51 +1165,148 @@ def test_sync_canvas_courses(settings, mocker, django_assert_num_queries, canvas mocker.patch( "learning_resources.tasks.get_bucket_by_name", return_value=mock_bucket ) + return mock_bucket - # Create two canvas LearningResources - one stale - lr1 = LearningResourceFactory.create( - readable_id="course1", - etl_source=ETLSource.canvas.name, +@pytest.mark.parametrize("canvas_ids", [["1"], None]) +def test_sync_canvas_courses(mocker, mocked_celery, canvas_archive_bucket, canvas_ids): + """ + sync_canvas_courses should queue one ingest task per archive rather than + importing the courses inline + """ + delay_mock = mocker.patch("learning_resources.tasks.ingest_canvas_course.delay") + sweep_mock = mocker.patch( + "learning_resources.tasks.unpublish_removed_canvas_courses" + ) + + queued = sync_canvas_courses.delay( + canvas_course_ids=canvas_ids, overwrite=False + ).get() + + queued_keys = [call.args[0] for call in delay_mock.call_args_list] + if canvas_ids: + # a filtered run only queues the courses it was asked for, and doesn't + # list every archive, so it must not sweep + assert queued_keys == ["canvas/1/archive1.imscc"] + assert sweep_mock.call_count == 0 + else: + assert sorted(queued_keys) == [ + "canvas/1/archive1.imscc", + "canvas/2/archive2.imscc", + ] + # the sweep is driven by the listing, and runs before the fan-out so a + # culled import can't hold it up + assert sorted(sweep_mock.call_args.args[0]) == ["1", "2"] + assert queued == len(queued_keys) + # the imports are independent tasks - nothing waits on them, so the sync + # must not build a group/chord just to fan out + assert mocked_celery.group.call_count == 0 + assert mocked_celery.replace.call_count == 0 + + +def test_sync_canvas_courses_no_archives(mocker, mocked_celery, canvas_archive_bucket): + """ + An empty bucket listing should queue nothing and sweep nothing, rather than + reading as "canvas offers no courses" and deleting the catalog + """ + canvas_archive_bucket.objects.filter.return_value = [] + delay_mock = mocker.patch("learning_resources.tasks.ingest_canvas_course.delay") + sweep_mock = mocker.patch( + "learning_resources.tasks.unpublish_removed_canvas_courses" + ) + + assert sync_canvas_courses.delay(overwrite=False).get() is None + + assert delay_mock.call_count == 0 + assert sweep_mock.call_count == 0 + assert mocked_celery.group.call_count == 0 + assert mocked_celery.replace.call_count == 0 + + +def test_unpublish_removed_canvas_courses(mocker): + """ + unpublish_removed_canvas_courses should delete canvas resources whose course + folder is no longer in the S3 listing, matching on the readable id prefix + """ + mock_unpublished_actions = mocker.patch( + "learning_resources.tasks.resource_unpublished_actions" + ) + lr1, lr2, lr_stale = ( + LearningResourceFactory.create( + readable_id=readable_id, + etl_source=ETLSource.canvas.name, + published=True, + test_mode=True, + resource_type="course", + ) + # folder "1" must not match folder "12"'s course, hence the trailing "-" + for readable_id in ("1-COURSE1", "12-COURSE12", "3-COURSE3") + ) + other_source = LearningResourceFactory.create( + readable_id="3-COURSE3-edx", + etl_source=ETLSource.mit_edx.name, published=True, - test_mode=True, resource_type="course", ) - lr2 = LearningResourceFactory.create( - readable_id="course2", + + assert unpublish_removed_canvas_courses(["1", "12"]) == 1 + + assert not LearningResource.objects.get(id=lr_stale.id).published + assert LearningResource.objects.filter(id=lr1.id).exists() + assert LearningResource.objects.filter(id=lr2.id).exists() + assert LearningResource.objects.filter(id=other_source.id).exists() + assert mock_unpublished_actions.call_count == 1 + # the hook skips content files on a test_mode resource, so it must be handed + # the resource as it is after the unpublish, not as it was before + unpublished = mock_unpublished_actions.call_args.args[0] + assert unpublished.id == lr_stale.id + assert unpublished.test_mode is False + assert unpublished.published is False + + +def test_unpublish_removed_canvas_courses_none_stale(mocker): + """ + A listing covering every course folder should delete nothing, without + unpublishing the courses it is keeping + """ + mock_unpublished_actions = mocker.patch( + "learning_resources.tasks.resource_unpublished_actions" + ) + resource = LearningResourceFactory.create( + readable_id="1-COURSE1", etl_source=ETLSource.canvas.name, published=True, test_mode=True, resource_type="course", ) - lr_stale = LearningResourceFactory.create( - readable_id="course3", + + assert unpublish_removed_canvas_courses(["1"]) == 0 + + resource.refresh_from_db() + assert resource.published is True + assert resource.test_mode is True + assert mock_unpublished_actions.call_count == 0 + + +def test_unpublish_removed_canvas_courses_empty(mocker): + """ + A listing that came back empty should leave every canvas course alone rather + than deleting the whole catalog + """ + mocker.patch("learning_resources.tasks.resource_unpublished_actions") + resource = LearningResourceFactory.create( + readable_id="1-COURSE1", etl_source=ETLSource.canvas.name, published=True, test_mode=True, resource_type="course", ) - # Patch ingest_canvas_course to return the readable_ids for the two non-stale courses - mock_ingest_course = mocker.patch( - "learning_resources.tasks.ingest_canvas_course", - side_effect=["course1", "course2"], - ) - sync_canvas_courses(canvas_course_ids=canvas_ids, overwrite=False) + assert unpublish_removed_canvas_courses([]) == 0 - # The stale course should be unpublished and deleted - if canvas_ids: - assert LearningResource.objects.filter(id=lr_stale.id).exists() - else: - assert not LearningResource.objects.filter(id=lr_stale.id).exists() - # The non-stale courses should still exist - assert LearningResource.objects.filter(id=lr1.id).exists() - assert LearningResource.objects.filter(id=lr2.id).exists() - - if canvas_ids: - assert mock_ingest_course.call_count == 1 - else: - assert mock_ingest_course.call_count == 2 + resource.refresh_from_db() + assert resource.published is True + assert resource.test_mode is True @pytest.mark.parametrize( From 3dbc882a4b9c9b1989d0ce32f224c4d7e885169d Mon Sep 17 00:00:00 2001 From: Nathan Levesque Date: Wed, 19 Aug 2026 13:55:53 -0400 Subject: [PATCH 05/10] Make apisix userinfo updates togglable (#3747) * Make apisix userinfo updates togglable * Set default update flag to false * Address feedback --- README-keycloak.md | 19 +++++ env/backend.local.example.env | 5 ++ main/middleware/apisix_user.py | 88 ++++++++++++++++------- main/middleware/apisix_user_test.py | 105 ++++++++++++++++++++++++++++ main/settings.py | 16 +++++ 5 files changed, 207 insertions(+), 26 deletions(-) diff --git a/README-keycloak.md b/README-keycloak.md index 994eac0527..e4df9bb0c0 100644 --- a/README-keycloak.md +++ b/README-keycloak.md @@ -78,6 +78,25 @@ there emails a confirmation link rather than changing the address immediately, and the new address reaches Learn when Keycloak pushes it over SCIM. The local realm has verification off, so the change applies straight away. +### Controlling user provisioning from APISIX headers + +By default, `ApisixUserMiddleware` creates users it hasn't seen before, but does _not_ +update existing users or their profiles from the APISIX userinfo headers. Two settings in +`backend.local.env` control that: + +- `MITOL_APIGATEWAY_USERINFO_CREATE` (defaults to `True`) - controls whether the + middleware will create _new_ users. If `False`, users have to be pre-created (for + example via SCIM) before they can authenticate; an unknown identity is treated as + anonymous. +- `MITOL_APIGATEWAY_USERINFO_UPDATE` (defaults to `False`) - controls whether the + middleware will update _existing_ users. While it is `False`, neither the `User` nor its + `Profile` is written from the headers, so a backchannel (SCIM) needs to keep that data + in sync with Keycloak. Set it to `True` if nothing else is keeping users up to date. + +These names match the settings in +[mitol-django-apigateway](https://github.com/mitodl/ol-django/tree/main/src/apigateway), +which this middleware is intended to be replaced by. + ### MITx Online integration The user dashboard at `/dashboard` includes some integration with the MITx Online diff --git a/env/backend.local.example.env b/env/backend.local.example.env index 01f723f11f..1d94d4e933 100644 --- a/env/backend.local.example.env +++ b/env/backend.local.example.env @@ -24,6 +24,11 @@ MAILGUN_SENDER_DOMAIN=open.odl.local MAILGUN_KEY=fake # APISIX/Keycloak settings +# Set to False to stop provisioning users from the APISIX userinfo headers +# MITOL_APIGATEWAY_USERINFO_CREATE=False +# Set to True to enable syncing known users/profiles from the APISIX userinfo +# headers (requires SCIM to keep them in sync otherwise) +# MITOL_APIGATEWAY_USERINFO_UPDATE=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/main/middleware/apisix_user.py b/main/middleware/apisix_user.py index df8a42181b..ab81232d4a 100644 --- a/main/middleware/apisix_user.py +++ b/main/middleware/apisix_user.py @@ -105,7 +105,59 @@ def decode_apisix_headers( } -def get_user_from_apisix_headers( # noqa: C901 +def resolve_apisix_user( + request: HttpRequest, + global_id: str, + email: str, + user_fields: Mapping[str, Any], +) -> "tuple[User | None, bool]": + """ + Find the user matching the APISIX headers, creating one if that's allowed. + + Args: + request: Django request object + global_id: global_id from the APISIX headers + email: email from the APISIX headers + user_fields: User field values to create a new user with + + Returns: + (user, created) tuple. user is None if the identity is ambiguous, or if + it's unknown and MITOL_APIGATEWAY_USERINFO_CREATE is disabled. + + """ + User = get_user_model() + + if ( + request.user + and request.user.is_authenticated + and request.user.global_id == global_id + ): + return request.user, False + + candidates = User.objects.filter( + Q(global_id=global_id) | Q(global_id__isnull=True, email=email) + ).select_related("profile") + + try: + if settings.MITOL_APIGATEWAY_USERINFO_CREATE: + return candidates.get_or_create(defaults=user_fields) + return candidates.get(), False + except User.MultipleObjectsReturned: + log.exception( + "Ambiguous APISIX user identity for global_id=%s and email=%s", + global_id, + email, + ) + except User.DoesNotExist: + log.debug( + "resolve_apisix_user: User %s not found and user creation is disabled", + global_id, + ) + + return None, False + + +def get_user_from_apisix_headers( request: HttpRequest, decoded_headers: Mapping[str, Any] | None, original_header: str, @@ -139,29 +191,9 @@ def get_user_from_apisix_headers( # noqa: C901 ) user_fields = user_fields_from_headers(decoded_headers) - if ( - request.user - and request.user.is_authenticated - and request.user.global_id == global_id - ): - user = request.user - created = False - else: - try: - user, created = ( - User.objects.filter( - Q(global_id=global_id) | Q(global_id__isnull=True, email=email) - ) - .select_related("profile") - .get_or_create(defaults=user_fields) - ) - except User.MultipleObjectsReturned: - log.exception( - "Ambiguous APISIX user identity for global_id=%s and email=%s", - global_id, - email, - ) - return None + user, created = resolve_apisix_user(request, global_id, email, user_fields) + if user is None: + return None if created: log.info( @@ -191,7 +223,9 @@ def get_user_from_apisix_headers( # noqa: C901 user.set_unusable_password() user.is_active = True user.save() - elif user_needs_update(user, user_fields): + elif settings.MITOL_APIGATEWAY_USERINFO_UPDATE and user_needs_update( + user, user_fields + ): for field, value in user_fields.items(): setattr(user, field, value) user.save(update_fields=[*user_fields, "updated_on"]) @@ -199,7 +233,9 @@ def get_user_from_apisix_headers( # noqa: C901 if created: user_created_actions(user=user, is_new=True, details=profile_data) user = User.objects.select_related("profile").get(pk=user.pk) - elif profile_needs_update(user, profile_data): + elif settings.MITOL_APIGATEWAY_USERINFO_UPDATE and profile_needs_update( + user, profile_data + ): log.debug( "get_user_from_apisix_headers: Updating profile for %s", global_id, diff --git a/main/middleware/apisix_user_test.py b/main/middleware/apisix_user_test.py index ac32fda7b9..cb83a0f907 100644 --- a/main/middleware/apisix_user_test.py +++ b/main/middleware/apisix_user_test.py @@ -13,6 +13,7 @@ from main.constants import PostHogEvents from main.factories import UserFactory from main.middleware.apisix_user import ApisixUserMiddleware +from profiles.models import Profile User = get_user_model() @@ -33,6 +34,17 @@ def mock_login(mocker): return mocker.patch("main.middleware.apisix_user.login") +@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. + """ + settings.MITOL_APIGATEWAY_USERINFO_CREATE = True + settings.MITOL_APIGATEWAY_USERINFO_UPDATE = True + + @pytest.fixture(autouse=True) def setup_test_database(): """ @@ -261,3 +273,96 @@ def test_user_update_bumps_updated_on(mocker, synced_user): ) synced_user.refresh_from_db() assert synced_user.updated_on > original_updated_on + + +@pytest.mark.django_db(transaction=True) +def test_userinfo_create_disabled_unknown_user(mocker, mock_login, settings): + """With creation disabled, an unknown APISIX identity resolves to no user.""" + close_old_connections() + settings.MITOL_APIGATEWAY_USERINFO_CREATE = False + mock_logout = mocker.patch("main.middleware.apisix_user.logout") + ApisixUserMiddleware(mocker.Mock()).process_request( + mocker.Mock( + META={"HTTP_X_USERINFO": b64encode(json.dumps(apisix_user_info).encode())}, + user=AnonymousUser(), + ) + ) + assert not User.objects.filter(global_id=apisix_user_info["sub"]).exists() + mock_login.assert_not_called() + mock_logout.assert_not_called() + + +@pytest.mark.django_db(transaction=True) +def test_userinfo_create_disabled_logs_out_authenticated_user( + mocker, mock_login, settings +): + """An unresolvable identity still logs out whoever the request was authenticated as.""" + close_old_connections() + settings.MITOL_APIGATEWAY_USERINFO_CREATE = False + other_user = UserFactory.create() + mock_logout = mocker.patch("main.middleware.apisix_user.logout") + ApisixUserMiddleware(mocker.Mock()).process_request( + mocker.Mock( + META={"HTTP_X_USERINFO": b64encode(json.dumps(apisix_user_info).encode())}, + user=other_user, + ) + ) + assert not User.objects.filter(global_id=apisix_user_info["sub"]).exists() + mock_login.assert_not_called() + mock_logout.assert_called_once() + + +@pytest.mark.django_db(transaction=True) +def test_userinfo_create_disabled_existing_user(mocker, mock_login, settings): + """Creation being disabled doesn't stop a known user from authenticating.""" + close_old_connections() + settings.MITOL_APIGATEWAY_USERINFO_CREATE = False + user = UserFactory.create(global_id=apisix_user_info["sub"]) + ApisixUserMiddleware(mocker.Mock()).process_request( + mocker.Mock( + META={"HTTP_X_USERINFO": b64encode(json.dumps(apisix_user_info).encode())}, + user=AnonymousUser(), + ) + ) + mock_login.assert_called_once() + user.refresh_from_db() + assert user.email == apisix_user_info["email"] + + +@pytest.mark.django_db(transaction=True) +@pytest.mark.parametrize( + "change", + [ + ("family_name", "changed", lambda u: u.last_name), + ("name", "New Name", lambda u: u.profile.name), + ], +) +def test_userinfo_update_disabled_skips_writes(mocker, settings, synced_user, change): + """With updates disabled, changed header fields aren't written to a known user.""" + changed_field, new_value, get_attr = change + settings.MITOL_APIGATEWAY_USERINFO_UPDATE = False + original = get_attr(synced_user) + changed_header = b64encode( + json.dumps({**apisix_user_info, changed_field: new_value}).encode() + ) + ApisixUserMiddleware(mocker.Mock()).process_request( + mocker.Mock(META={"HTTP_X_USERINFO": changed_header}, user=synced_user) + ) + reloaded = User.objects.select_related("profile").get(pk=synced_user.pk) + assert get_attr(reloaded) == original + + +@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.""" + close_old_connections() + settings.MITOL_APIGATEWAY_USERINFO_UPDATE = False + user = UserFactory.create(global_id=apisix_user_info["sub"], no_profile=True) + ApisixUserMiddleware(mocker.Mock()).process_request( + mocker.Mock( + META={"HTTP_X_USERINFO": b64encode(json.dumps(apisix_user_info).encode())}, + user=AnonymousUser(), + ) + ) + mock_login.assert_called_once() + assert not Profile.objects.filter(user=user).exists() diff --git a/main/settings.py b/main/settings.py index 6edebe91c3..2a9c848ff9 100644 --- a/main/settings.py +++ b/main/settings.py @@ -352,6 +352,22 @@ default=False, ) +# Set to True to create users that we see but aren't aware of. +# Set to False if you're managing that elsewhere (like with SCIM). +# Named to match mitol-django-apigateway, which we intend to port to. +MITOL_APIGATEWAY_USERINFO_CREATE = get_bool( + name="MITOL_APIGATEWAY_USERINFO_CREATE", + default=True, +) + +# Set to True to update users we've seen before. If you set this to False, make +# sure there's a backchannel way to update the user data (SCIM, etc) or user +# info will fall out of sync with the IdP pretty quickly. +MITOL_APIGATEWAY_USERINFO_UPDATE = get_bool( + name="MITOL_APIGATEWAY_USERINFO_UPDATE", + default=False, +) + # Social Auth configurations - [END] # Static files (CSS, JavaScript, Images) From d6117e02ff484c406508f5f3060379f0423d32d4 Mon Sep 17 00:00:00 2001 From: Chris Chudzicki Date: Wed, 19 Aug 2026 15:18:36 -0400 Subject: [PATCH 06/10] Display price ranges on product pages (#3794) * Put the program price amounts on their theme typography tokens ProgramPriceAmount and ProgramListPriceAmount were the only elements in the Certificate Track card not using the theme font: both hardcoded 'Helvetica Neue', Helvetica, Arial alongside hand-typed sizes, while the title, both captions, and the savings line all rendered in neue-haas-grotesk-text. On macOS that showed as two typefaces in one card; everywhere else the amounts fell back to Arial. The hardcoded metrics turn out to be the tokens spelled out by hand -- 34/40 bold is exactly h2, and 28/36 is exactly h3 -- which matches what the designs specify. So this swaps in theme.typography.h2 and h3 and drops the font-family overrides. Rendered size and weight are unchanged: h3 is bold, so the list price overrides fontWeight back to regular, which is deliberate rather than incidental -- the struck comparison price is meant to read lighter than the current price beside it. Neue Haas Grotesk is wider than Helvetica at the same size (a $499 - $1,499 range measures 235px against 215px), so rows containing a wide price wrap slightly sooner than before. Co-Authored-By: Claude Opus 5 (1M context) * Draw the program price separator as a gap decoration The vertical rule between the current price and the struck list price was a 1px x 48px div sitting between them as a third flex item. That row wraps in narrow cells, and when it did the rule stranded itself at the end of the first line, floating beside the current price with nothing left to separate it from. A separator between two items is only meaningful when they share a line, and that is what gap decorations express: the rule is painted into gaps that exist, so wrapping removes it with no wrap detection. CSS has no way to select "the item that ended up first on a wrapped line", so a sibling element could never have known to hide itself. Because the rule now lives in the gap rather than beside it, the column gap absorbs the spacing the divider used to get from a gap on either side: 24 + 1 + 24 becomes a 48px gap with the rule down its middle, which keeps the blocks the same distance apart to within the width of the rule itself. Where gap decorations are unsupported nothing is drawn, which is an acceptable resting state: the two prices are already distinguished by size, weight, colour, strikethrough, and their captions. Dropping the element also gives the rule an intrinsic height instead of an arbitrary 48px. Co-Authored-By: Claude Opus 5 (1M context) * Show advertised price ranges on product pages (hq#12786) mitxonline exposes min_price/max_price on courses and programs. Where the two differ the resource is advertised as a range, but the product pages rendered only the single certificate product price, so an about page could say $1,000 while the resource drawer next to it said $250 - $1,000. That discrepancy already existed because mit-learn's own ETL reads min/max with no additional gating (learning_resources/etl/mitxonline.py, parse_prices). common/mitxonline gains toPriceRange, formatPriceRange, and formatResourcePrice. The last is MitxOnlineResourceCard's local helper lifted out, with two corrections: the range predicate is min < max rather than min !== max, and the separator is an en dash to match getDisplayPrice in ol-utilities, so a resource reads identically on the about page and in the drawer. Both certificate-price hooks route the displayed price through it, behind their unchanged `if (!product?.price)` gate, so a resource with no purchasable product still shows no price. ProgramSavingsBlock now takes a PriceRange: savings derive from the top of the range and read "Save $150+", and a list price falling inside the range drops the savings framing entirely, since it does not beat every price in the range. TrackCard's header becomes a title-plus-price row with the subtitle at full card width beneath it. The subtitle, not the price, was what forced the wrap: a flex item's line-breaking uses its max-content width, and the subtitle's 195px beat the price every time. With the subtitle out of that row the title's flexGrow lets its own text decide when the price wraps. Ranges additionally render one step down the heading scale (compactPrice, h5) so they sit beside the title rather than below it, which is how spec item 8 is satisfied. The EnrollAreas set that flag from the same toPriceRange predicate the hooks format on, so sizing tracks display. Two things not to retry here. Do not claw back the header's extra height with a negative margin: when the price wraps it is the last flex line, so there is nothing beneath it to reclaim from and the glyphs collide with the subtitle. Do not render a smaller price by nesting a smaller element inside the price container: the container's line box is struck for its own font size, so smaller text inside sits on that larger strut's baseline and hangs below the title however the boxes are aligned. One element, one type token. A range never fits beside a list price in ProgramSavingsBlock at the desktop sidebar width -- 312px of content against a 235px range, a 121px caption and the gap between them -- so that row is always wrapped for ranges. No type scale reaches it; verified by measuring the real card with the gap zeroed, the rule removed and the caption already wrapped, which still needed 327px. The mitxonline test factories previously drew min_price and max_price as independent faker values, so course fixtures always advertised a range and program fixtures did about half the time. They now default equal, making a range opt-in per test; without that this change makes unrelated suites flaky. Co-Authored-By: Claude Opus 5 (1M context) * Move the financial aid link into the certificate card header Per the hq#12786 wireframes, the financial aid link leaves the feature list and becomes its own row directly beneath the Certificate Track title -- the slot the design pairs with a right-hand price caption. It loses its check icon along with its place among the bullets. TrackCard gains a headerAside slot for it. The slot sits 4px under the title row so it reads as part of the title rather than as another header row, and widens the header's gap to the subtitle to 16px when filled. Cards with no aside keep the 8px gap and are unchanged. The link is now driven by linkStyles rather than a hand-rolled rule. Its small "red" variant is body3 at #A31F34, matching the wireframe exactly, so the unapplied state needs no override. The approved state keeps that scale and swaps in darkGreen, which the Link component has no variant for: the green used by the feature check icons is only 2.7:1 against the card and fails AA as text. Green marks a resolved state rather than a call to action, so it also drops the resting underline and takes one on hover -- it still links to the application record, but users have no reason to follow it. Copy becomes "Apply for financial aid" / "Financial aid applied (visible at checkout)". The enrollment dialog's own financial aid link is a separate surface and keeps its wording. Co-Authored-By: Claude Opus 5 (1M context) * Address review feedback on the certificate card price and aid link Pin PriceContainer's line height to the title's. The price is the taller of the two in the flattened title row, so its token leading was setting the row height and pushing the subtitle and everything under it down 10px relative to the pre-flattening layout -- visible on every card with a top-right price. This also restores the aid link's 4px gap to the title, which was measuring from the row's bottom rather than the title's. Show an advertised price even when a resource has no purchasable product. Both hooks previously returned a null price before reaching the formatter, so a resource with an advertised range showed it on the carousel card and no price at all in the InfoBox. Program savings stay behind the product guard: there is nothing to have saved without a price you would pay. Withhold the aid link while the approval lookup is in flight, holding its row so resolving it does not shift the card. The lookup is client-only, so an already-approved user was being shown the red "apply" call to action on first paint. `pending` reads isLoading rather than isPending, because a disabled query stays pending forever and this one is disabled for anonymous visitors, who have nothing to wait for. The approved state's green now matches the `Save $X+` text that can sit a few rows below it in the same card, rather than introducing a second green. It is a raw hex in both places; no token is this shade, and the `green` token fails AA as text at 2.7:1 against the card. Also: approved copy reads "approved" rather than "applied", which otherwise collided with having submitted an application; the financial aid shape is a shared FinancialAid type instead of three inline duplicates; and the range-beats-product-price tests use a product price distinct from both ends of the range, so an implementation composing the range's minimum with the product price no longer passes them. Co-Authored-By: Claude Opus 5 (1M context) * Narrow the factory's advertised price before formatting it The `no product` test formats `program.min_price` for its expectation, but the field is `number | null` on the API type even though the factory always sets it, so `yarn typecheck` failed on the branch. An invariant narrows it and documents the factory assumption the expectation rests on. Co-Authored-By: Claude Opus 5 (1M context) * Draw the program price rule with a clip instead of a gap decoration `column-rule` on a flex container is CSS Gap Decorations (css-gaps-1), which Chrome and Edge only shipped in 149 and Firefox and Safari have not shipped at all, so the separator was missing for most visitors -- including in the common single-price-plus-savings case, where the two blocks do share a line. `@supports` cannot detect this, since the declaration parses everywhere for multi-column layout. Each block now paints a rule in the gap preceding it, and the row clips whatever lands at its left content edge, which is exactly the rule of a block that starts a line. That keeps the property the gap decoration was chosen for -- the rule disappears when the row wraps, with no width breakpoint predicting where the prices wrap -- using only overflow clipping and an absolutely positioned pseudo-element. Co-Authored-By: Claude Opus 5 (1M context) * Render an advertised range at the title's size, per the design The wireframe puts the range price on Subtitle/S1 -- 16px, one weight lighter than the title beside it -- rather than a step down the heading scale, so the compact price is subtitle1 instead of h5. A single price keeps h4. The line-height pin stays: it is what keeps h4 from adding 10px to the header, and is simply redundant for subtitle1, which already has that line height. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../test-utils/factories/courses.ts | 7 +- .../test-utils/factories/programs.ts | 7 +- .../CertificateTrackCard.test.tsx | 29 ++++-- .../ProductPages/CertificateTrackCard.tsx | 70 +++++++++++--- .../ProductPages/CourseEnrollArea.test.tsx | 67 ++++++++++++- .../ProductPages/CourseEnrollArea.tsx | 4 + .../ProductPages/EnrollOfferingBoxes.tsx | 9 +- .../InfoBoxProgramAsCourse.test.tsx | 2 +- .../MitxOnlineResourceCard.test.tsx | 2 +- .../ProductPages/MitxOnlineResourceCard.tsx | 37 +------- .../ProductPages/ProgramEnrollArea.test.tsx | 2 +- .../ProductPages/ProgramEnrollArea.tsx | 4 + .../ProductPages/ProgramSavingsBlock.test.tsx | 18 +++- .../ProductPages/ProgramSavingsBlock.tsx | 92 ++++++++++++------ .../src/app-pages/ProductPages/TrackCard.tsx | 94 +++++++++++++++---- .../src/app-pages/ProductPages/enrollTypes.ts | 12 +++ .../ProductPages/useCourseCertificatePrice.ts | 26 +++-- .../useProgramCertificatePrice.test.tsx | 75 ++++++++++++++- .../useProgramCertificatePrice.ts | 62 +++++++----- frontends/main/src/common/mitxonline.test.ts | 63 +++++++++++++ frontends/main/src/common/mitxonline/index.ts | 57 +++++++++++ 21 files changed, 589 insertions(+), 150 deletions(-) diff --git a/frontends/api/src/mitxonline/test-utils/factories/courses.ts b/frontends/api/src/mitxonline/test-utils/factories/courses.ts index 116cb487ed..3f64c5dfd4 100644 --- a/frontends/api/src/mitxonline/test-utils/factories/courses.ts +++ b/frontends/api/src/mitxonline/test-utils/factories/courses.ts @@ -148,6 +148,9 @@ const course: PartialFactory = ( : runs.length > 0 ? faker.helpers.arrayElement(runs).id : null + // Almost every real course advertises a single price (min === max); an + // advertised range is the flexible-pricing exception, so tests opt into it. + const advertisedPrice = faker.number.int({ min: 50, max: 2000 }) const defaults: CourseWithCourseRunsSerializerV2 = { id: uniqueCourseId.enforce(() => faker.number.int()), title: faker.lorem.words(3), @@ -192,8 +195,8 @@ const course: PartialFactory = ( min_weekly_hours: `${faker.number.int({ min: 1, max: 5 })} hours`, max_weekly_hours: `${faker.number.int({ min: 6, max: 10 })} hours`, courseruns: runs, - min_price: faker.number.int({ min: 0, max: 1000 }), - max_price: faker.number.int({ min: 1000, max: 2000 }), + min_price: advertisedPrice, + max_price: advertisedPrice, include_in_learn_catalog: faker.datatype.boolean(), ingest_content_files_for_ai: faker.datatype.boolean(), possible_variant_sets: [], diff --git a/frontends/api/src/mitxonline/test-utils/factories/programs.ts b/frontends/api/src/mitxonline/test-utils/factories/programs.ts index c1abff94f0..3239b1b5b0 100644 --- a/frontends/api/src/mitxonline/test-utils/factories/programs.ts +++ b/frontends/api/src/mitxonline/test-utils/factories/programs.ts @@ -27,6 +27,9 @@ const baseProgram: Factory = (overrides = {}) => { } const program: PartialFactory = (overrides = {}) => { + // Almost every real program advertises a single price (min === max); an + // advertised range is the flexible-pricing exception, so tests opt into it. + const advertisedPrice = faker.number.int({ min: 50, max: 5000 }) const defaults: V2ProgramDetail = { id: uniqueProgramId.enforce(() => faker.number.int()), title: faker.lorem.words(3), @@ -87,8 +90,8 @@ const program: PartialFactory = (overrides = {}) => { min_weekly_hours: `${faker.number.int({ min: 1, max: 5 })} hours`, max_weekly_hours: `${faker.number.int({ min: 6, max: 10 })} hours`, start_date: faker.date.past().toISOString(), - max_price: faker.number.int({ min: 50, max: 5000 }), - min_price: faker.number.int({ min: 50, max: 5000 }), + max_price: advertisedPrice, + min_price: advertisedPrice, enrollment_start: faker.helpers.maybe(() => faker.date.past().toISOString(), ), diff --git a/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.test.tsx b/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.test.tsx index 81d5f45479..f5f91fe2a4 100644 --- a/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.test.tsx +++ b/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.test.tsx @@ -46,14 +46,14 @@ describe("CertificateTrackCard", () => { test.each([ { - name: "available, when not applied", + name: "apply, when not applied", applied: false, - linkText: "Financial assistance available", + linkText: "Apply for financial aid", }, { - name: "approved (applied at checkout), when applied", + name: "applied (visible at checkout), when applied", applied: true, - linkText: "Financial assistance approved (applied at checkout)", + linkText: "Financial aid approved (visible at checkout)", }, ])( "renders financial aid link to the form — $name", @@ -63,7 +63,7 @@ describe("CertificateTrackCard", () => { $250} productNoun="course" - financialAid={{ href, applied }} + financialAid={{ href, applied, pending: false }} />, ) const link = screen.getByRole("link", { name: linkText }) @@ -72,13 +72,26 @@ describe("CertificateTrackCard", () => { }, ) + test("reserves the row without a link while approval is still loading", () => { + renderWithProviders( + $250} + productNoun="course" + financialAid={{ + href: "https://example.com/financial-aid", + applied: false, + pending: true, + }} + />, + ) + expect(screen.queryByRole("link", { name: /financial aid/i })).toBeNull() + }) + test("does not render a financial aid link when financialAid is not provided", () => { renderWithProviders( $250} productNoun="course" />, ) - expect( - screen.queryByRole("link", { name: /Financial assistance/ }), - ).toBeNull() + expect(screen.queryByRole("link", { name: /financial aid/i })).toBeNull() }) test("renders the action node when provided", () => { diff --git a/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.tsx b/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.tsx index 6f97b97652..a099e44534 100644 --- a/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.tsx +++ b/frontends/main/src/app-pages/ProductPages/CertificateTrackCard.tsx @@ -1,20 +1,52 @@ import React from "react" import { styled } from "@mitodl/smoot-design" +import { linkStyles } from "ol-components" +import type { FinancialAid } from "./enrollTypes" import TrackCard, { FeatureRow, FeatureIcon, AccessFeatureRow, } from "./TrackCard" -const FinancialAidLink = styled.a(({ theme }) => ({ - ...theme.typography.body3, - color: theme.custom.colors.darkGray2, - textDecoration: "underline", +/** + * `linkStyles`' small "red" link is the body3 scale the card wants, and its red + * is the call-to-action colour for the unapproved state. The approved state + * keeps that scale and swaps only the colour, which Link has no variant for: + * green marks it as a resolved state rather than something to act on, so it also + * drops the resting underline and takes one on hover instead — it stays a link + * to the application record, but users have no reason to follow it. + * + * The green matches the `Save $X+` text in ProgramSavingsBlock, which can sit a + * few rows below it in the same card. It is a raw hex in both places: the + * `green` token is only 2.7:1 against the card and fails AA as text, and no + * other token is this shade. + */ +const APPROVED_GREEN = "#008000" + +const FinancialAidLink = styled.a<{ $approved?: boolean }>( + linkStyles({ size: "small", color: "red" }), + ({ $approved }) => + $approved + ? { + color: APPROVED_GREEN, + ":hover": { color: APPROVED_GREEN, textDecoration: "underline" }, + } + : { textDecoration: "underline" }, +) + +/** + * Holds the aid link's row while the approval lookup is in flight, so resolving + * it does not shift the rest of the card. Sized by the link's own line box. + */ +const FinancialAidPlaceholder = styled.span(({ theme }) => ({ + display: "block", + height: theme.typography.body3.lineHeight, })) type CertificateTrackCardProps = { price: React.ReactNode - financialAid?: { href: string; applied: boolean } | null + compactPrice?: boolean + financialAid?: FinancialAid | null productNoun: "course" | "program" priceBlock?: React.ReactNode action?: React.ReactNode @@ -23,6 +55,7 @@ type CertificateTrackCardProps = { const CertificateTrackCard: React.FC = ({ price, + compactPrice, financialAid, productNoun, priceBlock, @@ -35,7 +68,24 @@ const CertificateTrackCard: React.FC = ({ title="Certificate Track" subtitle="Earn a verified certificate of completion" price={price} + compactPrice={compactPrice} priceBlock={priceBlock} + headerAside={ + financialAid ? ( + financialAid.pending ? ( + + ) : ( + + {financialAid.applied + ? "Financial aid approved (visible at checkout)" + : "Apply for financial aid"} + + ) + ) : null + } action={action} fill={fill} > @@ -48,16 +98,6 @@ const CertificateTrackCard: React.FC = ({