diff --git a/docs/cognito-google-setup.md b/docs/cognito-google-setup.md index 7831401..2fa8aca 100644 --- a/docs/cognito-google-setup.md +++ b/docs/cognito-google-setup.md @@ -109,19 +109,33 @@ sign-in page. The style is harmless on the classic UI, so styles go first: 3. After apply: open an app's sign-in (or run the probe below) and confirm the managed-login page or a straight redirect to the IdP, not an error page. -Probe (unauthenticated GET, no credentials; use any internal client ID and one of its callback URLs — -cupcake's are in `/javabin/apps/cupcake/cupcake/{cognito-client-id,auth}`): +Probe: the platform keeps a probe client in the internal pool, `platform-idp-probe`, that lists only +`internal_idp_probe_identity_providers` (identity module, default `["Google"]`; created once the +Google IdP exists). It is an ordinary app-auth-sync request written by the identity module +(`/javabin/apps/platform/idp-probe/auth`; only `platform` requests may set `identity_providers`), so it +gets a managed-login branding style like every app client. It cannot sign anyone in: public client, +callback `https://localhost/idp-probe`, and allowed group `idp-probe-nobody`, which does not exist, so +auth-claims refuses every token (its auth-status lists that group under `unknown_groups`, as expected). + +The probe is an unauthenticated GET; it only reads the redirect Cognito returns (the client ID is not +secret): ```bash DOMAIN=javabin-internal.auth.eu-central-1.amazoncognito.com -curl -s -o /dev/null -w '%{redirect_url}\n' \ - "https://$DOMAIN/oauth2/authorize?response_type=code&client_id=&redirect_uri=&scope=openid&identity_provider=Google&prompt=select_account" \ - | grep -q 'prompt=select_account' && echo OK || echo "prompt dropped" +CID=$(aws ssm get-parameter --name /javabin/apps/platform/idp-probe/cognito-client-id --query Parameter.Value --output text) +Q="response_type=code&client_id=$CID&redirect_uri=https://localhost/idp-probe&scope=openid+email" +for extra in "&prompt=select_account" "&prompt=select_account&identity_provider=Google" ""; do + loc=$(curl -s -o /dev/null -w '%{redirect_url}' "https://$DOMAIN/oauth2/authorize?$Q$extra") + echo "${extra:-}: host=$(echo "$loc" | cut -d/ -f3)" \ + "prompt=$(echo "$loc" | grep -o 'prompt=[^&]*' || echo -)" "hd=$(echo "$loc" | grep -o 'hd=[^&]*' || echo -)" +done ``` -`identity_provider=Google` works only for a client that lists `Google`, so before the cutover this -needs a client with `Google` (or run it right after cutover). `scripts/auth-doctor.py` sends the pool's -`COGNITO_EXTRA_AUTH_PARAMS` the same way and warns when Google is not asked for the account chooser. +Expected on managed login: the first two go to `accounts.google.com` with `prompt=select_account`; the +last (an app that sends no `prompt`) has none. To probe another IdP (e.g. a replacement OIDC provider), +point `internal_idp_probe_identity_providers` at it and use that name in `identity_provider=`. +For app clients, `scripts/auth-doctor.py` sends the pool's `COGNITO_EXTRA_AUTH_PARAMS` the same way and +warns when Google is not asked for the account chooser. If `prompt` still does not reach Google on managed login, the Google-type provider is the problem: replace it with an `OIDC`-type provider for `https://accounts.google.com` (Cognito documents forwarding `prompt` for OIDC IdPs). diff --git a/docs/lambda-functions.md b/docs/lambda-functions.md index 403eb6c..7e53838 100644 --- a/docs/lambda-functions.md +++ b/docs/lambda-functions.md @@ -175,7 +175,7 @@ Idempotent: skips resources that already have a `created-by` tag (preserves orig **Managed-login branding:** after the credentials, every client gets a managed-login style with Cognito's defaults (`CreateManagedLoginBranding`, `UseCognitoProvidedValues=True`), only if `DescribeManagedLoginBrandingByClient` finds none, so a style customised in the console is never changed. A pool domain on managed login (`ManagedLoginVersion` 2) has no sign-in page for a client without a style; on the classic hosted UI the style is unused. Deleting a client (request gone, pool switch, `client_type` change, orphan) deletes its style first. Reconcile backfills styles for existing clients. auth-status `branding`: `created` / `exists`. A failure here comes after the credentials are written but still makes the sync `error` (alarm). -**Identity providers:** every client in a pool gets exactly the pool's list, from `INTERNAL_IDENTITY_PROVIDERS` / `EXTERNAL_IDENTITY_PROVIDERS` (comma-separated; set from the identity module's `internal_identity_providers` = `["GoogleSAML"]` and `external_identity_providers` = `["COGNITO"]`). With a single IdP the hosted UI redirects straight to it, so internal apps show no password form or sign-up link. Reconcile overwrites console edits to the list (reported as drift). If a configured IdP does not exist in the pool, the request is skipped with an `ERROR` log and status `error` (reconcile lists it under `error`): nothing is created, moved or changed, and the existing client keeps working. Internal-pool requests asking for `aws.cognito.signin.user.admin` are rejected as invalid (that scope would let a user rewrite the email auth-claims trusts). +**Identity providers:** every client in a pool gets exactly the pool's list, from `INTERNAL_IDENTITY_PROVIDERS` / `EXTERNAL_IDENTITY_PROVIDERS` (comma-separated; set from the identity module's `internal_identity_providers` = `["GoogleSAML"]` and `external_identity_providers` = `["COGNITO"]`). With a single IdP the hosted UI redirects straight to it, so internal apps show no password form or sign-up link. Reconcile overwrites console edits to the list (reported as drift). If a configured IdP does not exist in the pool, the request is skipped with an `ERROR` log and status `error` (reconcile lists it under `error`): nothing is created, moved or changed, and the existing client keeps working. Platform-owned requests (`/javabin/apps/platform/...`; `platform` is never a registered team) may set `identity_providers` to override the list — the IdP probe client (`platform-idp-probe`, see cognito-google-setup.md) uses it; each IdP must exist in the pool and the internal pool never allows `COGNITO`; the field in any other team's request is invalid. Internal-pool requests asking for `aws.cognito.signin.user.admin` are rejected as invalid (that scope would let a user rewrite the email auth-claims trusts). **Errors:** reconcile syncs every app even if one fails, and returns normally (raising would make the async EventBridge/CI invoke retry the whole run twice). The CloudWatch alarm `javabin-app-auth-sync-skipped` (metric filter on `client left unchanged` or `Sync failed for`) posts to `javabin-alerts` for a skipped request or a failed app. A failing change event still raises (Lambda `Errors`). diff --git a/terraform/lambda-src/app_auth_sync/handler.py b/terraform/lambda-src/app_auth_sync/handler.py index 266657f..9a9a455 100644 --- a/terraform/lambda-src/app_auth_sync/handler.py +++ b/terraform/lambda-src/app_auth_sync/handler.py @@ -32,6 +32,14 @@ IdP does not exist in the pool, the request is skipped and logged as an error (status "error"); the existing client is left as it is. +Platform-owned requests (team PLATFORM_TEAM, "platform": no registered team +has that name, so only the platform's own Terraform can write under +/{project}/apps/platform/) may set "identity_providers" to override the +pool's list, e.g. the IdP probe client (terraform/platform/identity/) that +lists only an IdP no app uses yet. Every listed IdP must exist in the pool; +the internal pool never allows COGNITO. Any other team's request with the +field is invalid. + An app can also request extra clients in a slot, one level down: /{project}/apps/{team}/{app}/login-N/auth (service-routing writes these for app.yaml routing.login, one per ALB sign-in rule). The client is then named @@ -85,6 +93,10 @@ def _idp_list(value): "external": _idp_list(os.environ.get("EXTERNAL_IDENTITY_PROVIDERS", "COGNITO")), } MEMBERSHIPS_TABLE = os.environ.get("MEMBERSHIPS_TABLE", "") +# The only owner whose requests may override the pool's IdP list +PLATFORM_TEAM = "platform" +# Pools whose clients must never offer local (COGNITO) sign-in +POOL_FORBIDDEN_IDPS = {"internal": {"COGNITO"}} # Hosted-UI domains, reported in auth-status for the doctor's probes POOL_DOMAINS = { "internal": os.environ.get("INTERNAL_POOL_DOMAIN", ""), @@ -147,8 +159,9 @@ def client_name(team, app, slot=""): return f"{team}-{app}.{slot}" if slot else f"{team}-{app}" -def parse_request(raw): - """Validate the JSON written by the cognito-app-client module.""" +def parse_request(raw, team=None): + """Validate the JSON written by the cognito-app-client module (or, for + team PLATFORM_TEAM, by the platform's own Terraform).""" try: req = json.loads(raw) except (TypeError, ValueError) as e: @@ -188,8 +201,19 @@ def urls(key, required): if not isinstance(groups, list) or not all(isinstance(g, str) and g for g in groups): raise InvalidRequest("allowed_groups must be a list of group names") + idps = req.get("identity_providers") + if idps is not None: + if team != PLATFORM_TEAM: + raise InvalidRequest("identity_providers can only be set by the platform") + if not isinstance(idps, list) or not idps or not all(isinstance(p, str) and p for p in idps): + raise InvalidRequest("identity_providers must be a non-empty list of IdP names") + forbidden = set(idps) & POOL_FORBIDDEN_IDPS.get(pool, set()) + if forbidden: + raise InvalidRequest(f"identity_providers {sorted(forbidden)} are not allowed in the {pool} pool") + return { "pool": pool, + "identity_providers": list(dict.fromkeys(idps)) if idps else None, "client_type": client_type, "callback_urls": urls("callback_urls", required=True), "logout_urls": urls("logout_urls", required=False), @@ -228,13 +252,14 @@ def find_client(user_pool_id, client_name): _idp_cache = {} -def identity_providers(pool, user_pool_id): - """The pool's configured sign-in IdPs, after checking each one exists. +def identity_providers(pool, user_pool_id, override=None): + """The pool's configured sign-in IdPs (or a platform request's override), + after checking each one exists. With exactly one IdP on a client, the hosted UI redirects straight to it, so apps need no identity_provider parameter. """ - wanted = POOL_IDPS[pool] + wanted = override or POOL_IDPS[pool] if not wanted: raise PoolMisconfigured(f"no identity providers configured for the {pool} pool") if user_pool_id not in _idp_cache: @@ -299,7 +324,7 @@ def client_settings(pool, user_pool_id, client_name, req): "AllowedOAuthScopes": req["scopes"], "CallbackURLs": req["callback_urls"], "LogoutURLs": req["logout_urls"], - "SupportedIdentityProviders": identity_providers(pool, user_pool_id), + "SupportedIdentityProviders": identity_providers(pool, user_pool_id, req.get("identity_providers")), "ExplicitAuthFlows": ["ALLOW_REFRESH_TOKEN_AUTH", "ALLOW_USER_SRP_AUTH"], "PreventUserExistenceErrors": "ENABLED", } @@ -571,7 +596,7 @@ def sync_and_report(team, app, raw, version, reason, slot=""): """ name = client_name(team, app, slot) try: - status = sync(team, app, parse_request(raw), reason, slot) + status = sync(team, app, parse_request(raw, team), reason, slot) except InvalidRequest as e: logger.error("Invalid auth request for %s: %s", name, e) status = {"status": "invalid", "error": str(e)} diff --git a/terraform/lambda-src/tests/test_app_auth_sync.py b/terraform/lambda-src/tests/test_app_auth_sync.py index 774e3e3..e87e8a9 100644 --- a/terraform/lambda-src/tests/test_app_auth_sync.py +++ b/terraform/lambda-src/tests/test_app_auth_sync.py @@ -365,6 +365,59 @@ def test_reconcile_reports_and_keeps_skipped_clients(self): self.assertEqual(self.ssm.params[self.ID_PARAM], self.cid) +class PlatformIdentityProviderOverrideTest(unittest.TestCase): + """Platform-owned requests (the IdP probe) may pick their own IdPs.""" + PROBE = "/javabin/apps/platform/idp-probe/auth" + PROBE_REQUEST = {"pool": "internal", "client_type": "public", "callback_urls": ["https://localhost/idp-probe"], + "scopes": ["openid", "email"], "allowed_groups": ["idp-probe-nobody"], + "identity_providers": ["Google"]} + + def setUp(self): + self.ssm, self.cognito, self.dynamo = FakeSSM(), FakeCognito(internal_idps=("GoogleSAML", "Google")), FakeDynamo() + self.h = load_handler(self.ssm, self.cognito, self.dynamo) + + def sync(self, req, name=PROBE): + self.ssm.params[name] = json.dumps(req) + return self.h.handler(event(name, "Create"), None) + + def test_platform_request_gets_only_its_own_idps_and_a_branding_style(self): + result = self.sync(self.PROBE_REQUEST) + self.assertEqual(result["status"], "synced") + cid, client = next(iter(self.cognito.clients.items())) + self.assertEqual(client["ClientName"], "platform-idp-probe") + self.assertEqual(client["SupportedIdentityProviders"], ["Google"]) + self.assertNotIn("ClientSecret", client) + self.assertEqual(self.cognito.styles(), {cid: True}) + self.assertEqual(self.ssm.params["/javabin/apps/platform/idp-probe/cognito-client-identity-provider"], "Google") + status = json.loads(self.ssm.params["/javabin/apps/platform/idp-probe/auth-status"]) + self.assertEqual(status["identity_providers"], ["Google"]) + + def test_app_clients_keep_the_pool_list(self): + self.sync(self.PROBE_REQUEST) + self.sync(REQUEST, REQ) + by_name = {c["ClientName"]: c for c in self.cognito.clients.values()} + self.assertEqual(by_name["cupcake-frosting"]["SupportedIdentityProviders"], ["GoogleSAML"]) + + def test_other_teams_cannot_override(self): + result = self.sync({**REQUEST, "identity_providers": ["Google"]}, REQ) + self.assertEqual(result["status"], "invalid") + self.assertIn("platform", result["error"]) + self.assertEqual(self.cognito.clients, {}) + + def test_override_must_exist_in_the_pool(self): + with self.assertLogs(level="ERROR"): + result = self.sync({**self.PROBE_REQUEST, "identity_providers": ["GoogleOIDC"]}) + self.assertEqual(result["status"], "error") + self.assertIn("GoogleOIDC", result["error"]) + self.assertEqual(self.cognito.clients, {}) + + def test_internal_pool_override_cannot_offer_local_sign_in(self): + for bad in (["COGNITO"], ["Google", "COGNITO"], [], "Google", [""]): + result = self.sync({**self.PROBE_REQUEST, "identity_providers": bad}) + self.assertEqual(result["status"], "invalid", bad) + self.assertEqual(self.cognito.clients, {}) + + class AuthFeaturesTest(AppAuthSyncTest): SECRET = "/javabin/apps/cupcake/frosting/cognito-client-secret" diff --git a/terraform/platform/identity/main.tf b/terraform/platform/identity/main.tf index 9838e8b..d88069f 100644 --- a/terraform/platform/identity/main.tf +++ b/terraform/platform/identity/main.tf @@ -134,6 +134,37 @@ resource "aws_cognito_user_pool_domain" "internal" { managed_login_version = var.internal_managed_login_version } +# IdP probe client — a platform-owned internal-pool client that lists only +# var.internal_idp_probe_identity_providers (default: Google), so the +# redirect managed login sends to an IdP can be checked (prompt=, hd=) before +# any app client is cut over to it. app-auth-sync creates it from this +# request like any app's (branding style included); only platform requests +# may set identity_providers. It can never sign anyone in usefully: public +# client, a localhost callback, and an allowed group that does not exist, so +# auth-claims refuses every token. Probe: docs/cognito-google-setup.md. +locals { + idp_probe_enabled = length(var.internal_idp_probe_identity_providers) > 0 && ( + !contains(var.internal_idp_probe_identity_providers, "Google") || local.google_internal_ready + ) +} + +resource "aws_ssm_parameter" "idp_probe_auth" { + count = local.idp_probe_enabled ? 1 : 0 + + name = "/${var.project}/apps/platform/idp-probe/auth" + description = "IdP probe client request for app-auth-sync (platform-owned)" + type = "String" + value = jsonencode({ + pool = "internal" + client_type = "public" + callback_urls = ["https://localhost/idp-probe"] + logout_urls = [] + scopes = ["openid", "email"] + allowed_groups = ["idp-probe-nobody"] + identity_providers = var.internal_idp_probe_identity_providers + }) +} + # Cognito groups are managed by the team provisioner (from groups.yaml and # team YAMLs), not by Terraform. The provisioner creates them dynamically. diff --git a/terraform/platform/identity/variables.tf b/terraform/platform/identity/variables.tf index 77ebbb6..aaf7afd 100644 --- a/terraform/platform/identity/variables.tf +++ b/terraform/platform/identity/variables.tf @@ -29,6 +29,19 @@ variable "internal_managed_login_version" { } } +# Empty disables the probe client. A list naming Google waits for the internal +# Google IdP (its OAuth client in SSM). +variable "internal_idp_probe_identity_providers" { + description = "IdPs on the platform's internal-pool probe client (platform-idp-probe); [] removes it" + type = list(string) + default = ["Google"] + + validation { + condition = !contains(var.internal_idp_probe_identity_providers, "COGNITO") + error_message = "The internal pool must not offer COGNITO (local sign-up)." + } +} + variable "google_client_id" { description = "Google OAuth 2.0 client ID for the external pool's Google IdP (from GCP console)" type = string