diff --git a/README-keycloak.md b/README-keycloak.md index 559ab10e03..e4df9bb0c0 100644 --- a/README-keycloak.md +++ b/README-keycloak.md @@ -51,6 +51,52 @@ 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. + +### 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/RELEASE.rst b/RELEASE.rst index bdea52c7cc..5df55ba790 100644 --- a/RELEASE.rst +++ b/RELEASE.rst @@ -1,6 +1,19 @@ Release Notes ============= +Version 0.77.10 +--------------- + +- version bump for the mynumber and hacksnack games (#3798) +- always select currently running course run for card context (#3792) +- refactor(otel): own the OpenTelemetry setup instead of patching Sentry's (#3788) +- Display price ranges on product pages (#3794) +- Make apisix userinfo updates togglable (#3747) +- make canvas etl resistant to pod culling (#3779) +- fix(otel): continue the edge trace by extracting W3C traceparent (#3787) +- Update Terms of Service (MicroMasters bundle, AI Tutor, date) (#3767) +- feat(settings): change email and password via Keycloak (#3726) + Version 0.77.8 (Released August 19, 2026) -------------- 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/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/env/frontend.env b/env/frontend.env index db8b5f5e30..aeae1d863a 100644 --- a/env/frontend.env +++ b/env/frontend.env @@ -51,10 +51,10 @@ GTM_COOKIES_WIN=${GTM_COOKIES_WIN} # OpenTelemetry tracing (server-side only — no NEXT_PUBLIC_ prefix needed) # These are read at runtime by the OTEL NodeSDK and injected by Kubernetes for -# deployed environments. Sampling is disabled locally (0.0); set -# OTEL_EXPORTER_OTLP_TRACES_ENDPOINT (or OTEL_EXPORTER_OTLP_ENDPOINT) and -# OTEL_TRACES_SAMPLER_ARG in your K8s/Helm values to enable tracing in -# staging/production. +# deployed environments. Set OTEL_EXPORTER_OTLP_TRACES_ENDPOINT (or +# OTEL_EXPORTER_OTLP_ENDPOINT) in your K8s/Helm values to enable tracing in +# staging/production. There is no head-sampling knob: every span is created, +# and the Grafana Alloy tail sampler decides what Tempo keeps. # # OTEL_SERVICE_NAME=mit-learn-frontend # OTEL_EXPORTER_OTLP_ENDPOINT=http://alloy.monitoring:4318 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/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/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/package.json b/frontends/main/package.json index d4fe006d6b..53e8085b87 100644 --- a/frontends/main/package.json +++ b/frontends/main/package.json @@ -15,9 +15,9 @@ "@emotion/cache": "^11.13.1", "@emotion/styled": "^11.11.0", "@floating-ui/react": "^0.27.16", - "@mitodl/arithmix": "^0.2.3", + "@mitodl/arithmix": "^0.2.4", "@mitodl/course-search-utils": "^3.5.2", - "@mitodl/hacksnack": "^0.1.1", + "@mitodl/hacksnack": "^0.1.2", "@mitodl/mitxonline-api-axios": "2026.8.18", "@mitodl/smoot-design": "6.31.1", "@mui/base": "5.0.0-beta.70", @@ -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/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/ContractContent.test.tsx b/frontends/main/src/app-pages/DashboardPage/ContractContent.test.tsx index 5cdeca9724..847988b434 100644 --- a/frontends/main/src/app-pages/DashboardPage/ContractContent.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/ContractContent.test.tsx @@ -51,6 +51,16 @@ const API_BASE_URL = process.env.NEXT_PUBLIC_MITX_ONLINE_BASE_URL const managerOrganizationsUrl = `${API_BASE_URL}/api/v0/b2b/manager/organizations/` const makeCourseEnrollment = factories.enrollment.courseEnrollment + +// The progress badge describes the displayed run, so any enrollment whose badge +// is asserted has to pin the dates it reads instead of taking the factory's +// random ones. +const daysFromNow = (days: number) => + new Date(Date.now() + days * 24 * 60 * 60 * 1000).toISOString() +const underwayRunDates = { + start_date: daysFromNow(-30), + end_date: daysFromNow(30), +} const makeGrade = factories.enrollment.grade const normalizeCourseForCardAssertions = ( @@ -297,6 +307,7 @@ describe("ContractContent", () => { id: normalizedCoursesA[1].id, title: normalizedCoursesA[1].title, }, + ...underwayRunDates, }, grades: [], certificate: null, @@ -1244,6 +1255,7 @@ describe("ContractContent", () => { (r) => r.b2b_contract === contractIds[0], )?.id, course: { id: courses[1].id, title: courses[1].title }, + ...underwayRunDates, }, b2b_contract_id: contracts[0].id, b2b_organization_id: contracts[0].organization, diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx index 01a5438629..1fde5e8ae5 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.test.tsx @@ -178,6 +178,33 @@ describe.each([ ) }) + test("shows View Certificate link when the certificate is on a sibling run", () => { + setupUserApis() + const certUuid = faker.string.uuid() + const certificateLink = `https://courses.example.com/certificate/${certUuid}/` + const displayedEnrollment = + mitxonline.factories.enrollment.courseEnrollment({ + certificate: null, + run: currentRunDates, + }) + const earlierEnrollment = mitxonline.factories.enrollment.courseEnrollment({ + certificate: { uuid: certUuid, link: certificateLink }, + run: pastRunDates, + }) + renderWithProviders( + , + ) + expect( + within(getCard()).getByRole("link", { name: /View Certificate/ }), + ).toHaveAttribute( + "href", + `https://courses.example.com/certificate/course/${certUuid}/`, + ) + }) + test("does not show View Certificate when certificate is absent", () => { setupUserApis() const enrollment = mitxonline.factories.enrollment.courseEnrollment({ @@ -1085,12 +1112,30 @@ describe("EnrolledCourseCard progress badge", () => { const getDesktopCard = () => screen.getByTestId("enrollment-card-desktop") + // The badge describes the displayed run, so every case pins the run dates it + // reads rather than leaving them to the factory's random values. test.each([ { + case: "run underway", + runDates: currentRunDates, enrollmentData: { grades: [], certificate: null }, expectedLabel: "In Progress", }, { + case: "run not yet started", + runDates: futureRunDates, + enrollmentData: { grades: [], certificate: null }, + expectedLabel: "Not Started", + }, + { + case: "run over without a certificate", + runDates: pastRunDates, + enrollmentData: { grades: [], certificate: null }, + expectedLabel: "Ended", + }, + { + case: "certificate earned", + runDates: pastRunDates, enrollmentData: { grades: [mitxonline.factories.enrollment.grade({ passed: true })], certificate: { @@ -1101,12 +1146,13 @@ describe("EnrolledCourseCard progress badge", () => { expectedLabel: "Completed", }, ])( - "shows '$expectedLabel' next to the card type label", - ({ enrollmentData, expectedLabel }) => { + "shows '$expectedLabel' next to the card type label ($case)", + ({ runDates, enrollmentData, expectedLabel }) => { setupUserApis() const enrollment = mitxonline.factories.enrollment.courseEnrollment({ ...enrollmentData, b2b_contract_id: null, + run: runDates, }) renderWithProviders() expect( diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx index 768e641ad5..0dbf8eaae9 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/EnrolledCourseCard.tsx @@ -22,6 +22,7 @@ import { DashboardType, getCertificateLink, getDashboardEnrollmentStatus, + pickCertificateEnrollment, } from "./model/dashboardViewModel" import { getCourseDateText } from "./courseDateUtils" import { isVerifiedEnrollmentMode } from "@/common/mitxonline" @@ -273,7 +274,8 @@ export const EnrolledCourseCard = ({ const title = isCompact ? course.title : run?.title || course.title const coursewareUrl = run?.courseware_url const certificateLink = getCertificateLink( - enrollment?.certificate?.link, + pickCertificateEnrollment([enrollment, ...(siblingEnrollments ?? [])]) + ?.certificate?.link, "course", ) const enrollmentMode = enrollment?.enrollment_mode @@ -499,7 +501,11 @@ export const EnrolledCourseCard = ({ const progressBadgeSection = isModule && isCompact ? null : ( - + {cardTypeLabel} diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/HomeEnrollmentsDisplay.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/HomeEnrollmentsDisplay.test.tsx index 6ddcd1d085..a02ab5bf73 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/HomeEnrollmentsDisplay.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/HomeEnrollmentsDisplay.test.tsx @@ -104,13 +104,18 @@ describe("HomeEnrollmentsDisplay", () => { const sharedCourseId = faker.number.int() // Both enrollments share the same course and have identical default variant // fields, so they should collapse to a single card. - // Run A is the more recent of the two so the dedup policy (recency, - // since neither has a certificate or grade) deterministically picks it. + // Run A is the one in progress, so the recency-based dedup policy + // deterministically picks it. Both dates are pinned because that policy + // reads start_date and end_date, which the factory would otherwise fill + // with random values. + const daysFromNow = (days: number) => + new Date(Date.now() + days * 24 * 60 * 60 * 1000).toISOString() const enrollmentA = mitxonline.factories.enrollment.courseEnrollment({ run: { title: "Same Course — Run A", course: { id: sharedCourseId }, - start_date: "2024-01-01T00:00:00Z", + start_date: daysFromNow(-30), + end_date: daysFromNow(30), }, certificate: null, grades: [], @@ -119,7 +124,8 @@ describe("HomeEnrollmentsDisplay", () => { run: { title: "Same Course — Run B", course: { id: sharedCourseId }, - start_date: "2020-01-01T00:00:00Z", + start_date: daysFromNow(-800), + end_date: daysFromNow(-700), }, certificate: null, grades: [], diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx index 13f063f6fa..2c414ec898 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.test.tsx @@ -127,6 +127,52 @@ describe("ProgramAsCourseCard", () => { ).toBeGreaterThan(0) }) + test("counts a course as complete when an earlier run was passed and the learner re-enrolled", async () => { + const cardData = setupCardData({ includeProgramEnrollment: true }) + const [moduleOne] = cardData.moduleCourses + // enrollment_mode is pinned to audit on both: the factory picks it at + // random, and a verified enrollment sends the card looking for a receipt + // via orders/history, which is not mocked here. The count only reads grades. + const passedEnrollment = mitxonline.factories.enrollment.courseEnrollment({ + run: { + ...moduleOne.courseruns[0], + course: moduleOne, + start_date: moment().subtract(400, "days").toISOString(), + end_date: moment().subtract(300, "days").toISOString(), + }, + enrollment_mode: "audit", + grades: [mitxonline.factories.enrollment.grade({ passed: true })], + certificate: null, + }) + const reEnrollment = mitxonline.factories.enrollment.courseEnrollment({ + run: { + course: moduleOne, + start_date: moment().subtract(30, "days").toISOString(), + end_date: moment().add(30, "days").toISOString(), + }, + enrollment_mode: "audit", + grades: [], + certificate: null, + }) + + renderWithProviders( + , + ) + + // The re-enrollment is the run the card displays, but completion belongs to + // the course, so the passed earlier run still has to count. + expect( + await screen.findByText("2 Modules (1 of 2 complete)"), + ).toBeInTheDocument() + }) + test("module rows show an enrollment status indicator instead of a 'Module' label", async () => { const cardData = setupCardData({ includeProgramEnrollment: true }) diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx index 817b1512b3..0d7ce48edf 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgramAsCourseCard.tsx @@ -14,14 +14,7 @@ import { V3UserProgramEnrollment, V2ProgramRequirement, } from "@mitodl/mitxonline-api-axios/v2" -import { - EnrollmentStatus, - getEnrollmentStatus, - getKey, - getProgramEnrollmentStatus, - ResourceType, - selectBestEnrollment, -} from "./helpers" +import { getKey, getProgramEnrollmentStatus, ResourceType } from "./helpers" import { ProgressBadge } from "./ProgressBadge" import { CoursewareCard } from "./CoursewareCard" import { @@ -34,6 +27,7 @@ import { import { getCertificateLink, buildCourseEntry, + courseIsCompleted, } from "./model/dashboardViewModel" import { getIdsFromReqTree, @@ -309,21 +303,18 @@ const ProgramAsCourseCard: React.FC = ({ Boolean(course), ) + // Counted across all of a course's enrollments rather than the one a card + // would display: the displayed run is the one underway, so a learner who + // passed an earlier run and re-enrolled would otherwise stop counting as + // having completed the course. const enrolledCount = displayedModuleCourses.filter((course) => { - const bestEnrollment = selectBestEnrollment( - course, - moduleEnrollmentsByCourseId[course.id] || [], - ) - return getEnrollmentStatus(bestEnrollment) === EnrollmentStatus.Enrolled + const enrollments = moduleEnrollmentsByCourseId[course.id] || [] + return enrollments.length > 0 && !courseIsCompleted(enrollments) }).length - const completedCount = displayedModuleCourses.filter((course) => { - const bestEnrollment = selectBestEnrollment( - course, - moduleEnrollmentsByCourseId[course.id] || [], - ) - return getEnrollmentStatus(bestEnrollment) === EnrollmentStatus.Completed - }).length + const completedCount = displayedModuleCourses.filter((course) => + courseIsCompleted(moduleEnrollmentsByCourseId[course.id] || []), + ).length const totalCount = displayedModuleCourses.length diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgressBadge.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgressBadge.tsx index 729b083286..00cc16d503 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgressBadge.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/ProgressBadge.tsx @@ -1,17 +1,27 @@ import React from "react" import { styled, Typography } from "ol-components" import { EnrollmentStatus } from "./helpers" +import { getRunTimeState } from "./courseDateUtils" import { RiCheckLine } from "@remixicon/react" +type BadgeVariant = "completed" | "in-progress" | "ended" | "not-started" + +const BADGE_LABELS: Record = { + completed: "Completed", + "in-progress": "In Progress", + ended: "Ended", + "not-started": "Not Started", +} + const BadgeContainer = styled("div")<{ - status: EnrollmentStatus -}>(({ theme, status }) => { + variant: BadgeVariant +}>(({ theme, variant }) => { let backgroundColor = theme.custom.colors.lightGray1 let color = theme.custom.colors.silverGrayDark - if (status === EnrollmentStatus.Completed) { + if (variant === "completed") { backgroundColor = `${theme.custom.colors.black}0A` - } else if (status === EnrollmentStatus.Enrolled) { + } else if (variant === "in-progress") { backgroundColor = `${theme.custom.colors.red}0A` color = theme.custom.colors.red } @@ -30,22 +40,43 @@ const BadgeContainer = styled("div")<{ } }) +/** + * The badge describes the run the card is displaying, so an enrolled learner + * whose run has not started yet reads as "Not Started" and one whose run is + * over reads as "Ended" — never "In Progress", which the dates contradict. + * Callers without a single run to point at (programs) simply omit the dates. + */ +const getBadgeVariant = ( + enrollmentStatus: EnrollmentStatus, + startDate?: string | null, + endDate?: string | null, +): BadgeVariant => { + if (enrollmentStatus === EnrollmentStatus.Completed) return "completed" + if (enrollmentStatus !== EnrollmentStatus.Enrolled) return "not-started" + + const timeState = getRunTimeState(startDate, endDate) + if (timeState === "upcoming") return "not-started" + if (timeState === "ended") return "ended" + return "in-progress" +} + interface ProgressBadgeProps { enrollmentStatus: EnrollmentStatus + startDate?: string | null + endDate?: string | null } -const ProgressBadge: React.FC = ({ enrollmentStatus }) => { - const label = - enrollmentStatus === EnrollmentStatus.Completed - ? "Completed" - : enrollmentStatus === EnrollmentStatus.Enrolled - ? "In Progress" - : "Not Started" +const ProgressBadge: React.FC = ({ + enrollmentStatus, + startDate, + endDate, +}) => { + const variant = getBadgeVariant(enrollmentStatus, startDate, endDate) return ( - - {label} - {enrollmentStatus === EnrollmentStatus.Completed && ( + + {BADGE_LABELS[variant]} + {variant === "completed" && ( diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.test.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.test.tsx index 8b3bc6645f..dd885f4a80 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.test.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.test.tsx @@ -139,6 +139,74 @@ describe("SiblingRunsToggle + SiblingRunsPanel", () => { expect(screen.getByText(/Jan 5, 2026/)).toBeInTheDocument() }) + // The run icons are aria-hidden, so this label is the only status a screen + // reader gets for the current run. Every time state has to produce words. + test.each([ + { + case: "still running", + startDate: moment().subtract(90, "days").toISOString(), + endDate: moment().add(30, "days").toISOString(), + expected: "In Progress", + }, + { + case: "already ended", + startDate: moment().subtract(90, "days").toISOString(), + endDate: moment().subtract(30, "days").toISOString(), + expected: "Ended", + }, + { + case: "not yet started", + startDate: moment().add(30, "days").toISOString(), + endDate: moment().add(90, "days").toISOString(), + expected: "Upcoming", + }, + ])( + "current run $case is labelled '$expected'", + async ({ startDate, endDate, expected }) => { + const enrollment = mitxonline.factories.enrollment.courseEnrollment({ + certificate: null, + grades: [], + run: { + start_date: startDate, + end_date: endDate, + }, + }) + renderWithProviders( + , + ) + await expandAccordion() + const row = (await screen.findByText("Current run:")).closest("div") + expect(row).toHaveTextContent(`(${expected})`) + }, + ) + + test("a past sibling run announces that it has ended", async () => { + const enrollment = makeEnrollment({ + start_date: moment().subtract(30, "days").toISOString(), + end_date: moment().add(30, "days").toISOString(), + }) + const pastSibling = makeEnrollment({ + start_date: moment().subtract(400, "days").toISOString(), + end_date: moment().subtract(300, "days").toISOString(), + }) + renderWithProviders( + , + ) + await expandAccordion() + // The row shows only a date range visually; the expired icon is + // aria-hidden, so the status has to reach screen readers some other way. + expect(await screen.findByText("Ended")).toBeInTheDocument() + expect( + screen.getByRole("link", { name: /View content for .*\(Ended\)/ }), + ).toBeInTheDocument() + }) + test("each sibling with a courseware URL shows a 'View content' link after expanding", async () => { const urlA = faker.internet.url() const urlB = faker.internet.url() diff --git a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.tsx b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.tsx index 9b043b3ce3..1d6722da3c 100644 --- a/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.tsx +++ b/frontends/main/src/app-pages/DashboardPage/CoursewareDisplay/SiblingRunsAccordion.tsx @@ -11,7 +11,10 @@ import { RiSubtractLine, RiTimeLine, } from "@remixicon/react" -import { isInPast, formatDate } from "ol-utilities" +import { formatDate } from "ol-utilities" +import { getRunTimeState } from "./courseDateUtils" +import type { RunTimeState } from "./courseDateUtils" +import { VisuallyHidden } from "@mitodl/smoot-design" import { EnrollmentStatusIcon } from "./EnrollmentStatus" import NextLink from "next/link" import { CourseRunEnrollmentV3 } from "@mitodl/mitxonline-api-axios/v2" @@ -122,8 +125,21 @@ const formatRunDateRange = ( return parts.join(" – ") } -const getRunStatusLabel = (status: EnrollmentStatus): string => { +/** + * Every state gets words, not just "In Progress". The run icons are + * `aria-hidden`, so anything conveyed only by an icon is silent to a screen + * reader; this label is the sole status text on the row. + * + * "Upcoming" rather than the badge's "Not Started" because the sibling rows in + * this same list already prefix future runs with "Upcoming:". + */ +const getRunStatusLabel = ( + status: EnrollmentStatus, + timeState: RunTimeState, +): string => { if (status === EnrollmentStatus.Completed) return "Completed" + if (timeState === "upcoming") return "Upcoming" + if (timeState === "ended") return "Ended" if (status === EnrollmentStatus.Enrolled) return "In Progress" return "" } @@ -240,7 +256,11 @@ const SiblingRunsPanel: React.FC = ({ type: DashboardType.CourseRunEnrollment, data: enrollment, }) - const currentStatusLabel = getRunStatusLabel(currentStatus) + const currentTimeState = getRunTimeState( + currentRun?.start_date, + currentRun?.end_date, + ) + const currentStatusLabel = getRunStatusLabel(currentStatus, currentTimeState) const currentDateRange = formatRunDateRange( currentRun?.start_date, currentRun?.end_date, @@ -262,7 +282,17 @@ const SiblingRunsPanel: React.FC = ({ } + icon={ + currentStatus === EnrollmentStatus.Completed ? ( + + ) : currentTimeState === "upcoming" ? ( +