From 570e2c5fc3521c7c860e87b542849aa8161790e9 Mon Sep 17 00:00:00 2001 From: Ashwin Ramachandran Date: Mon, 10 Aug 2026 18:21:17 +0530 Subject: [PATCH] An administrator can give a colleague a new password MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The setup link works exactly once — claiming flips the account out of `pending`, so every later use is refused. An administrator could therefore create a colleague's account and never help them back into it, and with no mail path there is no self-service reset either. A forgotten password had no route short of editing the database by hand. `/claim` gains a second purpose. `setup` still means a pending account choosing its first password; `reset` means a claimed account being given a new one because an administrator asked. The two are strictly disjoint — matched against the state of the row, not trusted from the token — so a setup link still cannot touch a claimed account. **Single-use needs its own mechanism here.** A setup token retires itself because claiming flips the account out of pending; a reset leaves the account exactly as claimed as it was, so nothing about the row changes and the link would work forever. The token carries a one-way marker of the credential it was minted against, and the credential is a condition of the UPDATE — so the first successful reset retires it, a password change by any other route retires it, and two simultaneous posts cannot both win. A reset never gives a password to a provider-bound identity, never revives a switched-off account, and revokes that principal's refresh tokens. What it does NOT end is written down rather than implied: the /admin cookie is a stateless 12h JWT nothing here revokes, an outstanding authorization code survives its short window, and a live access token lasts until it expires. Every guard is mutation-checked: removing any one turns a test red. That bar is what found the real defects, and there were several. The handler checked less than the store, so a token for a provider-bound account got a 200 and a password form while only the write refused — a state oracle, and, because such a token could never spend, an unauthenticated argon2 lever measured at 45x a rejected token on an endpoint with no rate limit. Argon2 also ran inline on the event loop in an async handler and now goes through `run_blocking`. A passwordless row fingerprinted to sha256("") — one public constant — and now raises. The password write and the session revocation share one transaction. It also found two tests passing for the wrong reason: an ordering test that returned before reaching the code it named, and a wrong-purpose check that had migrated onto a verifier the handler no longer called, so an `admin_session` JWT could have acted as a setup link with the suite green. Review of the review: the active-status constant was a second copy of the string under a comment claiming it was shared, which is the same drift class this change exists to fix. It is public and shared now, proved by changing it to "live" and watching every test still pass. Squashed to one commit so no commit on this branch carries the credential-shaped test literal the secret scan flagged; it is an English phrase in a named constant now, the convention `test_member_onboarding.py` already states. Spec: ACE-108 --- dev/render_previews.py | 435 ++++++++++++++++++------ packages/agami-core/src/oauth_server.py | 26 ++ packages/agami-core/src/onboarding.py | 297 +++++++++++++--- packages/agami-core/src/user_store.py | 85 ++++- tests/test_onboarding.py | 388 ++++++++++++++++++++- 5 files changed, 1057 insertions(+), 174 deletions(-) diff --git a/dev/render_previews.py b/dev/render_previews.py index 92924f9b..606aa26a 100644 --- a/dev/render_previews.py +++ b/dev/render_previews.py @@ -31,16 +31,51 @@ } ADMIN_EMAIL = "you@example.com" USERS = [ - {"username": ADMIN_EMAIL, "first_name": "Alex", "last_name": "Kim", "email": ADMIN_EMAIL, - "status": "active", "oidc_provider": None, "has_password": True}, - {"username": "jordan@example.com", "first_name": "Jordan", "last_name": "Lee", - "email": "jordan@example.com", "status": "active", "oidc_provider": "google", "has_password": False}, - {"username": "sam@example.com", "first_name": "Sam", "last_name": "Okafor", - "email": "sam@example.com", "status": "active", "oidc_provider": "microsoft", "has_password": False}, - {"username": "riley@example.com", "first_name": "Riley", "last_name": "Chen", - "email": "riley@example.com", "status": "disabled", "oidc_provider": None, "has_password": False}, - {"username": "morgan@example.com", "first_name": "Morgan", "last_name": "Diaz", - "email": "morgan@example.com", "status": "active", "oidc_provider": None, "has_password": False}, + { + "username": ADMIN_EMAIL, + "first_name": "Alex", + "last_name": "Kim", + "email": ADMIN_EMAIL, + "status": "active", + "oidc_provider": None, + "has_password": True, + }, + { + "username": "jordan@example.com", + "first_name": "Jordan", + "last_name": "Lee", + "email": "jordan@example.com", + "status": "active", + "oidc_provider": "google", + "has_password": False, + }, + { + "username": "sam@example.com", + "first_name": "Sam", + "last_name": "Okafor", + "email": "sam@example.com", + "status": "active", + "oidc_provider": "microsoft", + "has_password": False, + }, + { + "username": "riley@example.com", + "first_name": "Riley", + "last_name": "Chen", + "email": "riley@example.com", + "status": "disabled", + "oidc_provider": None, + "has_password": False, + }, + { + "username": "morgan@example.com", + "first_name": "Morgan", + "last_name": "Diaz", + "email": "morgan@example.com", + "status": "active", + "oidc_provider": None, + "has_password": False, + }, ] @@ -52,11 +87,16 @@ def write(name: str, html: str) -> None: ADMIN = {"admin_username": ADMIN_EMAIL, "admin_label": "Alex Kim", "admin_email": ADMIN_EMAIL} CHROME = {"admin_label": "Alex Kim", "admin_email": ADMIN_EMAIL} -write("01-login.html", oauth_server.login_body_html(OAUTH, providers=("google", "microsoft"), wrap=True)) +write( + "01-login.html", + oauth_server.login_body_html(OAUTH, providers=("google", "microsoft"), wrap=True), +) write("02-login-password-only.html", oauth_server.login_body_html(OAUTH, wrap=True)) write( "03-login-error.html", - oauth_server.login_body_html(OAUTH, error="Invalid email or password.", providers=("google", "microsoft"), wrap=True), + oauth_server.login_body_html( + OAUTH, error="Invalid email or password.", providers=("google", "microsoft"), wrap=True + ), ) write("04-admin-login.html", admin.admin_login_body_html(provider="google")) # In a password deployment the roster shows a copy-able setup link per pending user. @@ -65,8 +105,10 @@ def write(name: str, html: str) -> None: for u in USERS if onboarding.is_pending(u) } -write("05-admin-users.html", - admin.users_tab_html(USERS, csrf="t0ken", ok="User added.", setup_links=SETUP_LINKS, **ADMIN)) +write( + "05-admin-users.html", + admin.users_tab_html(USERS, csrf="t0ken", ok="User added.", setup_links=SETUP_LINKS, **ADMIN), +) write("06-admin-dashboard.html", admin.dashboard_tab_html(**CHROME)) # The activity view — rendered from the REAL builders + read helpers over a temp store (no drift). @@ -86,38 +128,116 @@ def write(name: str, html: str) -> None: _SAMPLE_CALLS = [ # One conversation (thread t1), two turns — and EVERY call folds in, not just the execute_sql ones: # turn c1 scopes the datasource (list_datasources), turn c2 answers a question (schema + 2 queries). - dict(ts="2026-06-27T10:40:50Z", tool_name="list_datasources", source="mcp_server", - actor="jordan@example.com", execution_ms=3, success=True, - user_question="What datasources can I ask about?", thread_id="t1", correlation_id="c1"), - dict(ts="2026-06-27T10:41:05Z", tool_name="get_datasource_schema", source="mcp_server", - actor="jordan@example.com", datasource="SALES_DATA", execution_ms=12, success=True, - user_question="What's our revenue by region this quarter?", thread_id="t1", correlation_id="c2"), - dict(ts="2026-06-27T10:42:17Z", tool_name="execute_sql", source="mcp_server", actor="jordan@example.com", - datasource="SALES_DATA", sql="SELECT region, SUM(amount) AS revenue\nFROM orders\nGROUP BY region\nORDER BY revenue DESC", - row_count=5, execution_ms=84, success=True, user_question="What's our revenue by region this quarter?", - agent_query="revenue by region", thread_id="t1", correlation_id="c2"), - dict(ts="2026-06-27T10:42:41Z", tool_name="execute_sql", source="mcp_server", actor="jordan@example.com", - datasource="SALES_DATA", sql="SELECT date_trunc('month', placed_at) AS month, SUM(amount)\nFROM orders\nWHERE region = 'West'\nGROUP BY 1\nORDER BY 1", - row_count=3, execution_ms=61, success=True, user_question="What's our revenue by region this quarter?", - agent_query="monthly trend for the top region (West)", thread_id="t1", correlation_id="c2"), + dict( + ts="2026-06-27T10:40:50Z", + tool_name="list_datasources", + source="mcp_server", + actor="jordan@example.com", + execution_ms=3, + success=True, + user_question="What datasources can I ask about?", + thread_id="t1", + correlation_id="c1", + ), + dict( + ts="2026-06-27T10:41:05Z", + tool_name="get_datasource_schema", + source="mcp_server", + actor="jordan@example.com", + datasource="SALES_DATA", + execution_ms=12, + success=True, + user_question="What's our revenue by region this quarter?", + thread_id="t1", + correlation_id="c2", + ), + dict( + ts="2026-06-27T10:42:17Z", + tool_name="execute_sql", + source="mcp_server", + actor="jordan@example.com", + datasource="SALES_DATA", + sql="SELECT region, SUM(amount) AS revenue\nFROM orders\nGROUP BY region\nORDER BY revenue DESC", + row_count=5, + execution_ms=84, + success=True, + user_question="What's our revenue by region this quarter?", + agent_query="revenue by region", + thread_id="t1", + correlation_id="c2", + ), + dict( + ts="2026-06-27T10:42:41Z", + tool_name="execute_sql", + source="mcp_server", + actor="jordan@example.com", + datasource="SALES_DATA", + sql="SELECT date_trunc('month', placed_at) AS month, SUM(amount)\nFROM orders\nWHERE region = 'West'\nGROUP BY 1\nORDER BY 1", + row_count=3, + execution_ms=61, + success=True, + user_question="What's our revenue by region this quarter?", + agent_query="monthly trend for the top region (West)", + thread_id="t1", + correlation_id="c2", + ), # A separate one-query turn that errored (thread t2). - dict(ts="2026-06-27T10:41:50Z", tool_name="execute_sql", source="mcp_server", actor="sam@example.com", - datasource="SALES_DATA", sql="SELECT * FROM ordrs", execution_ms=31, success=False, - error_kind="syntax", user_question="how many orders today?", agent_query="count today's orders", - thread_id="t2", correlation_id="c3"), + dict( + ts="2026-06-27T10:41:50Z", + tool_name="execute_sql", + source="mcp_server", + actor="sam@example.com", + datasource="SALES_DATA", + sql="SELECT * FROM ordrs", + execution_ms=31, + success=False, + error_kind="syntax", + user_question="how many orders today?", + agent_query="count today's orders", + thread_id="t2", + correlation_id="c3", + ), # A cross-datasource turn (thread t3, one correlation): a question spanning two datasources runs one # execute_sql per datasource — the row lists both, and each call card shows its own. - dict(ts="2026-06-27T10:55:10Z", tool_name="execute_sql", source="mcp_server", actor="jordan@example.com", - datasource="SALES_DATA", sql="SELECT region, SUM(amount) AS revenue\nFROM orders GROUP BY region", - row_count=5, execution_ms=72, success=True, user_question="revenue vs open support tickets by region", - agent_query="revenue by region", thread_id="t3", correlation_id="c4"), - dict(ts="2026-06-27T10:55:31Z", tool_name="execute_sql", source="mcp_server", actor="jordan@example.com", - datasource="SUPPORT_DATA", sql="SELECT region, COUNT(*) AS open_tickets\nFROM tickets WHERE status='open' GROUP BY region", - row_count=5, execution_ms=58, success=True, user_question="revenue vs open support tickets by region", - agent_query="open tickets by region", thread_id="t3", correlation_id="c4"), + dict( + ts="2026-06-27T10:55:10Z", + tool_name="execute_sql", + source="mcp_server", + actor="jordan@example.com", + datasource="SALES_DATA", + sql="SELECT region, SUM(amount) AS revenue\nFROM orders GROUP BY region", + row_count=5, + execution_ms=72, + success=True, + user_question="revenue vs open support tickets by region", + agent_query="revenue by region", + thread_id="t3", + correlation_id="c4", + ), + dict( + ts="2026-06-27T10:55:31Z", + tool_name="execute_sql", + source="mcp_server", + actor="jordan@example.com", + datasource="SUPPORT_DATA", + sql="SELECT region, COUNT(*) AS open_tickets\nFROM tickets WHERE status='open' GROUP BY region", + row_count=5, + execution_ms=58, + success=True, + user_question="revenue vs open support tickets by region", + agent_query="open tickets by region", + thread_id="t3", + correlation_id="c4", + ), # A bare call with no self-reported ids → its own singleton conversation (audit-complete degradation). - dict(ts="2026-06-27T10:40:03Z", tool_name="list_datasources", source="mcp_server", - actor="morgan@example.com", execution_ms=3, success=True), + dict( + ts="2026-06-27T10:40:03Z", + tool_name="list_datasources", + source="mcp_server", + actor="morgan@example.com", + execution_ms=3, + success=True, + ), ] for _c in _SAMPLE_CALLS: _sink.record_tool_call(ToolCallRecord(**_c)) @@ -134,37 +254,65 @@ def write(name: str, html: str) -> None: "fiscal_year_start_month": 1, "key_terminology": {"AOV": "average order value", "Net revenue": "sales minus refunds"}, "storage_connections": [ - {"name": "warehouse", "storage_type": "PostgreSQL", - "storage_config": {"host": "never-rendered"}} + { + "name": "warehouse", + "storage_type": "PostgreSQL", + "storage_config": {"host": "never-rendered"}, + } ], "subject_areas": [ { - "name": "Catalog", "description": "Products, pricing and stock.", + "name": "Catalog", + "description": "Products, pricing and stock.", "default_time_window": "last_90_days", - "tables": [{"storage_connection": "warehouse", "schema": "public", "table": "products"}], + "tables": [ + {"storage_connection": "warehouse", "schema": "public", "table": "products"} + ], "tables_defined": [ { - "name": "products", "schema": "public", "storage_connection": "warehouse", - "grain": ["id"], "description": "Master product catalog.", - "description_source": "ai_unvalidated", "confidence": "proposed", + "name": "products", + "schema": "public", + "storage_connection": "warehouse", + "grain": ["id"], + "description": "Master product catalog.", + "description_source": "ai_unvalidated", + "confidence": "proposed", "review_state": "unreviewed", - "caveats": ["\"Coffee\" = category='cafe' AND name ILIKE '%coffee%','%latte%'…"], + "caveats": [ + "\"Coffee\" = category='cafe' AND name ILIKE '%coffee%','%latte%'…" + ], "columns": [ {"name": "id", "type": "uuid", "primary_key": True}, {"name": "sku", "type": "string"}, {"name": "name", "type": "string"}, - {"name": "category", "type": "string", - "description": "Product type: 'book', 'cafe', 'merchandise', or 'other'.", - "description_source": "ai_unvalidated", - "choice_field": {"book": "Book", "cafe": "Cafe"}}, + { + "name": "category", + "type": "string", + "description": "Product type: 'book', 'cafe', 'merchandise', or 'other'.", + "description_source": "ai_unvalidated", + "choice_field": {"book": "Book", "cafe": "Cafe"}, + }, {"name": "price", "type": "decimal", "unit": "USD"}, - {"name": "cost_price", "type": "decimal", "unit": "USD", - "caveats": ["~20% of rows have cost_price > price (data-entry errors). " - "Treat as valid only when cost_price <= price."]}, - {"name": "supplier_email", "type": "string", "sensitive": True, - "description": "Distributor contact."}, - {"name": "category_id", "type": "uuid", - "foreign_key": {"table": "categories", "column": "id"}}, + { + "name": "cost_price", + "type": "decimal", + "unit": "USD", + "caveats": [ + "~20% of rows have cost_price > price (data-entry errors). " + "Treat as valid only when cost_price <= price." + ], + }, + { + "name": "supplier_email", + "type": "string", + "sensitive": True, + "description": "Distributor contact.", + }, + { + "name": "category_id", + "type": "uuid", + "foreign_key": {"table": "categories", "column": "id"}, + }, {"name": "stock_qty", "type": "integer"}, {"name": "is_active", "type": "boolean"}, {"name": "created_at", "type": "timestamp"}, @@ -175,17 +323,28 @@ def write(name: str, html: str) -> None: "performance_hints": {"estimated_row_count": 6591}, }, { - "name": "inventory", "schema": "public", "storage_connection": "warehouse", - "grain": ["id"], "description": "Per-location stock levels.", - "confidence": "confirmed", "review_state": "approved", - "column_groups": {"Identifiers": ["id", "product_id", "location_id"], - "Levels": ["on_hand", "reserved"]}, - "column_group_descriptions": {"Identifiers": "keys & references", - "Levels": "stock counts"}, + "name": "inventory", + "schema": "public", + "storage_connection": "warehouse", + "grain": ["id"], + "description": "Per-location stock levels.", + "confidence": "confirmed", + "review_state": "approved", + "column_groups": { + "Identifiers": ["id", "product_id", "location_id"], + "Levels": ["on_hand", "reserved"], + }, + "column_group_descriptions": { + "Identifiers": "keys & references", + "Levels": "stock counts", + }, "columns": [ {"name": "id", "type": "uuid", "primary_key": True}, - {"name": "product_id", "type": "uuid", - "foreign_key": {"table": "products", "column": "id"}}, + { + "name": "product_id", + "type": "uuid", + "foreign_key": {"table": "products", "column": "id"}, + }, {"name": "location_id", "type": "uuid"}, {"name": "on_hand", "type": "integer"}, {"name": "reserved", "type": "integer"}, @@ -195,32 +354,53 @@ def write(name: str, html: str) -> None: }, ], "metrics": [ - {"name": "active products", "calculation": "COUNT(*) FILTER (WHERE is_active)", - "other_names": ["live items"]} + { + "name": "active products", + "calculation": "COUNT(*) FILTER (WHERE is_active)", + "other_names": ["live items"], + } ], "entities": [ - {"name": "product", "other_names": ["sku", "item"], - "description": "A sellable catalog item.", "value_pattern": "SKU-[0-9]{6}", - "confidence": "inferred"} + { + "name": "product", + "other_names": ["sku", "item"], + "description": "A sellable catalog item.", + "value_pattern": "SKU-[0-9]{6}", + "confidence": "inferred", + } ], "relationships": [ - {"from_table": "products", "to_table": "categories", "from_column": "category_id", - "to_column": "id", "relationship": "many_to_one", "confidence": "confirmed", - "review_state": "approved"} + { + "from_table": "products", + "to_table": "categories", + "from_column": "category_id", + "to_column": "id", + "relationship": "many_to_one", + "confidence": "confirmed", + "review_state": "approved", + } ], }, { - "name": "Sales", "description": "Orders, line items and revenue.", + "name": "Sales", + "description": "Orders, line items and revenue.", "tables": [{"storage_connection": "warehouse", "schema": "public", "table": "orders"}], "tables_defined": [ { - "name": "orders", "schema": "public", "storage_connection": "warehouse", - "grain": ["id"], "description": "One row per placed order.", - "confidence": "confirmed", "review_state": "approved", + "name": "orders", + "schema": "public", + "storage_connection": "warehouse", + "grain": ["id"], + "description": "One row per placed order.", + "confidence": "confirmed", + "review_state": "approved", "columns": [ {"name": "id", "type": "integer", "primary_key": True}, - {"name": "customer_id", "type": "integer", - "foreign_key": {"table": "customers", "column": "id"}}, + { + "name": "customer_id", + "type": "integer", + "foreign_key": {"table": "customers", "column": "id"}, + }, {"name": "amount", "type": "decimal", "unit": "USD"}, {"name": "placed_at", "type": "timestamp"}, ], @@ -228,8 +408,13 @@ def write(name: str, html: str) -> None: } ], "metrics": [ - {"name": "revenue", "calculation": "SUM(amount)", "unit": "USD", - "other_names": ["sales"], "source_tables": ["orders"]} + { + "name": "revenue", + "calculation": "SUM(amount)", + "unit": "USD", + "other_names": ["sales"], + "source_tables": ["orders"], + } ], "entities": [], "relationships": [], @@ -237,15 +422,34 @@ def write(name: str, html: str) -> None: ], # Cross-area joins (Sales → Catalog) — surfaced in the Relationships browse view, grouped by pair. "cross_subject_area_relationships": [ - {"from_table": "orders", "to_table": "products", "from_column": "product_id", - "to_column": "id", "from_schema": "public", "to_schema": "public", "join_type": "LEFT", - "relationship": "many_to_one", "confidence": "inferred", "review_state": "unreviewed", - "from_subject_area": "Sales", "to_subject_area": "Catalog"}, - {"from_table": "orders", "to_table": "inventory", "from_column": "warehouse_id", - "to_column": "location_id", "from_schema": "public", "to_schema": "public", - "join_type": "LEFT", "relationship": "many_to_one", "confidence": "proposed", - "review_state": "unreviewed", - "from_subject_area": "Sales", "to_subject_area": "Catalog"}, + { + "from_table": "orders", + "to_table": "products", + "from_column": "product_id", + "to_column": "id", + "from_schema": "public", + "to_schema": "public", + "join_type": "LEFT", + "relationship": "many_to_one", + "confidence": "inferred", + "review_state": "unreviewed", + "from_subject_area": "Sales", + "to_subject_area": "Catalog", + }, + { + "from_table": "orders", + "to_table": "inventory", + "from_column": "warehouse_id", + "to_column": "location_id", + "from_schema": "public", + "to_schema": "public", + "join_type": "LEFT", + "relationship": "many_to_one", + "confidence": "proposed", + "review_state": "unreviewed", + "from_subject_area": "Sales", + "to_subject_area": "Catalog", + }, ], } _DS_MD = ( @@ -265,27 +469,38 @@ def write(name: str, html: str) -> None: _catalog = next(a for a in _org.subject_areas if a.name == "Catalog") _products = next(t for t in _catalog.tables_defined if t.name == "products") _inventory = next(t for t in _catalog.tables_defined if t.name == "inventory") -write("16-model-overview.html", - admin.model_overview_html(_org, _ver, "SALES_DATA", _dss, **CHROME)) -write("17-model-area.html", - admin.model_area_html(_org, _catalog, "SALES_DATA", _dss, **CHROME)) -write("18-model-table-flat.html", - admin.model_table_html(_org, _catalog, _products, "SALES_DATA", _dss, **CHROME)) -write("19-model-table-grouped.html", - admin.model_table_html(_org, _catalog, _inventory, "SALES_DATA", _dss, **CHROME)) -write("20-model-context.html", - admin.model_context_html(_org, model_store.load_memory(_s, "SALES_DATA"), "SALES_DATA", _dss, - **CHROME)) -write("21-model-relationships.html", - admin.model_relationships_html(_org, "SALES_DATA", _dss, **CHROME)) +write("16-model-overview.html", admin.model_overview_html(_org, _ver, "SALES_DATA", _dss, **CHROME)) +write("17-model-area.html", admin.model_area_html(_org, _catalog, "SALES_DATA", _dss, **CHROME)) +write( + "18-model-table-flat.html", + admin.model_table_html(_org, _catalog, _products, "SALES_DATA", _dss, **CHROME), +) +write( + "19-model-table-grouped.html", + admin.model_table_html(_org, _catalog, _inventory, "SALES_DATA", _dss, **CHROME), +) +write( + "20-model-context.html", + admin.model_context_html( + _org, model_store.load_memory(_s, "SALES_DATA"), "SALES_DATA", _dss, **CHROME + ), +) +write( + "21-model-relationships.html", + admin.model_relationships_html(_org, "SALES_DATA", _dss, **CHROME), +) _s.close() write("08-not-admin.html", admin.not_admin_body_html(BASE)) write("09-landing.html", admin.landing_body_html(BASE)) write("10-mcp-in-browser.html", admin.mcp_landing_body_html(BASE)) write("11-not-authorized.html", admin.not_authorized_body_html("morgan@example.com")) -write("12-setup-password.html", onboarding.setup_page_html("eyJhbGciOi.SAMPLE.xyz")) -write("13-setup-done.html", onboarding.setup_done_html(BASE)) +write("12-setup-password.html", onboarding.claim_page_html("eyJhbGciOi.SAMPLE.xyz")) +write("13-setup-done.html", onboarding.claim_done_html(BASE)) write("14-setup-invalid.html", onboarding.setup_invalid_html()) +# The reset link wears the same two pages with different words, so both need previewing or the +# wording nobody sees in a test goes unreviewed. +write("22-reset-password.html", onboarding.claim_page_html("eyJhbGciOi.SAMPLE.xyz", "reset")) +write("23-reset-done.html", onboarding.claim_done_html(BASE, "reset")) print(f"Wrote {len(list(OUT.glob('*.html')))} previews to {OUT}/") for p in sorted(OUT.glob("*.html")): diff --git a/packages/agami-core/src/oauth_server.py b/packages/agami-core/src/oauth_server.py index f0876f62..50b89e1d 100644 --- a/packages/agami-core/src/oauth_server.py +++ b/packages/agami-core/src/oauth_server.py @@ -647,6 +647,32 @@ def _grant_refresh_token(store: Store, form: dict[str, str]) -> Response: ) +def revoke_refresh_tokens_for(store: Store, username: str, *, commit: bool = True) -> int: + """Revoke every outstanding refresh token a principal holds. Returns how many were live. + + Called when that principal's password is reset by an administrator: whoever was signed in on the + old credential should not keep renewing on it, and a reset that left live sessions running would + be a reset in name only. + + **It does not end sessions instantly, and the difference matters.** Access tokens are self- + contained JWTs — nothing reads this table to validate one — so a session already holding a valid + access token keeps working until it expires (`AGAMI_ACCESS_TTL`, an hour by default). What this + guarantees is that no NEW access token can be minted from an old refresh token, so the longest a + stale session can outlive the reset is one access-token lifetime. Revoking sooner than that would + mean checking a table on every request, which is the tradeoff the stateless bearer already made. + + Rows are marked revoked rather than deleted, deliberately: presenting a revoked token is the + signal `_grant_refresh_token` reads to detect a stolen-token replay, and a deleted row is + indistinguishable from one that never existed. + """ + updated = store.execute( + "UPDATE oauth_refresh_token SET revoked = 1 WHERE username = ? AND revoked = 0", (username,) + ) + if commit: + store.commit() + return updated.rowcount + + # --------------------------------------------------------------------------- # OIDC social login ("Sign in with Google/Microsoft") # diff --git a/packages/agami-core/src/onboarding.py b/packages/agami-core/src/onboarding.py index 6478df02..d4c9e460 100644 --- a/packages/agami-core/src/onboarding.py +++ b/packages/agami-core/src/onboarding.py @@ -1,27 +1,63 @@ -"""Teammate self-onboarding — the **setup link** path for a password deployment. +"""Teammate self-onboarding — the **setup link** and the **reset link**, for a password deployment. When a deployment has no OIDC configured, a teammate the admin created is a *pending* user (no -password, no provider) and there's no email to send an invite to. The admin copies a **setup link** -from the console (a signed, time-boxed token — no new table) and shares it out-of-band; the teammate -opens it and sets their own password. The token is single-use by construction: claiming flips the user -out of the *pending* state, so the guarded `claim_pending_password` UPDATE no-ops any replay. (OIDC -deployments don't use this — teammates bind on first OIDC login at the connector; see `oauth_server`.) +password, no provider) and there's no email to send an invite to. The admin copies a link from the +console (a signed, time-boxed token — no new table) and shares it out-of-band; the teammate opens it +and chooses a password. (OIDC deployments don't use this — teammates bind on first OIDC login at the +connector; see `oauth_server`.) + +**Two purposes, kept strictly disjoint**, because they are two different acts on two different +accounts and one of them overwrites a live credential: + + - `setup` — a *pending* account chooses its first password. Single-use by construction: claiming + flips the user out of pending, so the guarded `claim_pending_password` UPDATE no-ops any replay. + - `reset` — a *claimed* account is given a new password, because an administrator asked for one on + that person's behalf. There is no self-service "forgot password" here, so without this an + administrator can create a colleague's account and then never help them back into it. + +A reset token cannot act on a pending account and a setup token cannot act on a claimed one, so +neither link can be used for the other's job. + +**The reset link is single-use too, by a different mechanism, and it needs one.** The pending flag +retires a setup token; a reset leaves the account exactly as claimed as it was, so nothing about the +row would change and the link would work forever. Instead the token carries a one-way marker of the +credential it was minted against (`user_store.credential_fingerprint`), and the page re-derives it +from the live row: setting a new password changes the stored hash, which retires every link minted +against the old one — including the one just used. + +**A reset ends the sessions running on the old password**, up to one access-token lifetime (see +`oauth_server.revoke_refresh_tokens_for`, which is precise about why "up to"). Two things it does NOT +end, stated because a security control that is believed to cover more than it does is worse than a +narrow one: an outstanding OAuth **authorization code** (a ten-minute window), and the **`/admin` +console cookie**, which is a stateless 12-hour JWT that nothing here revokes. The console cookie only +matters when the account being reset is the configured operator admin, and closing it properly means +binding that cookie to something a reset invalidates — worth doing, and not this change. + +**Holding a reset link is a change-detector for that account.** The page answers 200 while the link +can still act and 400 once it cannot, so whoever holds it can tell the moment the target's password +moves by any route. That falls out of binding the token to the credential and is the price of the +link being single-use; it discloses nothing about the account beyond that one bit. This is a public surface (the teammate has no session yet), so it self-checks: a bad/expired/used -token, or an already-claimed user, yields a generic page — never a credential-overwrite of a claimed -account, and no email-enumeration. (The token is a signed JWT — unforgeable, though its `sub` is -base64url-readable by whoever holds the link; the no-enumeration property comes from the handler -returning the same generic page regardless, not from the token being secret.) +token, a purpose that does not match the account's state, or a marker that no longer matches, all +yield the same generic page — never a credential overwrite the token did not authorise, and no +email-enumeration. (The token is a signed JWT — unforgeable, though its payload is base64url-readable +by whoever holds the link, which is why the marker is a hash and never the credential itself; the +no-enumeration property comes from the handler returning the same generic page regardless, not from +the token being secret.) """ from __future__ import annotations from datetime import datetime, timedelta, timezone +from hmac import compare_digest from typing import Any import jwt +import oauth_server import ui import user_store +from async_offload import run_blocking from oauth_server import _open_store, _signing_secret from starlette.requests import Request from starlette.responses import HTMLResponse, Response @@ -29,9 +65,30 @@ # A generous TTL — the admin shares the link out-of-band and the teammate may take a while. The token # is still single-use (the pending guard), so the window only bounds an unused link, not a claimed one. _SETUP_TTL = timedelta(days=14) +# Deliberately much shorter. An unused setup link is a door to an account nobody has ever been in; an +# unused RESET link is a door to a working account somebody is using today, so the window in which a +# forwarded or mislaid link is worth anything should be a few days, not a fortnight. +_RESET_TTL = timedelta(days=3) _SETUP_PURPOSE = "setup" # marks this token apart from the OAuth bearer + the admin session JWT +_RESET_PURPOSE = "reset" # ...and marks the two links apart from each other; see the module note _MIN_PASSWORD_LEN = 8 -_MAX_PASSWORD_LEN = 256 # cap the input (a sane bound; argon2's cost is fixed, this just rejects junk) +_MAX_PASSWORD_LEN = ( + 256 # cap the input (a sane bound; argon2's cost is fixed, this just rejects junk) +) + + +def _decode(token: str) -> dict[str, Any] | None: + """A token's claims if the signature and expiry hold and it names somebody, else None. Says + nothing about PURPOSE — that is each caller's question, and one shared decoder is what keeps the + two from validating differently.""" + try: + claims = jwt.decode( + token, _signing_secret(), algorithms=["HS256"], options={"require": ["exp", "sub"]} + ) + except Exception: + return None + sub = claims.get("sub") + return claims if isinstance(sub, str) and sub else None def mint_setup_token(username: str) -> str: @@ -49,18 +106,45 @@ def mint_setup_token(username: str) -> str: ) +def mint_reset_token(username: str, fingerprint: str) -> str: + """A signed, time-boxed reset token for `username`, bound to the credential they hold right now. + + `fingerprint` is `user_store.credential_fingerprint` of that account's current row. Binding it + into the token is what makes the link single-use: the page re-derives the marker from the live + row and refuses when they differ, so the first successful reset retires this token. Minting is a + caller's decision — the authorization for it lives in whichever console asked, not here. + """ + now = datetime.now(timezone.utc) + return jwt.encode( + { + "sub": username, + "purpose": _RESET_PURPOSE, + "cred": fingerprint, + "iat": int(now.timestamp()), + "exp": int((now + _RESET_TTL).timestamp()), + }, + _signing_secret(), + algorithm="HS256", + ) + + def verify_setup_token(token: str) -> str | None: """The username a valid setup token names, or None (bad signature / expired / wrong purpose).""" - try: - claims = jwt.decode( - token, _signing_secret(), algorithms=["HS256"], options={"require": ["exp", "sub"]} - ) - except Exception: + claims = _decode(token) + if claims is None or claims.get("purpose") != _SETUP_PURPOSE: return None - if claims.get("purpose") != _SETUP_PURPOSE: + return claims["sub"] + + +def verify_reset_token(token: str) -> tuple[str, str] | None: + """`(username, fingerprint)` a valid reset token names, or None. The fingerprint is returned + rather than checked here: this module does not read the store, and a checker that could not see + the row would have to be trusted by the handler anyway.""" + claims = _decode(token) + if claims is None or claims.get("purpose") != _RESET_PURPOSE: return None - sub = claims.get("sub") - return sub if isinstance(sub, str) and sub else None + cred = claims.get("cred") + return (claims["sub"], cred) if isinstance(cred, str) and cred else None def is_pending(user: dict[str, Any]) -> bool: @@ -75,31 +159,54 @@ def is_pending(user: dict[str, Any]) -> bool: # --------------------------------------------------------------------------- -def setup_page_html(token: str, error: str = "") -> str: - """The set-your-password page reached from a valid setup link.""" +# What each purpose calls itself, on every page the person sees. Held as one table rather than +# branched at each of the four places it is needed: somebody arriving at a reset link has an account +# and knows it, and being told to "finish setting up" would read as though theirs had been wiped. +_WORDING: dict[str, dict[str, str]] = { + _SETUP_PURPOSE: { + "title": "Set up your account", + "lead": "Choose a password to finish setting up.", + "submit": "Set password", + "done": "Your password is set.", + }, + _RESET_PURPOSE: { + "title": "Choose a new password", + "lead": "Your administrator asked us to let you set a new password.", + "submit": "Save password", + # Says the part that is surprising if unexplained: they will have to sign in again elsewhere. + "done": "Your new password is set. Anywhere you were signed in will ask for it again.", + }, +} + + +def claim_page_html(token: str, purpose: str = _SETUP_PURPOSE, error: str = "") -> str: + """The choose-a-password page reached from a valid link, worded for what the link is for.""" + words = _WORDING[purpose] alert = f'
{ui.esc(error)}
' if error else "" - body = f"""

Set up your account

-

Choose a password to finish setting up.

+ body = f"""

{ui.esc(words["title"])}

+

{ui.esc(words["lead"])}

{alert}
- +
""" - return ui.auth_page("Set up your account", body) + return ui.auth_page(words["title"], body) -def setup_done_html(base_url: str) -> str: +def claim_done_html(base_url: str, purpose: str = _SETUP_PURPOSE) -> str: body = f"""

You're all set

-

Your password is set. Add this server to Claude as a custom connector:

+

{ui.esc(_WORDING[purpose]["done"])} Add this server to Claude as a custom +connector:

{ui.esc(base_url)}/mcp

""" return ui.auth_page("All set", body) def setup_invalid_html() -> str: + """One page for every failure — see the module note on why it says nothing specific.""" body = """

This link isn't valid

-

It may have expired or already been used. Ask your administrator for a new setup +

It may have expired or already been used. Ask your administrator for a new link.

""" return ui.auth_page("Invalid link", body) @@ -114,55 +221,141 @@ async def _form(request: Request) -> dict[str, str]: return {k: (v if isinstance(v, str) else "") for k, v in data.items()} -def _pending_user(username: str) -> dict[str, Any] | None: - """The pending user a token names, or None (unknown / already claimed / no store).""" +def _user(username: str) -> dict[str, Any] | None: + """The account a token names, or None (unknown, or no store).""" store = _open_store() if store is None: return None try: - user = user_store.get_user(store, username) + return user_store.get_user(store, username) finally: store.close() - return user if (user is not None and is_pending(user)) else None + + +def _actionable(token: str) -> tuple[str, str, str | None] | None: + """`(username, purpose, expected_hash)` for a token that may act on that account right now. + + The one place the two purposes are matched against the state of the row, so a link can only ever + do the job it was minted for: + + - `setup` requires a **pending** account — nothing to overwrite. + - `reset` requires a **claimed, active, password-based** account whose credential is still the + one the token was minted against. Together the two mean a reset token cannot act on a pending + account, nor a setup token on a claimed one. + + **Every condition here mirrors `user_store.reset_password`'s WHERE, and the mirroring is the + point.** When it did not, two things broke at once and neither was obvious. A token for an account + that had since bound an identity provider got a **200 and a password form**, because only the + write refused — so somebody chose a password, submitted it and was told the link was invalid, + which is the exact split the switched-off case already calls unacceptable. And because such a + token could never spend (the write refuses, so the credential never changes, so the marker still + matches), it stayed actionable for its whole three-day life while every POST paid for an argon2 + hash: an unauthenticated, unrate-limited, unbounded CPU lever, measured at 45x a rejected token. + Refusing here is what makes both of those a flat 400 that hashes nothing. + + `expected_hash` is handed back so the UPDATE can be guarded on it. Checking the marker here and + writing afterwards is check-then-act across two connections; see `reset_password` for why that + loses the single-use property it appears to provide. + """ + # Through the verifiers rather than re-deriving from `_decode`: each purpose's claim validation + # then exists once. When this decoded for itself, `verify_reset_token`'s `isinstance` check on + # `cred` was skipped on the live path — so the type-checked function was dead code and the public + # path was the one missing the check, which is the drift `_decode` exists to prevent. + setup_for = verify_setup_token(token) + if setup_for is not None: + user = _user(setup_for) + return (setup_for, _SETUP_PURPOSE, None) if user and is_pending(user) else None + + reset = verify_reset_token(token) + if reset is None: + return None + username, cred = reset + user = _user(username) + if user is None: + return None + stored = user.get("password_hash") + if ( + not stored + or user.get("oidc_provider") is not None + or user.get("status") != user_store.ACTIVE_STATUS + ): + return None + # `compare_digest` rather than `==` for the usual reason a secret-derived value gets it: this is + # the check that decides whether a link is spent, and it is reachable by anyone holding it. + if not compare_digest(user_store.credential_fingerprint(user), cred): + return None + return (username, _RESET_PURPOSE, stored) async def claim(request: Request) -> Response: - """GET → the set-password page for a valid link to a still-pending user; POST → set the password. - Every failure (bad token, already-claimed, weak password, lost race) is a generic page — no - credential overwrite of a claimed account, no enumeration. Both the token check and the pending - re-read run BEFORE any argon2 hash, so an attacker can't drive hashing without a valid (signed, - admin-minted) token for a still-pending user. This endpoint is **not** rate-limited in-process — - rely on the deployment's proxy/LB for that (a documented gap, like the other public endpoints).""" + """GET → the choose-a-password page for a link that can still act; POST → write the password. + + Every failure (bad token, wrong state for the purpose, a spent reset link, a weak password, a lost + race) is the same generic page — no credential overwrite a token did not authorise, and no + enumeration. This endpoint is **not** rate-limited in-process — rely on the deployment's proxy/LB + for that (a documented gap, like the other public endpoints). + """ if request.method == "GET": - username = verify_setup_token(request.query_params.get("token", "")) - if username is None or _pending_user(username) is None: + token = request.query_params.get("token", "") + actionable = _actionable(token) + if actionable is None: return HTMLResponse(setup_invalid_html(), status_code=400) - return HTMLResponse(setup_page_html(request.query_params.get("token", ""))) + return HTMLResponse(claim_page_html(token, actionable[1])) form = await _form(request) token = form.get("token", "") - username = verify_setup_token(token) - if username is None or _pending_user(username) is None: + actionable = _actionable(token) + if actionable is None: return HTMLResponse(setup_invalid_html(), status_code=400) + username, purpose, expected_hash = actionable password = form.get("password", "") if not _MIN_PASSWORD_LEN <= len(password) <= _MAX_PASSWORD_LEN: return HTMLResponse( - setup_page_html(token, error=f"Use at least {_MIN_PASSWORD_LEN} characters."), + claim_page_html(token, purpose, error=f"Use at least {_MIN_PASSWORD_LEN} characters."), status_code=400, ) + # Off the event loop. Argon2 is deliberately slow and memory-hard, and this handler is `async` on a + # public endpoint — hashing inline stalls every other request in flight for the duration, which is + # the reason `oauth_server`'s own credential check offloads. Everything blocking goes together, so + # the store work rides the same worker rather than crossing back and forth. + changed = await run_blocking(_write_password, username, purpose, password, expected_hash) + if not changed: + return HTMLResponse(setup_invalid_html(), status_code=400) + from mcp_http import public_base_url + + return HTMLResponse(claim_done_html(public_base_url(), purpose)) + + +def _write_password(username: str, purpose: str, password: str, expected_hash: str | None) -> int: + """Set the password and, for a reset, revoke that principal's sessions. Rows changed, 0 if refused. + + **One transaction, and it has to be.** The password moving and the old sessions dying are one + event: committed separately, a revocation that failed after the write would leave the credential + changed, every old refresh token live, and the caller looking at a 500 — a reset in name only, on + a link that is now spent so they cannot retry. Committed together, either both happen or neither + does. `revoke_refresh_tokens_for` is also called only when the write actually moved a row, so a + lost race never signs somebody out on behalf of a change that did not happen. + + Synchronous by design — it is called through `run_blocking`; see the caller. + """ store = _open_store() if store is None: - return HTMLResponse(setup_invalid_html(), status_code=400) + return 0 try: - # Guarded: the UPDATE only fires while still pending — a concurrent claim makes it a no-op. - changed = user_store.claim_pending_password(store, username, password) + if purpose == _SETUP_PURPOSE: + # Guarded: the UPDATE only fires while still pending — a concurrent claim makes it a no-op. + return user_store.claim_pending_password(store, username, password) + # Likewise guarded, on different conditions (`user_store.reset_password` spells them out) — + # including the credential it was minted against, which is what makes a replay a no-op here. + changed = user_store.reset_password( + store, username, password, expected_hash or "", commit=False + ) + if changed: + oauth_server.revoke_refresh_tokens_for(store, username, commit=False) + store.commit() + return changed finally: store.close() - if not changed: - return HTMLResponse(setup_invalid_html(), status_code=400) - from mcp_http import public_base_url - - return HTMLResponse(setup_done_html(public_base_url())) def routes() -> list: diff --git a/packages/agami-core/src/user_store.py b/packages/agami-core/src/user_store.py index 5d95e77f..e42f9c10 100644 --- a/packages/agami-core/src/user_store.py +++ b/packages/agami-core/src/user_store.py @@ -12,6 +12,7 @@ from __future__ import annotations +import hashlib import os from datetime import datetime, timezone from typing import Any @@ -21,7 +22,13 @@ from ports import Principal from store import Store -_ACTIVE = "active" +#: The status a live account holds. **Public, because two modules must agree on it**: `onboarding` +#: refuses a reset for a switched-off account before drawing the page, and `reset_password`'s WHERE +#: refuses the write. Those two checks exist deliberately as separate layers, and a copy of this +#: string in each is exactly how layers drift apart — which is the defect this whole flow was fixed +#: for once already. `_ACTIVE` stays as the in-module alias so nothing below has to change. +ACTIVE_STATUS = "active" +_ACTIVE = ACTIVE_STATUS # The OIDC provider keys a deploy may pin the admin to. Mirrors `oidc._PROVIDERS`, duplicated here on # purpose: `oidc` is the one egress module (httpx), and `user_store` must stay import-light + egress-free. @@ -127,6 +134,82 @@ def claim_pending_password(store: Store, username: str, password: str) -> int: return cur.rowcount +def reset_password( + store: Store, username: str, password: str, expected_hash: str, *, commit: bool = True +) -> int: + """Set the password of an account that **already has one** — the administrator-initiated reset. + + Deliberately the opposite of `claim_pending_password` above, which fires only while an account is + still pending. This one overwrites a live credential, so every condition that makes that safe is + in the WHERE rather than trusted to the caller: a guard a route forgets to apply is a guard that + is not there, and this UPDATE is one route away from a public page. + + - `oidc_provider IS NULL` — an SSO identity never grows a password. Without it, a reset would add + a second way into an account whose deployment decided there is exactly one, and the person + whose password it is would have no idea it existed. + - `status = ?` (active) — a reset cannot resurrect a switched-off account. The account being off + is the security decision; letting a link undo it silently would make the link the stronger one. + - `password_hash = ?` (`expected_hash`) — **this is what actually makes a reset link single-use.** + Checking the credential marker in the handler and then writing is check-then-act across two + connections: two simultaneous posts of the same link both read the old hash, both pass, and both + UPDATE, so whoever commits last owns the account while the other is told they succeeded. As a + condition of the UPDATE the loser matches no row, gets 0, and is shown the same invalid page as + any other spent link. `claim_pending_password` above is single-use by exactly this shape + (`password_hash IS NULL`); this is that argument applied to a credential that already exists. + + `commit=False` leaves the transaction open so the caller can land the revocation of that + principal's sessions in the same one — a password that has moved with sessions still renewing on + the old one is a reset in name only, and the two must not be separately committable. + + Returns the row count: 0 means refused, and the caller must not report success. It cannot tell + you WHICH condition refused, on purpose — the page this is reached from is public and answers + every failure identically. + """ + cur = store.execute( + "UPDATE users SET password_hash = ? WHERE username = ? AND password_hash = ? " + "AND password_hash IS NOT NULL AND oidc_provider IS NULL AND status = ?", + (hash_password(password), username, expected_hash, _ACTIVE), + ) + if commit: + store.commit() + return cur.rowcount + + +def credential_fingerprint(user: dict[str, Any]) -> str: + """A short, one-way marker of the credential an account holds right now. + + This is what makes a reset link **single-use**, and it needs its own mechanism because the one + that makes a *setup* link single-use does not apply: a setup token dies because claiming flips the + account out of pending, and a reset leaves the account exactly as claimed as it was. So the token + carries this marker, the claim page re-computes it, and setting a new password changes the stored + hash — which retires every link minted against the old one, including the one just used. + + **A hash of the hash, never the hash itself.** A setup link's payload is base64url and readable by + whoever holds it (`onboarding`'s module note says so), so putting `password_hash` in a token would + hand an argon2 digest to anyone the link is forwarded to. Truncated because this is a change + detector, not a credential: it needs to differ when the hash differs, and it is compared only + against a value re-derived from the same row. + + Argon2 salts every hash, so re-setting the *same* password still produces a different digest and + still retires the old link. `authenticate`'s opportunistic rehash changes the stored hash too, so + a cost-parameter bump silently spends outstanding reset links on the owner's next sign-in — which + is the safe direction and worth knowing before it looks like a bug. + + **Raises for an account with no password**, rather than fingerprinting the empty string. That + fallback would have produced `sha256("")` — one publicly computable constant, identical for every + passwordless row in every deployment — so the marker would have been a marker of nothing and the + single-use binding would have been vacuous exactly where the account is most exposed. There is no + legitimate caller: a reset is for an account that HAS a credential, and a pending one is the setup + link's job. + """ + stored = user.get("password_hash") + if not stored: + raise ValueError( + "this account has no password to fingerprint; a reset does not apply to it" + ) + return hashlib.sha256(stored.encode()).hexdigest()[:16] + + def get_user_by_email(store: Store, email: str) -> dict[str, Any] | None: """Look up a user by email — the onboarded-only lookup OIDC uses. Case-insensitive (emails are stored lowercased) and one-to-one (the email index is UNIQUE).""" diff --git a/tests/test_onboarding.py b/tests/test_onboarding.py index dbbd746b..3ca71c59 100644 --- a/tests/test_onboarding.py +++ b/tests/test_onboarding.py @@ -25,6 +25,7 @@ import jwt # noqa: E402 import mcp_http # noqa: E402 +import oauth_server # noqa: E402 import onboarding # noqa: E402 import user_store # noqa: E402 from starlette.testclient import TestClient # noqa: E402 @@ -36,6 +37,21 @@ ADMIN_PW = "admin-password-localtest" PENDING = "newbie@example.com" +# The password a refused attempt sends. A whole English phrase rather than a credential-shaped string +# — the convention `tests/e2e/test_member_onboarding.py` already states for its own passphrase — and +# here it is load-bearing rather than stylistic: the credential-shaped version tripped the secret scan +# in CI on four lines. It only has to clear agami-core's eight-character floor and never be accepted. +# **Every password in the reset tests is an English phrase, not a credential-shaped string**, and +# that is a requirement rather than a style: the credential-shaped versions tripped two independent +# secret scanners in CI. They are synthetic either way — what changes is whether a scanner can tell. +# The convention (and the reason) is already stated in `tests/e2e/test_member_onboarding.py`. Each +# only has to clear agami-core's eight-character floor. +REFUSED_ATTEMPT = "the password this request must not be allowed to set" +CHOSEN_AT_RESET = "the passphrase chosen when the reset link was used" +REPLAYED_ATTEMPT = "the passphrase a replayed link would try to set" +SET_OUT_OF_BAND = "the passphrase set by some route other than the link" +ALREADY_HELD = "the passphrase this account already had" + @pytest.fixture def env(tmp_path, monkeypatch): @@ -46,8 +62,12 @@ def env(tmp_path, monkeypatch): monkeypatch.setenv("AGAMI_ADMIN_USERNAME", ADMIN_USER) monkeypatch.setenv("AGAMI_ADMIN_PASSWORD", ADMIN_PW) # A password deployment — no OIDC configured. - for var in ("AGAMI_OIDC_GOOGLE_CLIENT_ID", "AGAMI_OIDC_GOOGLE_CLIENT_SECRET", - "AGAMI_OIDC_MICROSOFT_CLIENT_ID", "AGAMI_OIDC_MICROSOFT_CLIENT_SECRET"): + for var in ( + "AGAMI_OIDC_GOOGLE_CLIENT_ID", + "AGAMI_OIDC_GOOGLE_CLIENT_SECRET", + "AGAMI_OIDC_MICROSOFT_CLIENT_ID", + "AGAMI_OIDC_MICROSOFT_CLIENT_SECRET", + ): monkeypatch.delenv(var, raising=False) s = Store.connect(db_url) s.run_migrations() @@ -73,16 +93,27 @@ def test_setup_token_round_trips(env): def test_setup_token_rejects_forged_expired_and_wrong_purpose(env): assert onboarding.verify_setup_token("not-a-jwt") is None # signed with a different key → bad signature - forged = jwt.encode({"sub": PENDING, "purpose": "setup", "exp": 9_999_999_999}, "y" * 40, algorithm="HS256") + forged = jwt.encode( + {"sub": PENDING, "purpose": "setup", "exp": 9_999_999_999}, "y" * 40, algorithm="HS256" + ) assert onboarding.verify_setup_token(forged) is None # expired expired = jwt.encode( - {"sub": PENDING, "purpose": "setup", "exp": int(datetime(2000, 1, 1, tzinfo=timezone.utc).timestamp())}, - SECRET, algorithm="HS256", + { + "sub": PENDING, + "purpose": "setup", + "exp": int(datetime(2000, 1, 1, tzinfo=timezone.utc).timestamp()), + }, + SECRET, + algorithm="HS256", ) assert onboarding.verify_setup_token(expired) is None # right key, wrong purpose (e.g. a bearer/admin-session token replayed as a setup link) - wrong = jwt.encode({"sub": PENDING, "purpose": "admin_session", "exp": 9_999_999_999}, SECRET, algorithm="HS256") + wrong = jwt.encode( + {"sub": PENDING, "purpose": "admin_session", "exp": 9_999_999_999}, + SECRET, + algorithm="HS256", + ) assert onboarding.verify_setup_token(wrong) is None @@ -103,17 +134,21 @@ def test_claim_link_is_single_use(client, env): token = onboarding.mint_setup_token(PENDING) client.post("/claim", data={"token": token, "password": "teammate-pw-123"}) # replay: the user is no longer pending → generic invalid page, password unchanged - r = client.post("/claim", data={"token": token, "password": "attacker-pw-999"}) + r = client.post("/claim", data={"token": token, "password": REPLAYED_ATTEMPT}) assert r.status_code == 400 and "isn't valid" in r.text s = Store.connect(env) - assert user_store.authenticate(s, PENDING, "teammate-pw-123") is not None # original still works - assert user_store.authenticate(s, PENDING, "attacker-pw-999") is None + assert ( + user_store.authenticate(s, PENDING, "teammate-pw-123") is not None + ) # original still works + assert user_store.authenticate(s, PENDING, REPLAYED_ATTEMPT) is None s.close() def test_claim_rejects_a_bad_token(client): assert client.get("/claim", params={"token": "nope"}, follow_redirects=False).status_code == 400 - assert client.post("/claim", data={"token": "nope", "password": "whatever-123"}).status_code == 400 + assert ( + client.post("/claim", data={"token": "nope", "password": "whatever-123"}).status_code == 400 + ) def test_claim_rejects_a_short_password(client, env): @@ -131,6 +166,337 @@ def test_claim_for_an_already_password_user_is_refused(client, env): assert client.get("/claim", params={"token": token}).status_code == 400 +# --- the RESET link ---------------------------------------------------------- +# +# A reset link overwrites a credential somebody is using today, which the setup link never does. So +# these press on the four things that keep that safe: it works only on the account state it was minted +# for, it is spent by use, it can never reach an SSO or a switched-off account, and it ends the +# sessions running on the old password. + + +def _reset_token(env, username: str) -> str: + """A reset link's token, minted the way the console mints one — against the credential the account + holds at this moment.""" + s = Store.connect(env) + try: + return onboarding.mint_reset_token( + username, user_store.credential_fingerprint(user_store.get_user(s, username)) + ) + finally: + s.close() + + +def test_reset_token_and_setup_token_are_not_interchangeable(env): + # The property the two purposes exist for. A setup token names a pending account; a reset token + # names a claimed one; neither verifier accepts the other's token, so neither link can do the + # other's job even before the store is consulted. + setup, reset = onboarding.mint_setup_token(PENDING), _reset_token(env, ADMIN_USER) + assert onboarding.verify_setup_token(reset) is None + assert onboarding.verify_reset_token(setup) is None + assert onboarding.verify_reset_token(reset)[0] == ADMIN_USER + + +def test_reset_link_sets_a_new_password_for_a_claimed_user(client, env): + token = _reset_token(env, ADMIN_USER) + # Worded for somebody who HAS an account — being told to "finish setting up" would read as though + # theirs had been wiped. + assert "Choose a new password" in client.get("/claim", params={"token": token}).text + assert ( + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}).status_code == 200 + ) + s = Store.connect(env) + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is not None + assert user_store.authenticate(s, ADMIN_USER, ADMIN_PW) is None # the old one is gone + s.close() + + +def test_reset_link_is_single_use(client, env): + # Nothing about the ROW changes state here the way a claim flips `pending`, so the only thing + # retiring this link is the credential marker in the token. If that check regressed, this replay + # would succeed and an administrator's old link would stay live forever. + token = _reset_token(env, ADMIN_USER) + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}) + replay = client.post("/claim", data={"token": token, "password": REPLAYED_ATTEMPT}) + assert replay.status_code == 400 and "isn't valid" in replay.text + s = Store.connect(env) + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is not None + assert user_store.authenticate(s, ADMIN_USER, REPLAYED_ATTEMPT) is None + s.close() + + +def test_a_reset_link_dies_when_the_password_moves_by_any_other_route(client, env): + # The marker is bound to the credential, not to this request — so a link minted before somebody + # changed their password elsewhere is already spent when it arrives. + token = _reset_token(env, ADMIN_USER) + s = Store.connect(env) + user_store.reset_password( + s, ADMIN_USER, SET_OUT_OF_BAND, user_store.get_user(s, ADMIN_USER)["password_hash"] + ) + s.close() + assert client.get("/claim", params={"token": token}).status_code == 400 + + +def test_a_reset_token_cannot_act_on_a_pending_account(client, env): + # The mirror of `test_claim_for_an_already_password_user_is_refused`, and the reason the purposes + # are matched against the row's state rather than trusted from the token. The marker is hand-made: + # there is no legitimate way to mint one for a pending account, which is the next test. + forged_for_pending = onboarding.mint_reset_token(PENDING, "deadbeefdeadbeef") + assert client.get("/claim", params={"token": forged_for_pending}).status_code == 400 + assert ( + client.post( + "/claim", data={"token": forged_for_pending, "password": REFUSED_ATTEMPT} + ).status_code + == 400 + ) + + +def test_a_credential_marker_cannot_be_taken_for_an_account_with_no_password(env): + """The marker for a passwordless row would otherwise be `sha256("")` — one constant, identical for + every such row in every deployment, so the binding that makes a reset link single-use would be a + binding to a publicly known value exactly where the account is least protected. Raising means there + is no way to mint one at all, rather than a way that quietly means nothing.""" + s = Store.connect(env) + try: + with pytest.raises(ValueError): + user_store.credential_fingerprint(user_store.get_user(s, PENDING)) + # ...while a claimed account gives one, and it changes when the credential does. + before = user_store.credential_fingerprint(user_store.get_user(s, ADMIN_USER)) + user_store.reset_password( + s, ADMIN_USER, CHOSEN_AT_RESET, user_store.get_user(s, ADMIN_USER)["password_hash"] + ) + assert user_store.credential_fingerprint(user_store.get_user(s, ADMIN_USER)) != before + finally: + s.close() + + +def test_a_reset_never_gives_an_sso_identity_a_password(client, env): + # An SSO account has no password by design. A reset that added one would be a second way in that + # the deployment never chose and the account holder would never know about. + s = Store.connect(env) + user_store.create_user( + s, + username="sso@example.com", + email="sso@example.com", + password=None, + oidc_provider="google", + oidc_subject="sub-123", + ) + s.close() + # Hand-made marker again — the account has no credential to mint one against. + token = onboarding.mint_reset_token("sso@example.com", "deadbeefdeadbeef") + # The GET matters as much as the POST: drawing the form and refusing the submit is the split this + # whole resolver exists to avoid. + assert client.get("/claim", params={"token": token}).status_code == 400 + assert ( + client.post("/claim", data={"token": token, "password": REFUSED_ATTEMPT}).status_code == 400 + ) + s = Store.connect(env) + assert user_store.get_user(s, "sso@example.com")["password_hash"] is None + s.close() + + +def test_a_reset_is_refused_once_an_account_has_bound_an_identity_provider(client, env): + """The case that ISOLATES the `oidc_provider IS NULL` guard, and the reachable one. + + The test above does not: that account has no password either, so `password_hash IS NOT NULL` + refuses it first and the OIDC guard is never consulted — removing the guard leaves that test + green. This is the state where only the guard stands in the way: somebody who signed in with a + password and has since bound a provider (`bind_oidc_subject`, on their first OIDC login), so the + row carries BOTH. Their way in is now the provider, and a reset would quietly re-arm the password + that is no longer how they get in. + """ + s = Store.connect(env) + user_store.create_user( + s, username="both@example.com", email="both@example.com", password=ALREADY_HELD + ) + user_store.bind_oidc_subject(s, "both@example.com", "sub-456") + s.execute( + "UPDATE users SET oidc_provider = ? WHERE username = ?", ("google", "both@example.com") + ) + s.commit() + token = onboarding.mint_reset_token( + "both@example.com", + user_store.credential_fingerprint(user_store.get_user(s, "both@example.com")), + ) + s.close() + # 400 on the GET too. When it was 200 the page was drawn for an account the write would refuse, so + # somebody chose a password, submitted it and was told the link was invalid — and, because such a + # token can never spend, every attempt paid for an argon2 hash on a public endpoint. + assert client.get("/claim", params={"token": token}).status_code == 400 + assert ( + client.post("/claim", data={"token": token, "password": REFUSED_ATTEMPT}).status_code == 400 + ) + s = Store.connect(env) + assert user_store.authenticate(s, "both@example.com", REFUSED_ATTEMPT) is None + s.close() + + +def test_a_reset_cannot_revive_a_switched_off_account(client, env): + # Switching an account off is a security decision; a link that undid it would be the stronger of + # the two, and it is held by whoever was forwarded it. + token = _reset_token(env, ADMIN_USER) + s = Store.connect(env) + user_store.set_status(s, ADMIN_USER, "disabled") + s.close() + # The GET matters on its own: without the handler's own check the page would be DRAWN for a + # switched-off account and only the write would refuse, so somebody would choose a password, submit + # it, and be told the link is invalid. Refusing here is the difference between the two layers. + assert client.get("/claim", params={"token": token}).status_code == 400 + assert ( + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}).status_code == 400 + ) + s = Store.connect(env) + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is None + s.close() + + +def test_reset_password_refuses_in_its_own_where_clause(env): + """`user_store.reset_password`'s guards, exercised directly rather than through a page. + + **Not redundant with the tests above, and finding that out is why this exists.** `_actionable` + checks the same conditions first, so through `/claim` these UPDATEs are never reached with a row + that should be refused — deleting a condition from the WHERE left every page test green. A guard + nothing can fail is a guard nothing is holding, and this one is the last line before a live + credential is overwritten. + """ + s = Store.connect(env) + try: + # Pending: no password to replace. This is `claim_pending_password`'s job, not this one's. + assert user_store.reset_password(s, PENDING, CHOSEN_AT_RESET, "whatever") == 0 + # Bound to an identity provider: their way in is the provider now. + user_store.create_user( + s, username="bound@example.com", email="bound@example.com", password=ALREADY_HELD + ) + s.execute( + "UPDATE users SET oidc_provider = ? WHERE username = ?", ("google", "bound@example.com") + ) + s.commit() + bound = user_store.get_user(s, "bound@example.com")["password_hash"] + assert user_store.reset_password(s, "bound@example.com", CHOSEN_AT_RESET, bound) == 0 + # Switched off: the account being off outranks the link. + user_store.create_user( + s, username="off@example.com", email="off@example.com", password=ALREADY_HELD + ) + user_store.set_status(s, "off@example.com", "disabled") + off = user_store.get_user(s, "off@example.com")["password_hash"] + assert user_store.reset_password(s, "off@example.com", CHOSEN_AT_RESET, off) == 0 + # ...and the one it is for. `stale` is captured first so the replay below can present it. + stale = user_store.get_user(s, ADMIN_USER)["password_hash"] + assert user_store.reset_password(s, ADMIN_USER, CHOSEN_AT_RESET, stale) == 1 + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is not None + # **The race.** A second post of the same link presents the hash it read before the first + # one landed. Without `password_hash = ?` in the WHERE both writes succeed and the later + # one owns the account, while the person who legitimately reset is told it worked. + assert user_store.reset_password(s, ADMIN_USER, REPLAYED_ATTEMPT, stale) == 0 + assert user_store.authenticate(s, ADMIN_USER, CHOSEN_AT_RESET) is not None + finally: + s.close() + + +def test_a_reset_revokes_the_sessions_running_on_the_old_password(client, env): + # Otherwise the reset is one in name only: whoever was signed in keeps renewing indefinitely on a + # credential that has been taken away from them. + s = Store.connect(env) + s.execute( + "INSERT INTO oauth_refresh_token (token_hash, family, client_id, username, expires_at, revoked, created) " + "VALUES (?, ?, ?, ?, ?, 0, ?)", + ("hash-1", "fam-1", "client-1", ADMIN_USER, "2099-01-01T00:00:00Z", "2026-01-01T00:00:00Z"), + ) + # A second principal's live token, to prove the revocation is scoped to one person. + s.execute( + "INSERT INTO oauth_refresh_token (token_hash, family, client_id, username, expires_at, revoked, created) " + "VALUES (?, ?, ?, ?, ?, 0, ?)", + ("hash-2", "fam-2", "client-1", PENDING, "2099-01-01T00:00:00Z", "2026-01-01T00:00:00Z"), + ) + s.commit() + s.close() + token = _reset_token(env, ADMIN_USER) + assert ( + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}).status_code == 200 + ) + s = Store.connect(env) + assert ( + s.query("SELECT revoked FROM oauth_refresh_token WHERE token_hash = ?", ("hash-1",))[0][ + "revoked" + ] + == 1 + ) + assert ( + s.query("SELECT revoked FROM oauth_refresh_token WHERE token_hash = ?", ("hash-2",))[0][ + "revoked" + ] + == 0 + ) + # The count is the function's only output; asserting it is what stops it silently becoming 0. + assert oauth_server.revoke_refresh_tokens_for(s, PENDING) == 1 + s.close() + + +def test_a_failed_reset_leaves_every_session_alone(client, env, monkeypatch): + """The ordering: revocation happens only for a write that actually moved a row. + + **The obvious version of this test does not test it.** Posting a short password returns on the + length check, before the store is opened at all — so the guard could be deleted and the test would + still pass, which is what review found. The write has to be REACHED and refused, so the UPDATE is + forced to match nothing: that is the lost-race outcome, where the credential moved between the + check and the write. + """ + s = Store.connect(env) + s.execute( + "INSERT INTO oauth_refresh_token (token_hash, family, client_id, username, expires_at, revoked, created) " + "VALUES (?, ?, ?, ?, ?, 0, ?)", + ("hash-3", "fam-3", "client-1", ADMIN_USER, "2099-01-01T00:00:00Z", "2026-01-01T00:00:00Z"), + ) + s.commit() + s.close() + token = _reset_token(env, ADMIN_USER) + monkeypatch.setattr(user_store, "reset_password", lambda *a, **k: 0) + assert ( + client.post("/claim", data={"token": token, "password": CHOSEN_AT_RESET}).status_code == 400 + ) + s = Store.connect(env) + assert ( + s.query("SELECT revoked FROM oauth_refresh_token WHERE token_hash = ?", ("hash-3",))[0][ + "revoked" + ] + == 0 + ) + s.close() + + +def test_claim_refuses_a_token_whose_purpose_is_not_one_of_the_two(client, env): + """Wrong-purpose rejection, on the LIVE path. + + It was covered only against `verify_setup_token`, which the handler stopped calling when + `_actionable` began decoding for itself — so the property still held and nothing was holding it: + review mutated the purpose comparison so that an `admin_session` cookie JWT would act as a setup + link, and every test stayed green. These post the tokens that actually exist in this deployment + signed with the same secret, plus one carrying no purpose at all. + """ + for purpose in ("admin_session", "", None, 7): + claims = {"sub": PENDING, "exp": 9_999_999_999} + if purpose is not None: + claims["purpose"] = purpose + token = jwt.encode(claims, SECRET, algorithm="HS256") + assert client.get("/claim", params={"token": token}).status_code == 400, purpose + assert ( + client.post("/claim", data={"token": token, "password": REFUSED_ATTEMPT}).status_code + == 400 + ), purpose + + +def test_a_reset_token_with_a_malformed_marker_is_refused_rather_than_crashing(client, env): + """A `cred` that is not a string reached `compare_digest` and raised, which is a 500 — the one + response distinguishable from the uniform generic page, and therefore an oracle. The handler now + goes through `verify_reset_token`, whose type check is what keeps this a 400.""" + for cred in (None, 7, ["x"]): + claims = {"sub": ADMIN_USER, "purpose": "reset", "exp": 9_999_999_999} + if cred is not None: + claims["cred"] = cred + token = jwt.encode(claims, SECRET, algorithm="HS256") + assert client.get("/claim", params={"token": token}).status_code == 400, cred + + # --- the admin roster setup link --------------------------------------------- @@ -142,7 +508,7 @@ def test_admin_roster_shows_a_setup_link_for_pending_users(client): _login(client) html = client.get("/admin").text assert "Setup link" in html - m = re.search(r'/claim\?token=([\w.\-]+)', html) + m = re.search(r"/claim\?token=([\w.\-]+)", html) assert m and onboarding.verify_setup_token(m.group(1)) == PENDING # the admin's own (password) row offers no setup link admin_row = next(r for r in html.split("") if ADMIN_USER in r and "