diff --git a/docs/waves/wave-10.md b/docs/waves/wave-10.md index 2dba494..83afc8c 100644 --- a/docs/waves/wave-10.md +++ b/docs/waves/wave-10.md @@ -157,10 +157,19 @@ afterwards. `auth.find_user` (already case-insensitive across username **and** email). If it exists, update only `last_login_at` and — if empty locally — `full_name` and `email` from the directory. **Never write `role`.** If it does not exist, create it at -`ROLE_PROJECT_USER` with `full_name`/`email` from the directory, honouring the existing -`auto_add_projects` machinery so a JIT account lands in the right projects. Note the -flush-order warning in the `models.py` docstring: `create_user` in `app.py` already handles -this correctly for account + `ProjectMember` rows in one flush — follow it. +`ROLE_PROJECT_USER` with `full_name`/`email` from the directory. + +**A JIT account gets NO project access, and that is deliberate.** An earlier draft of this +task said to honour the `auto_add_projects` machinery so a new account "lands in the right +projects" — that was wrong about how the flag works. `auto_add_projects` is evaluated when a +**project** is created (`app.py:245`), marking accounts that should join every *new* job; it +cannot retroactively add a new account to existing ones. There is no correct default, so +least privilege applies: the account exists, can sign in, and sees nothing until someone +grants access. That is a real UX cliff — a successful sign-in into an empty app — so it has +to be visible to admins rather than silent, which is what the `AuditLog` row is for. + +Note the flush-order warning in the `models.py` docstring: `create_user` in `app.py` handles +account-then-membership correctly in one flush — follow it if you add rows. Write an `AuditLog` row for each JIT creation. An account appearing without an administrator creating it is exactly the kind of event that record exists for. @@ -171,7 +180,8 @@ creating it is exactly the kind of event that record exists for. - [ ] `full_name` and `email` are populated from the directory on creation - [ ] an existing `admin` signing in is still `admin` afterwards — asserted, not assumed - [ ] an existing account with a locally-set `full_name` does not have it overwritten -- [ ] a JIT account with `auto_add_projects` peers gets its `ProjectMember` rows +- [ ] a JIT account has NO `ProjectMember` rows and sees no projects +- [ ] the new account appears in the Admin console user list so access can be granted - [ ] each JIT creation writes an `AuditLog` row - [ ] a failed bind creates **no** row - [ ] a bind that succeeds but fails the group check creates **no** row diff --git a/server/app.py b/server/app.py index c37bfbd..4359cc2 100644 --- a/server/app.py +++ b/server/app.py @@ -727,6 +727,55 @@ def _clear_bind_failures(sam: str) -> None: _bind_attempts.pop((sam or "").strip().lower(), None) +def _match_directory_account(db: Session, result) -> Optional[models.User]: + """Find the local row for a directory identity — D13 criterion 4. + + Matched on `sAMAccountName` OR the directory's `mail`, because existing accounts + were created by hand with `manage_users.py` and some were typed as short logon + names while others were typed as email addresses. Matching on both is what keeps + an existing admin's role instead of handing them a second, default-role account. + + `auth.find_user` already compares case-insensitively against username AND email, + so each call covers two columns; the second call is for the case where the local + username is the person's address and the directory only told us their sAMAccountName. + """ + user = auth.find_user(db, result.sam) + if user is None and result.mail: + user = auth.find_user(db, result.mail) + if user is not None: + log.info("matched directory identity %r to existing local account %r by mail", + result.sam, user.username) + return user + + +def _provision_from_directory(db: Session, result) -> models.User: + """Create a local account for someone who just authenticated and has no row. + + Lands at `project_user` with NO project memberships. That is least privilege and + it is deliberate, but it means the person signs in successfully into an empty + app until an admin grants access — so it is written to the audit log rather than + happening silently. `auto_add_projects` cannot help here: it is evaluated when a + PROJECT is created, to mark who joins every new job, and cannot retroactively add + a new account to jobs that already exist. + """ + u = models.User( + id=gen_id("user"), + username=result.sam, + email=result.mail or "", + full_name=result.full_name or "", + role=auth.ROLE_PROJECT_USER, + ) + db.add(u) + db.flush() # see the flush-order note in models.py's docstring + log_event(db, u.username, "user_provisioned", "user", u.id, summary=u.username, + detail={"source": "directory", "role": u.role, "upn": result.upn, + "projects": 0, "note": "created on first successful sign-in"}) + log.info("provisioned local account %r from the directory at role %r with no " + "project access — an admin must grant access before they see anything", + u.username, u.role) + return u + + @app.post("/api/auth/login") def login(body: LoginIn, request: Request, response: Response, db: Session = Depends(get_db)): """Authenticate against the domain (D13 / T10.2) and set the session cookie. @@ -803,19 +852,37 @@ def login(body: LoginIn, request: Request, response: Response, db: Session = Dep # ── authenticated ───────────────────────────────────────────────────────── _clear_bind_failures(sam) - if not user: - # T10.4 provisions the account here. Until that lands, an authenticated - # person with no local row is refused rather than silently admitted. - log.warning("%r authenticated against the domain but has no local account " - "(JIT provisioning arrives in T10.4)", result.sam) - raise HTTPException(status_code=403, detail="No account on this system yet.") + + # The pre-bind lookup used the typed name only. Now that the directory has told + # us the account's real sAMAccountName and mail, re-match on both (T10.4). + if user is None: + user = _match_directory_account(db, result) + + provisioned = user is None + if provisioned: + user = _provision_from_directory(db, result) + else: + # NEVER touch `role` here. An existing admin stays an admin — that is D13 + # criterion 4, and it is the whole reason this branch is separate from the + # one above. Fill in identity fields only where they are empty locally, so a + # name deliberately set in the console is not overwritten by the directory. + if not user.full_name and result.full_name: + user.full_name = result.full_name + if not user.email and result.mail: + user.email = result.mail + if not user.is_active: + # Checked after provisioning so a brand-new account (is_active defaults True) + # is not caught by it, and after the role branch so a disabled admin is still + # refused. Local state overrides the directory: disabling here is how you + # revoke access to THIS app without touching the domain account. raise HTTPException(status_code=403, detail="Account is disabled") user.failed_attempts = 0 user.locked_until = None user.last_login_at = now db.commit() + db.refresh(user) token = auth.create_token(user) auth.set_session_cookie(response, request, token) return {"user": user.to_dict()}