fix: validate merge authority before ref updates
This commit is contained in:
@@ -256,6 +256,7 @@ pub struct RepairSelectorFrom {
|
|||||||
pub workspace_id: String,
|
pub workspace_id: String,
|
||||||
pub ticket_id: String,
|
pub ticket_id: String,
|
||||||
pub selector_from: String,
|
pub selector_from: String,
|
||||||
|
pub resolved_subject_ref: String,
|
||||||
pub repaired_by: WorkerIdentity,
|
pub repaired_by: WorkerIdentity,
|
||||||
pub reason: String,
|
pub reason: String,
|
||||||
pub now: DateTime<Utc>,
|
pub now: DateTime<Utc>,
|
||||||
@@ -638,7 +639,66 @@ impl MergeRequestStore {
|
|||||||
review,
|
review,
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
pub fn validate_completion(&self, i: &CompleteMergeRequest) -> Result<(), MergeRequestError> {
|
||||||
|
let mr = self.get(&i.auth.workspace_id, &i.ticket_id)?;
|
||||||
|
self.completion_auth(&i.auth, &i.ticket_id, &mr.repository_id)?;
|
||||||
|
if let Some(existing) = mr.thread.iter().find_map(|event| match event {
|
||||||
|
MergeRequestThreadEvent::Merge(value) if value.operation_id == i.operation_id => {
|
||||||
|
Some(value)
|
||||||
|
}
|
||||||
|
_ => None,
|
||||||
|
}) {
|
||||||
|
if existing.approval_event_id == i.approval_event_id
|
||||||
|
&& existing.target_ref_before == i.target_ref_before
|
||||||
|
&& existing.target_ref_after == i.target_ref_after
|
||||||
|
{
|
||||||
|
return Ok(());
|
||||||
|
}
|
||||||
|
return Err(MergeRequestError::Conflict(
|
||||||
|
"operation fingerprint mismatch".into(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
if mr.state != MergeRequestState::Open {
|
||||||
|
return Err(MergeRequestError::Conflict(
|
||||||
|
"Merge Request is not open".into(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
let review = mr
|
||||||
|
.effective_review(&i.current_subject_ref)
|
||||||
|
.filter(|review| review.event_id == i.approval_event_id)
|
||||||
|
.ok_or_else(|| {
|
||||||
|
MergeRequestError::NotReady(
|
||||||
|
"approval is not the current effective review for the source ref".into(),
|
||||||
|
)
|
||||||
|
})?;
|
||||||
|
if review.decision != ReviewDecision::Approve {
|
||||||
|
return Err(MergeRequestError::NotReady(
|
||||||
|
"current effective review does not approve the source ref".into(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
if i.target_ref_before == i.target_ref_after {
|
||||||
|
return Err(MergeRequestError::Validation(
|
||||||
|
"target ref did not change".into(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
let state: Option<String> = self
|
||||||
|
.lock()?
|
||||||
|
.query_row(
|
||||||
|
"SELECT workflow_state FROM typed_tickets WHERE workspace_id=?1 AND ticket_id=?2",
|
||||||
|
params![mr.workspace_id, i.ticket_id],
|
||||||
|
|row| row.get(0),
|
||||||
|
)
|
||||||
|
.optional()?;
|
||||||
|
if state.as_deref() != Some("inprogress") {
|
||||||
|
return Err(MergeRequestError::Conflict(
|
||||||
|
"Ticket must be inprogress".into(),
|
||||||
|
));
|
||||||
|
}
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
pub fn complete(&self, i: CompleteMergeRequest) -> Result<MergeEvent, MergeRequestError> {
|
pub fn complete(&self, i: CompleteMergeRequest) -> Result<MergeEvent, MergeRequestError> {
|
||||||
|
self.validate_completion(&i)?;
|
||||||
let mr = self.get(&i.auth.workspace_id, &i.ticket_id)?;
|
let mr = self.get(&i.auth.workspace_id, &i.ticket_id)?;
|
||||||
self.completion_auth(&i.auth, &i.ticket_id, &mr.repository_id)?;
|
self.completion_auth(&i.auth, &i.ticket_id, &mr.repository_id)?;
|
||||||
if let Some(v) = mr.thread.iter().find_map(|x| match x {
|
if let Some(v) = mr.thread.iter().find_map(|x| match x {
|
||||||
@@ -765,12 +825,21 @@ impl MergeRequestStore {
|
|||||||
i: RepairSelectorFrom,
|
i: RepairSelectorFrom,
|
||||||
) -> Result<MergeRequest, MergeRequestError> {
|
) -> Result<MergeRequest, MergeRequestError> {
|
||||||
nonempty("selector_from", &i.selector_from)?;
|
nonempty("selector_from", &i.selector_from)?;
|
||||||
|
nonempty("resolved_subject_ref", &i.resolved_subject_ref)?;
|
||||||
let mr = self.get(&i.workspace_id, &i.ticket_id)?;
|
let mr = self.get(&i.workspace_id, &i.ticket_id)?;
|
||||||
if mr.selector_from.is_some() {
|
if mr.selector_from.is_some() {
|
||||||
return Err(MergeRequestError::Conflict(
|
return Err(MergeRequestError::Conflict(
|
||||||
"selector_from is immutable after it is set".into(),
|
"selector_from is immutable after it is set".into(),
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
let approved = mr
|
||||||
|
.effective_review(&i.resolved_subject_ref)
|
||||||
|
.is_some_and(|review| review.decision == ReviewDecision::Approve);
|
||||||
|
if !approved {
|
||||||
|
return Err(MergeRequestError::NotReady(
|
||||||
|
"selector repair must resolve to an approved thread subject".into(),
|
||||||
|
));
|
||||||
|
}
|
||||||
let mut c = self.lock()?;
|
let mut c = self.lock()?;
|
||||||
let t = c.transaction()?;
|
let t = c.transaction()?;
|
||||||
let changed=t.execute("UPDATE merge_requests SET selector_from=?3,updated_at=?4 WHERE workspace_id=?1 AND merge_request_id=?2 AND selector_from IS NULL",params![mr.workspace_id,mr.merge_request_id,i.selector_from,i.now.to_rfc3339()])?;
|
let changed=t.execute("UPDATE merge_requests SET selector_from=?3,updated_at=?4 WHERE workspace_id=?1 AND merge_request_id=?2 AND selector_from IS NULL",params![mr.workspace_id,mr.merge_request_id,i.selector_from,i.now.to_rfc3339()])?;
|
||||||
|
|||||||
@@ -304,3 +304,51 @@ fn completion_cancels_outstanding_grants_and_late_submit_fails() {
|
|||||||
MergeRequestThreadEvent::ReviewCancelled(value)
|
MergeRequestThreadEvent::ReviewCancelled(value)
|
||||||
if value.reason.contains("completed before review submission"))));
|
if value.reason.contains("completed before review submission"))));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn selector_repair_requires_and_accepts_an_approved_resolved_subject() {
|
||||||
|
let (dir, store) = fixture();
|
||||||
|
open(&store);
|
||||||
|
approve(&store, "approved-subject", "approval");
|
||||||
|
Connection::open(dir.path().join("db")).unwrap()
|
||||||
|
.execute("UPDATE merge_requests SET selector_from=NULL WHERE workspace_id='W' AND merge_request_id='MR'", [])
|
||||||
|
.unwrap();
|
||||||
|
let repaired = store
|
||||||
|
.repair_selector_from(RepairSelectorFrom {
|
||||||
|
workspace_id: "W".into(),
|
||||||
|
ticket_id: "T".into(),
|
||||||
|
selector_from: "restored-work".into(),
|
||||||
|
resolved_subject_ref: "approved-subject".into(),
|
||||||
|
repaired_by: WorkerIdentity {
|
||||||
|
runtime_id: "browser".into(),
|
||||||
|
worker_id: "user".into(),
|
||||||
|
},
|
||||||
|
reason: "confirmed migrated source".into(),
|
||||||
|
now: at(8),
|
||||||
|
})
|
||||||
|
.unwrap();
|
||||||
|
assert_eq!(repaired.selector_from.as_deref(), Some("restored-work"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn selector_repair_rejects_unapproved_resolved_subject() {
|
||||||
|
let (dir, store) = fixture();
|
||||||
|
open(&store);
|
||||||
|
approve(&store, "approved-subject", "approval");
|
||||||
|
Connection::open(dir.path().join("db")).unwrap()
|
||||||
|
.execute("UPDATE merge_requests SET selector_from=NULL WHERE workspace_id='W' AND merge_request_id='MR'", [])
|
||||||
|
.unwrap();
|
||||||
|
let result = store.repair_selector_from(RepairSelectorFrom {
|
||||||
|
workspace_id: "W".into(),
|
||||||
|
ticket_id: "T".into(),
|
||||||
|
selector_from: "wrong-work".into(),
|
||||||
|
resolved_subject_ref: "different-subject".into(),
|
||||||
|
repaired_by: WorkerIdentity {
|
||||||
|
runtime_id: "browser".into(),
|
||||||
|
worker_id: "user".into(),
|
||||||
|
},
|
||||||
|
reason: "wrong candidate".into(),
|
||||||
|
now: at(8),
|
||||||
|
});
|
||||||
|
assert!(matches!(result, Err(MergeRequestError::NotReady(_))));
|
||||||
|
}
|
||||||
|
|||||||
@@ -3877,14 +3877,17 @@ async fn scoped_repair_merge_request_selector(
|
|||||||
}
|
}
|
||||||
let store = merge_request_store(&api, &workspace_id)?;
|
let store = merge_request_store(&api, &workspace_id)?;
|
||||||
let mr = store.get(&workspace_id, &ticket_id)?;
|
let mr = store.get(&workspace_id, &ticket_id)?;
|
||||||
api.repository_reader()
|
let resolved_subject_ref = api
|
||||||
|
.repository_reader()
|
||||||
.observe_merge_target(&mr.repository_id, Some(&input.selector_from))
|
.observe_merge_target(&mr.repository_id, Some(&input.selector_from))
|
||||||
.map_err(repository_merge_evidence_error)?;
|
.map_err(repository_merge_evidence_error)?
|
||||||
|
.commit;
|
||||||
Ok(Json(store.repair_selector_from(
|
Ok(Json(store.repair_selector_from(
|
||||||
merge_request::RepairSelectorFrom {
|
merge_request::RepairSelectorFrom {
|
||||||
workspace_id,
|
workspace_id,
|
||||||
ticket_id,
|
ticket_id,
|
||||||
selector_from: input.selector_from,
|
selector_from: input.selector_from,
|
||||||
|
resolved_subject_ref,
|
||||||
repaired_by: merge_request::WorkerIdentity {
|
repaired_by: merge_request::WorkerIdentity {
|
||||||
runtime_id: "browser".into(),
|
runtime_id: "browser".into(),
|
||||||
worker_id: "authenticated-user".into(),
|
worker_id: "authenticated-user".into(),
|
||||||
@@ -4079,17 +4082,6 @@ async fn scoped_complete_merge_request(
|
|||||||
)
|
)
|
||||||
.into());
|
.into());
|
||||||
}
|
}
|
||||||
let already = observed.commit == input.target_ref_after;
|
|
||||||
if !already {
|
|
||||||
repositories
|
|
||||||
.update_merge_target(
|
|
||||||
&mr.repository_id,
|
|
||||||
&mr.selector_to,
|
|
||||||
&input.target_ref_before,
|
|
||||||
&input.target_ref_after,
|
|
||||||
)
|
|
||||||
.map_err(repository_merge_evidence_error)?
|
|
||||||
}
|
|
||||||
let completion = merge_request::CompleteMergeRequest {
|
let completion = merge_request::CompleteMergeRequest {
|
||||||
ticket_id,
|
ticket_id,
|
||||||
operation_id: input.operation_id,
|
operation_id: input.operation_id,
|
||||||
@@ -4108,6 +4100,18 @@ async fn scoped_complete_merge_request(
|
|||||||
},
|
},
|
||||||
now: Utc::now(),
|
now: Utc::now(),
|
||||||
};
|
};
|
||||||
|
store.validate_completion(&completion)?;
|
||||||
|
let already = observed.commit == input.target_ref_after;
|
||||||
|
if !already {
|
||||||
|
repositories
|
||||||
|
.update_merge_target(
|
||||||
|
&mr.repository_id,
|
||||||
|
&mr.selector_to,
|
||||||
|
&input.target_ref_before,
|
||||||
|
&input.target_ref_after,
|
||||||
|
)
|
||||||
|
.map_err(repository_merge_evidence_error)?
|
||||||
|
}
|
||||||
match store.complete(completion) {
|
match store.complete(completion) {
|
||||||
Ok(v) => Ok(Json(v)),
|
Ok(v) => Ok(Json(v)),
|
||||||
Err(e) => {
|
Err(e) => {
|
||||||
|
|||||||
Reference in New Issue
Block a user