feat: authorize consequential pull reviews (Closes #507)
This commit is contained in:
1 parent
c8809a0728
commit
2a6f2ab12d
19 files changed
+263
-29
No files matched your search
@@ -480,6 +480,35 @@ outbox.enqueue({{kind:'pull-comment',repository:'o/r',number:2,body:'Review',ope
|
||||
assert output["remaining"] == []
|
||||
|
||||
|
||||
def test_authorization_hold_can_resume_in_foreground_with_original_review_identity():
|
||||
script = f"""
|
||||
const createAuthoredOutbox = require({json.dumps(str(OUTBOX))});
|
||||
const values=new Map();const sent=[];
|
||||
const storage={{getItem:k=>values.get(k)||null,setItem:(k,v)=>values.set(k,v)}};
|
||||
const outbox=createAuthoredOutbox({{
|
||||
storage,getOwnerLogin:()=>'timmy',
|
||||
backgroundSync:{{reconcile:async()=>{{}},requestSync:async()=>{{}},
|
||||
send:async item=>{{sent.push({{operationId:item.operationId,status:item.status,
|
||||
head:item.expectedHeadSha,decision:item.decision}});return{{message:{{id:91}}}};}}}},
|
||||
}});
|
||||
outbox.enqueue({{kind:'pull-review',repository:'o/r',number:7,operationId:'review-stable',
|
||||
body:'Ship it',decision:'approve',expectedHeadSha:'abc123'}});
|
||||
outbox.reconcileBackground([{{id:'review-stable',kind:'pull-review',status:'authorization',
|
||||
error:'Fresh authorization required'}}]);
|
||||
(async()=>{{const held=outbox.list()[0];const result=await outbox.retry(held.id,'timmy');
|
||||
process.stdout.write(JSON.stringify({{held,sent,result,remaining:outbox.list()}}));}})();
|
||||
"""
|
||||
output = run_node(script)
|
||||
|
||||
assert output["held"]["status"] == "authorization"
|
||||
assert output["sent"] == [{
|
||||
"operationId": "review-stable", "status": "authorization",
|
||||
"head": "abc123", "decision": "approve",
|
||||
}]
|
||||
assert output["result"]["confirmed"] == [{"id": 91}]
|
||||
assert output["remaining"] == []
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_mobile_dashboard_loads_and_operates_authored_message_outbox():
|
||||
html = await dashboard()
|
||||
|
||||
@@ -409,6 +409,41 @@ createBackgroundIssueSync({{store,fetchJson}}).flush().then(result=>process.stdo
|
||||
}]
|
||||
|
||||
|
||||
def test_closed_app_sync_holds_consequential_review_for_foreground_authorization():
|
||||
script = f"""
|
||||
const createBackgroundIssueSync = require({json.dumps(str(SYNC))});
|
||||
const item={{id:'review-op',operationId:'review-op',ownerLogin:'timmy',status:'queued',
|
||||
kind:'pull-review',repository:'stackchain/web',number:8,body:'Looks good',
|
||||
decision:'approve',expectedHeadSha:'abc123',comments:[]}};
|
||||
const state={{authorized:[],failed:[],released:[],completed:[]}};let claimed=false;
|
||||
const store={{
|
||||
claimNext:async()=>claimed?null:(claimed=true,item),
|
||||
authorization:async(id,error)=>state.authorized.push({{id,error}}),
|
||||
fail:async(...args)=>state.failed.push(args),release:async id=>state.released.push(id),
|
||||
complete:async id=>state.completed.push(id),countBlocked:async()=>0,
|
||||
}};
|
||||
const fetchJson=async url=>{{
|
||||
if(url==='api/v1/background-identity')return{{login:'timmy'}};
|
||||
const error=new Error('Fresh authorization required');error.status=428;
|
||||
error.code='step_up_required';throw error;
|
||||
}};
|
||||
(async()=>{{const result=await createBackgroundIssueSync({{store,fetchJson}}).flush();
|
||||
process.stdout.write(JSON.stringify({{state,result}}));}})();
|
||||
"""
|
||||
output = run_node(script)
|
||||
|
||||
assert output["state"] == {
|
||||
"authorized": [{"id": "review-op", "error": "Fresh authorization required"}],
|
||||
"failed": [], "released": [], "completed": [],
|
||||
}
|
||||
assert output["result"]["attention"] == 0
|
||||
assert output["result"]["authorization"] == 1
|
||||
assert output["result"]["receipts"] == [{
|
||||
"id": "review-op", "status": "authorization", "kind": "message",
|
||||
"route": "#/my-work/review/stackchain/web/8",
|
||||
}]
|
||||
|
||||
|
||||
def test_closed_app_sync_leaves_issue_closure_awaiting_foreground_authorization():
|
||||
records = [
|
||||
{
|
||||
|
||||
@@ -303,4 +303,4 @@ async def test_unread_update_offers_reply_mark_read_and_next_independent_of_toda
|
||||
assert '.update-reply-actions { display:grid; grid-template-columns:repeat(2,minmax(0,1fr));' in html
|
||||
assert '.update-reply-actions button { min-height:44px;' in html
|
||||
worker = (Path(__file__).parents[1] / "frontend" / "service-worker.js").read_text()
|
||||
assert "stackchain-dashboard-shell-v84" in worker
|
||||
assert "stackchain-dashboard-shell-v85" in worker
|
||||
@@ -953,6 +953,91 @@ async def test_other_high_impact_routes_require_fresh_authorization_before_mutat
|
||||
assert laptop_still_active.status_code == 200
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_consequential_review_requires_decision_and_head_bound_fresh_authorization(
|
||||
access_control, monkeypatch
|
||||
):
|
||||
calls = []
|
||||
|
||||
async def requested(repository, number):
|
||||
calls.append(("requested", repository, number))
|
||||
return True
|
||||
|
||||
async def submit(repository, number, head, decision, body):
|
||||
calls.append(("submit", repository, number, head, decision, body))
|
||||
return {"id": 91, "state": "APPROVED"}
|
||||
|
||||
monkeypatch.setattr(main, "is_requested_review", requested)
|
||||
monkeypatch.setattr(main.gitea_proxy, "submit_pull_review", submit)
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
payload = {
|
||||
"expected_head_sha": "abc123",
|
||||
"decision": "approve",
|
||||
"body": "Ready to ship.",
|
||||
}
|
||||
async with httpx.AsyncClient(transport=transport, base_url="https://test") as client:
|
||||
await client.post(
|
||||
"/api/v1/session",
|
||||
json={"access_token": "correct horse battery staple"},
|
||||
)
|
||||
headers = {
|
||||
"Origin": "https://test",
|
||||
"X-CSRF-Token": client.cookies["stackchain_csrf"],
|
||||
}
|
||||
missing = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json=payload,
|
||||
headers=headers,
|
||||
)
|
||||
wrong_grant = await fresh_grant(
|
||||
client,
|
||||
"submit_pull_review",
|
||||
"stackchain/api#7@different:approve",
|
||||
)
|
||||
mismatched = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json=payload,
|
||||
headers={**headers, "X-Step-Up-Grant": wrong_grant},
|
||||
)
|
||||
grant = await fresh_grant(
|
||||
client,
|
||||
"submit_pull_review",
|
||||
"stackchain/api#7@abc123:approve",
|
||||
)
|
||||
approved = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json=payload,
|
||||
headers={**headers, "X-Step-Up-Grant": grant},
|
||||
)
|
||||
replayed = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json=payload,
|
||||
headers={**headers, "X-Step-Up-Grant": grant},
|
||||
)
|
||||
comment = await client.post(
|
||||
"/api/v1/repos/stackchain/api/pulls/7/review",
|
||||
json={**payload, "decision": "comment"},
|
||||
headers=headers,
|
||||
)
|
||||
|
||||
assert [missing.status_code, mismatched.status_code] == [428, 428]
|
||||
assert missing.json()["detail"] == {
|
||||
"detail": "Fresh authorization required",
|
||||
"code": "step_up_required",
|
||||
"action": "submit_pull_review",
|
||||
"target": "stackchain/api#7@abc123:approve",
|
||||
}
|
||||
assert approved.status_code == 201
|
||||
assert replayed.status_code == 428
|
||||
assert comment.status_code == 201
|
||||
assert calls == [
|
||||
("requested", "stackchain/api", 7),
|
||||
("submit", "stackchain/api", 7, "abc123", "approve", "Ready to ship."),
|
||||
("requested", "stackchain/api", 7),
|
||||
("submit", "stackchain/api", 7, "abc123", "comment", "Ready to ship."),
|
||||
]
|
||||
|
||||
|
||||
@pytest.mark.anyio
|
||||
async def test_operator_can_review_and_revoke_one_remote_device(access_control):
|
||||
transport = httpx.ASGITransport(app=main.app)
|
||||
|
||||
@@ -142,6 +142,27 @@ process.stdout.write(JSON.stringify(item));
|
||||
assert output["quarantined"] is False
|
||||
|
||||
|
||||
def test_draft_inbox_exposes_held_review_as_awaiting_foreground_authorization():
|
||||
script = f"""
|
||||
const createDraftInbox = require({json.dumps(str(DRAFTS))});
|
||||
const values=new Map([['stackchain.authored-outbox.v1',JSON.stringify({{version:2,items:[{{
|
||||
id:'review-1',kind:'pull-review',repository:'stackchain/web',number:8,body:'Looks good',
|
||||
decision:'approve',expectedHeadSha:'abc',comments:[],ownerLogin:'timmy',
|
||||
status:'authorization',error:'Fresh authorization required',queuedAt:200
|
||||
}}]}})]]);
|
||||
const storage={{get length(){{return values.size}},key:i=>Array.from(values.keys())[i]||null,getItem:k=>values.get(k)||null,setItem:(k,v)=>values.set(k,v)}};
|
||||
const inbox=createDraftInbox({{storage,getCurrentLogin:()=>'timmy'}});
|
||||
process.stdout.write(JSON.stringify({{item:inbox.list()[0],partition:inbox.partition()}}));
|
||||
"""
|
||||
output = run_node(script)
|
||||
|
||||
assert output["item"]["status"] == "authorization"
|
||||
assert output["item"]["label"] == "Review awaiting authorization"
|
||||
assert output["item"]["authorization_required"] is True
|
||||
assert output["partition"]["counts"]["authorization"] == 1
|
||||
assert output["partition"]["retryable"] == []
|
||||
|
||||
|
||||
def test_draft_inbox_exposes_queued_issue_closure_as_awaiting_authorization():
|
||||
script = f"""
|
||||
const createDraftInbox = require({json.dumps(str(DRAFTS))});
|
||||
|
||||
@@ -347,5 +347,5 @@ async def test_dashboard_syncs_every_later_change_and_exposes_account_status():
|
||||
def test_later_sync_ships_atomically_in_the_offline_shell():
|
||||
source = (Path(__file__).parents[1] / "frontend" / "service-worker.js").read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/later-sync.js'" in source
|
||||
@@ -137,4 +137,4 @@ def test_markdown_work_bodies_are_mobile_safe_block_containers():
|
||||
assert ".markdown-content { min-width:0; max-width:100%; overflow-wrap:anywhere;" in css
|
||||
assert ".markdown-content pre { max-width:100%; overflow-x:auto;" in css
|
||||
assert ".markdown-content a { min-height:44px;" in css
|
||||
assert "stackchain-dashboard-shell-v84" in worker
|
||||
assert "stackchain-dashboard-shell-v85" in worker
|
||||
@@ -41,7 +41,7 @@ def test_offline_shell_contains_every_local_dashboard_runtime_asset():
|
||||
shell_assets = set(re.findall(r"BASE \+ '([^']+)'", worker.split("async function sessionCsrf", 1)[0]))
|
||||
|
||||
assert local_assets <= shell_assets, f"Offline shell is missing: {sorted(local_assets - shell_assets)}"
|
||||
assert "stackchain-dashboard-shell-v84" in worker
|
||||
assert "stackchain-dashboard-shell-v85" in worker
|
||||
|
||||
|
||||
def test_all_conversation_composers_offer_accessible_mobile_mentions():
|
||||
|
||||
@@ -4800,6 +4800,9 @@ async def test_review_attention_drafts_require_revision_instead_of_resending_sta
|
||||
assert "reviewOutbox && item.status === 'attention'" in html
|
||||
assert ">Open current review</button>" in html
|
||||
assert ">Copy feedback</button>" in html
|
||||
assert "reviewOutbox && item.status === 'authorization'" in html
|
||||
assert ">Authorize & send review</button>" in html
|
||||
assert "Review submitted and queued intent cleared." in html
|
||||
assert "item.kind === 'authored-outbox' && !reviewOutbox" in html
|
||||
|
||||
|
||||
|
||||
@@ -292,6 +292,6 @@ async def test_plan_today_wires_cancel_back_and_success_through_overlay_history(
|
||||
def test_plan_today_controller_is_available_in_the_offline_shell():
|
||||
source = SERVICE_WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/plan-today.js'" in source
|
||||
assert "BASE + 'static/plan-today-preview.js'" in source
|
||||
@@ -122,7 +122,7 @@ async function dispatchNotificationClick(route) {{
|
||||
def test_resumable_today_session_ships_in_a_new_offline_shell():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/my-work.js'" in source
|
||||
assert "BASE + 'static/dashboard.js'" in source
|
||||
assert "BASE + 'static/dashboard.css'" in source
|
||||
@@ -131,14 +131,14 @@ def test_resumable_today_session_ships_in_a_new_offline_shell():
|
||||
def test_ownership_exit_runtime_rolls_the_offline_shell_cache():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/dashboard.js'" in source
|
||||
|
||||
|
||||
def test_offline_review_next_ships_today_completion_atomically():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/today-completion.js'" in source
|
||||
assert "BASE + 'static/dashboard.js'" in source
|
||||
|
||||
@@ -146,7 +146,7 @@ def test_offline_review_next_ships_today_completion_atomically():
|
||||
def test_duplicate_aware_capture_ships_in_a_new_offline_shell():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/create-issue-sheet.js'" in source
|
||||
assert "BASE + 'static/dashboard.js'" in source
|
||||
|
||||
@@ -154,14 +154,14 @@ def test_duplicate_aware_capture_ships_in_a_new_offline_shell():
|
||||
def test_exact_later_picker_ships_atomically_in_a_new_offline_shell():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/later-picker.js'" in source
|
||||
|
||||
|
||||
def test_navigation_deadline_ships_in_a_new_shell_cache():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/dashboard.css'" in source
|
||||
assert "BASE + 'static/dashboard.js'" in source
|
||||
assert "BASE + 'static/install-app.js'" in source
|
||||
@@ -170,21 +170,21 @@ def test_navigation_deadline_ships_in_a_new_shell_cache():
|
||||
def test_today_convergence_ships_in_a_new_shell_cache():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/today-sync.js'" in source
|
||||
|
||||
|
||||
def test_mobile_search_viewport_ships_in_a_new_offline_shell():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/mobile-search-viewport.js'" in source
|
||||
|
||||
|
||||
def test_update_ownership_flow_ships_atomically_in_a_new_offline_shell():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/update-ownership.js'" in source
|
||||
|
||||
|
||||
@@ -328,6 +328,7 @@ def test_opted_in_background_sync_notifies_privately_and_receipt_tap_focuses_rou
|
||||
state.flushResult = {login:'timmy', receipts:[
|
||||
{id:'capture-1',status:'confirmed',kind:'issue',route:'#/my-work/issue/stackchain/api/44'},
|
||||
{id:'bad-1',status:'attention',kind:'message',route:'#/my-work/drafts'},
|
||||
{id:'review-1',status:'authorization',kind:'message',route:'#/my-work/drafts'},
|
||||
]};
|
||||
state.clientList = [{url:'https://forge.example/dashboard/', navigate:async function(url){ this.url=url; }, focus:async function(){ state.focused.push(this.url); }}];
|
||||
await dispatchSync('stackchain-issue-outbox-v1');
|
||||
@@ -353,6 +354,14 @@ def test_opted_in_background_sync_notifies_privately_and_receipt_tap_focuses_rou
|
||||
"data": {"route": "#/my-work/drafts"},
|
||||
},
|
||||
},
|
||||
{
|
||||
"title": "Queued review needs authorization",
|
||||
"options": {
|
||||
"body": "Tap to authorize it in the Delivery center.",
|
||||
"tag": "stackchain-delivery-review-1",
|
||||
"data": {"route": "#/my-work/drafts"},
|
||||
},
|
||||
},
|
||||
]
|
||||
assert result["focused"] == [
|
||||
"https://forge.example/dashboard/#/my-work/issue/stackchain/api/44"
|
||||
@@ -385,7 +394,7 @@ def test_one_session_bound_csrf_proof_is_reused_for_a_background_drain():
|
||||
def test_queue_today_ships_atomically_in_a_new_offline_shell():
|
||||
source = WORKER.read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/queue-today.js'" in source
|
||||
|
||||
|
||||
|
||||
@@ -221,7 +221,7 @@ async def test_today_blocker_opens_existing_preview_and_preserves_readiness_gate
|
||||
def test_readiness_runtime_is_available_in_offline_shell():
|
||||
service_worker = SERVICE_WORKER.read_text()
|
||||
|
||||
assert "const CACHE = 'stackchain-dashboard-shell-v84';" in service_worker
|
||||
assert "const CACHE = 'stackchain-dashboard-shell-v85';" in service_worker
|
||||
assert "BASE + 'static/today-readiness.js'" in service_worker
|
||||
|
||||
|
||||
|
||||
@@ -127,7 +127,7 @@ sync.enqueueConfiguration(120, {{'issue:r:1:':60}});
|
||||
def test_inflight_today_drain_ships_in_a_new_offline_shell():
|
||||
source = (Path(__file__).parents[1] / "frontend" / "service-worker.js").read_text()
|
||||
|
||||
assert "stackchain-dashboard-shell-v84" in source
|
||||
assert "stackchain-dashboard-shell-v85" in source
|
||||
assert "BASE + 'static/today-sync.js'" in source
|
||||
|
||||
|
||||
|
||||
Reference in new issue
Block a user