Browse Source

Fix defects found by a second adversarial review

The member permissions object could not be written back. The read shape emits
permissions null for anyone who is not a manage-all member, but the write models
declared a plain map with serde(default), which only covers a missing field: an
explicit null failed to deserialize and the request was rejected before the
handler ran. Reading a member and sending it straight back therefore failed for
every member except the manage-all case the previous test covered. The field is
optional now, and the smoke test round-trips a plain member as well.

Reinvite was the one member write that could still reach an owner, and it can
change an owner from invited to accepted, so it takes the same guard as the rest.

Group updates log before mutating, so a failure part way through the collection
associations cannot leave an unaudited change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
pull/7569/head
Rune Darrud 5 days ago
parent
commit
c9c81f3b02
  1. 7
      scripts/smoke_public_api_write.sh
  2. 20
      src/api/core/public.rs

7
scripts/smoke_public_api_write.sh

@ -421,9 +421,14 @@ req GET "/api/public/members/$MANAGEALL" "$TOKEN"
jqcheck "round-trip kept the custom role" '.type' "4"
jqcheck "round-trip kept manage-all" '.permissions.editAnyCollection' "true"
# A plain member carries no permissions object.
# A plain member carries no permissions object, and must still round-trip: the read shape
# has to be acceptable as a write body, null permissions and all.
req GET "/api/public/members/$MEMBER3" "$TOKEN"
jqcheck "a plain member has a permissions key" 'has("permissions")' "true"
jqcheck "a plain member has null permissions" '.permissions' "null"
PLAIN=$(jq -c '{type: .type, externalId: .externalId, permissions: .permissions, collections: []}' "$TMP/body")
reqj PUT "/api/public/members/$MEMBER3" "$TOKEN" "$PLAIN"
check_eq "a plain member round-trips too" "$HTTP_CODE" "200"
req DELETE "/api/public/members/$MANAGEALL" "$TOKEN"
check_eq "clean up the manage-all member -> 200" "$HTTP_CODE" "200"

20
src/api/core/public.rs

@ -500,8 +500,10 @@ struct MemberCreateData {
collections: Vec<AssociationData>,
#[serde(default)]
groups: Vec<GroupId>,
// Our own read shape emits null here for anyone who is not a manage-all member, and a
// client is expected to send a read straight back, so null has to deserialize.
#[serde(default)]
permissions: HashMap<String, Value>,
permissions: Option<HashMap<String, Value>>,
}
#[derive(Deserialize)]
@ -514,8 +516,10 @@ struct MemberUpdateData {
#[serde(default)]
collections: Vec<AssociationData>,
groups: Option<Vec<GroupId>>,
// Our own read shape emits null here for anyone who is not a manage-all member, and a
// client is expected to send a read straight back, so null has to deserialize.
#[serde(default)]
permissions: HashMap<String, Value>,
permissions: Option<HashMap<String, Value>>,
}
#[derive(Deserialize)]
@ -672,7 +676,8 @@ async fn post_member(data: Json<MemberCreateData>, token: PublicToken, ip: auth:
let org_id = token.0;
let data = data.into_inner();
let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &data.permissions) else {
let permissions = data.permissions.unwrap_or_default();
let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &permissions) else {
err!("Invalid type")
};
deny_owner_grant(new_type)?;
@ -764,7 +769,8 @@ async fn put_member(
let org_id = token.0;
let data = data.into_inner();
let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &data.permissions) else {
let permissions = data.permissions.unwrap_or_default();
let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &permissions) else {
err!("Invalid type")
};
deny_owner_grant(new_type)?;
@ -866,6 +872,8 @@ async fn post_member_reinvite(member_id: MembershipId, token: PublicToken, conn:
err_code!(format!("Member {member_id} not found in organization"), 404);
};
deny_owner_target(&member)?;
if member.status != MembershipStatus::Invited as i32 {
err!("The user is already accepted or confirmed to the organization")
}
@ -1011,10 +1019,10 @@ async fn put_group(
// Member assignments are owned by "/public/groups/<group_id>/member-ids" and are
// deliberately left untouched here.
set_group_collections(&group, &data.collections, &org_id, &conn).await?;
log_public_event(EventType::GroupUpdated as i32, &group.uuid, &org_id, &ip.ip, &conn).await;
set_group_collections(&group, &data.collections, &org_id, &conn).await?;
Ok(Json(group_to_json(&group)))
}

Loading…
Cancel
Save