Harden the Public API member and group write endpoints

Adversarial review of the new endpoints turned up several defects.

Ownership is now out of reach of a Public API client. The internal endpoints
only let an Owner grant, change or remove Owner, and that check cannot be
applied here because there is no user behind the request. Since the
organization API key can be created by an Admin, a client could previously
promote itself to Owner and take over the organization. Granting the Owner role
and acting on an existing owner are both refused now, which subsumes the
narrower last confirmed owner guard.

An omitted groups list on a member update no longer unassigns every group.
Upstream leaves group access untouched when the field is absent and only
collections reset on omission, so groups is optional now.

An omitted accessAll on a group update no longer clears the flag, and an
omitted externalId no longer clears the directory matching key that
/public/organization/import relies on to match members and groups.

Revoked members are reported with the upstream status of -1 instead of the
internal offset encoding, matching every other serializer in the codebase.

Restore refuses a member who is not revoked, rather than saving nothing and
logging a restore that did not happen, and group creation is logged before its
collection associations so a failure cannot leave an unaudited group.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

scripts/smoke_public_api_write.sh

@@ -282,6 +282,20 @@ check_eq "update group again -> 200" "$HTTP_CODE" "200"
 req GET "/api/public/groups/$NEWGROUP/member-ids" "$TOKEN"
 jqcheck "group update left member assignments intact" 'length' "1"
 
+echo ""
+echo "== accessAll survives an update that omits it =="
+
+reqj POST "/api/public/groups" "$TOKEN" "{\"name\":\"Full Access\",\"accessAll\":true,\"collections\":[]}"
+check_eq "create an accessAll group -> 200" "$HTTP_CODE" "200"
+jqcheck "created group has accessAll" '.accessAll' "true"
+AAGROUP=$(jqval '.id')
+
+reqj PUT "/api/public/groups/$AAGROUP" "$TOKEN" "{\"name\":\"Full Access Renamed\",\"collections\":[]}"
+check_eq "rename without accessAll -> 200" "$HTTP_CODE" "200"
+jqcheck "omitted accessAll is preserved" '.accessAll' "true"
+req DELETE "/api/public/groups/$AAGROUP" "$TOKEN"
+check_eq "clean up the accessAll group -> 200" "$HTTP_CODE" "200"
+
 echo ""
 echo "== Create a member =="
 
@@ -319,10 +333,12 @@ check_eq "member with an unknown type -> 400" "$HTTP_CODE" "400"
 echo ""
 echo "== Update a member =="
 
+# An omitted groups list must leave group membership alone, and an omitted externalId
+# must not clear the directory matching key.
 reqj PUT "/api/public/members/$NEWMEMBER" "$TOKEN" \
-    "{\"type\":2,\"externalId\":\"ext-updated\",\"collections\":[{\"id\":\"$COLLECTION\",\"readOnly\":false,\"hidePasswords\":true,\"manage\":false}],\"groups\":[]}"
+    "{\"type\":2,\"collections\":[{\"id\":\"$COLLECTION\",\"readOnly\":false,\"hidePasswords\":true,\"manage\":false}]}"
 check_eq "update member -> 200" "$HTTP_CODE" "200"
-jqcheck "updated member externalId" '.externalId' "ext-updated"
+jqcheck "omitted externalId is preserved" '.externalId' "ext-new-member"
 
 req GET "/api/public/members/$NEWMEMBER" "$TOKEN"
 jqcheck "update replaced the collection grants" '.collections | length' "1"
@@ -330,7 +346,15 @@ jqcheck "updated collection readOnly" '.collections[0].readOnly' "false"
 jqcheck "updated collection hidePasswords" '.collections[0].hidePasswords' "true"
 
 req GET "/api/public/members/$NEWMEMBER/group-ids" "$TOKEN"
-jqcheck "update cleared the group assignments" 'length' "0"
+jqcheck "omitted groups leave membership alone" 'length' "1"
+
+# An explicit empty list does clear them.
+reqj PUT "/api/public/members/$NEWMEMBER" "$TOKEN" \
+    "{\"type\":2,\"externalId\":\"ext-updated\",\"collections\":[],\"groups\":[]}"
+check_eq "update member with explicit empty groups -> 200" "$HTTP_CODE" "200"
+jqcheck "explicit externalId is applied" '.externalId' "ext-updated"
+req GET "/api/public/members/$NEWMEMBER/group-ids" "$TOKEN"
+jqcheck "explicit empty groups clears membership" 'length' "0"
 
 echo ""
 echo "== Member group ids =="
@@ -355,7 +379,7 @@ echo "== Revoke and restore =="
 req POST "/api/public/members/$MEMBER3/revoke" "$TOKEN"
 check_eq "revoke a member -> 200" "$HTTP_CODE" "200"
 req GET "/api/public/members/$MEMBER3" "$TOKEN"
-jqcheck "revoked member has a revoked status" '.status < 0' "true"
+jqcheck "revoked member reports the upstream revoked status" '.status' "-1"
 
 req POST "/api/public/members/$MEMBER3/revoke" "$TOKEN"
 check_eq "revoking twice -> 400" "$HTTP_CODE" "400"
@@ -368,21 +392,32 @@ jqcheck "restored member is confirmed again" '.status' "2"
 req POST "/api/public/members/$MEMBER3/restore" "$TOKEN"
 check_eq "restoring an active member -> 400" "$HTTP_CODE" "400"
 
+req POST "/api/public/members/$NEWMEMBER/restore" "$TOKEN"
+check_eq "restoring an invited member -> 400" "$HTTP_CODE" "400"
+
 echo ""
-echo "== The last confirmed owner is protected =="
+echo "== Ownership is out of reach for a Public API client =="
+
+reqj POST "/api/public/members" "$TOKEN" "{\"email\":\"owner@example.com\",\"type\":0}"
+check_eq "creating an Owner -> 400" "$HTTP_CODE" "400"
+
+reqj PUT "/api/public/members/$MEMBER3" "$TOKEN" "{\"type\":0}"
+check_eq "promoting a member to Owner -> 400" "$HTTP_CODE" "400"
+req GET "/api/public/members/$MEMBER3" "$TOKEN"
+jqcheck "the member was not promoted" '.type' "2"
 
 reqj PUT "/api/public/members/$MEMBER" "$TOKEN" "{\"type\":2}"
-check_eq "demoting the last owner -> 400" "$HTTP_CODE" "400"
+check_eq "demoting an owner -> 400" "$HTTP_CODE" "400"
 
 req DELETE "/api/public/members/$MEMBER" "$TOKEN"
-check_eq "deleting the last owner -> 400" "$HTTP_CODE" "400"
+check_eq "deleting an owner -> 400" "$HTTP_CODE" "400"
 
 req POST "/api/public/members/$MEMBER/revoke" "$TOKEN"
-check_eq "revoking the last owner -> 400" "$HTTP_CODE" "400"
+check_eq "revoking an owner -> 400" "$HTTP_CODE" "400"
 
 req GET "/api/public/members/$MEMBER" "$TOKEN"
-jqcheck "the last owner is untouched, type" '.type' "0"
+jqcheck "the owner is untouched, type" '.type' "0"
-jqcheck "the last owner is untouched, status" '.status' "2"
+jqcheck "the owner is untouched, status" '.status' "2"
 
 echo ""
 echo "== Organization scoping boundary =="

src/api/core/public.rs

@@ -243,6 +243,15 @@ async fn member_to_json(member: &Membership, conn: &DbConn) -> Value {
         None => (Value::Null, Value::Null),
     };
 
+    // Revoked members carry their pre-revocation status offset by ACTIVATE_REVOKE_DIFF so
+    // it can be restored later. Upstream only knows -1, which is what the other serializers
+    // report too, so clamp it here rather than leaking the internal encoding.
+    let status = if member.status < MembershipStatus::Revoked as i32 {
+        MembershipStatus::Revoked as i32
+    } else {
+        member.status
+    };
+
     json!({
         "object": "member",
         "id": member.uuid,
@@ -252,7 +261,7 @@ async fn member_to_json(member: &Membership, conn: &DbConn) -> Value {
         "type": member.atype,
         "externalId": member.external_id,
         "resetPasswordEnrolled": member.reset_password_key.is_some(),
-        "status": member.status,
+        "status": status,
     })
 }
 
@@ -473,10 +482,11 @@ struct MemberCreateData {
 struct MemberUpdateData {
     r#type: NumberOrString,
     external_id: Option<String>,
+    // An omitted collections list clears the assignments, but an omitted groups list
+    // leaves them alone, matching how upstream treats the two.
     #[serde(default)]
     collections: Vec<AssociationData>,
-    #[serde(default)]
+    groups: Option<Vec<GroupId>>,
-    groups: Vec<GroupId>,
     #[serde(default)]
     permissions: HashMap<String, Value>,
 }
@@ -486,9 +496,9 @@ struct MemberUpdateData {
 struct GroupCreateUpdateData {
     name: String,
     // Upstream dropped accessAll from its group model, but the Vaultwarden group still
-    // carries the flag, so it is accepted here and defaults to false when omitted.
+    // carries the flag. It is accepted here so it stays reachable, and left untouched
-    #[serde(default)]
+    // when omitted so a client following the upstream model cannot silently clear it.
-    access_all: bool,
+    access_all: Option<bool>,
     external_id: Option<String>,
     #[serde(default)]
     collections: Vec<AssociationData>,
@@ -528,6 +538,23 @@ fn member_type_and_access_all(
     Some((new_type, access_all))
 }
 
+// The internal endpoints only let an Owner grant, change or remove Owner. A Public API
+// client has no user behind it to check that against, and the organization API key can be
+// created by an Admin, so ownership is placed out of its reach entirely.
+fn deny_owner_grant(new_type: MembershipType) -> EmptyResult {
+    if new_type == MembershipType::Owner {
+        err!("The Public API cannot grant the Owner role")
+    }
+    Ok(())
+}
+
+fn deny_owner_target(member: &Membership) -> EmptyResult {
+    if member.atype == MembershipType::Owner {
+        err!("The Public API cannot modify an organization owner")
+    }
+    Ok(())
+}
+
 async fn validate_collections(collections: &[AssociationData], org_id: &OrganizationId, conn: &DbConn) -> EmptyResult {
     let org_collections = Collection::find_by_organization(org_id, conn).await;
     let org_collection_ids: HashSet<&CollectionId> = org_collections.iter().map(|c| &c.uuid).collect();
@@ -621,6 +648,7 @@ async fn post_member(data: Json<MemberCreateData>, token: PublicToken, ip: auth:
     let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &data.permissions) else {
         err!("Invalid type")
     };
+    deny_owner_grant(new_type)?;
 
     validate_collections(&data.collections, &org_id, &conn).await?;
     validate_groups(&data.groups, &org_id, &conn).await?;
@@ -669,7 +697,11 @@ async fn post_member(data: Json<MemberCreateData>, token: PublicToken, ip: auth:
     new_member.access_all = access_all;
     new_member.atype = new_type as i32;
     new_member.status = member_status;
+    // Only touch the external id when one was sent. Clearing it on omission would break
+    // the key "/public/organization/import" matches members and groups on.
+    if data.external_id.is_some() {
         new_member.set_external_id(data.external_id.clone());
+    }
     new_member.save(&conn).await?;
 
     if CONFIG.mail_enabled()
@@ -708,34 +740,32 @@ async fn put_member(
     let Some((new_type, access_all)) = member_type_and_access_all(data.r#type, &data.permissions) else {
         err!("Invalid type")
     };
+    deny_owner_grant(new_type)?;
 
     let Some(mut member) = Membership::find_by_uuid_and_org(&member_id, &org_id, &conn).await else {
         err_code!(format!("Member {member_id} not found in organization"), 404);
     };
+    deny_owner_target(&member)?;
 
     validate_collections(&data.collections, &org_id, &conn).await?;
-    validate_groups(&data.groups, &org_id, &conn).await?;
+    if let Some(group_ids) = &data.groups {
-
+        validate_groups(group_ids, &org_id, &conn).await?;
-    if member.atype == MembershipType::Owner
-        && new_type != MembershipType::Owner
-        && member.status == MembershipStatus::Confirmed as i32
-    {
-        // Removing owner permission, check that there is at least one other confirmed owner
-        if Membership::count_confirmed_by_org_and_type(&org_id, MembershipType::Owner, &conn).await <= 1 {
-            err!("Can't delete the last owner")
-        }
     }
 
     member.access_all = access_all;
     member.atype = new_type as i32;
+    if data.external_id.is_some() {
         member.set_external_id(data.external_id.clone());
+    }
 
     // This check is also done at accept_invite, _confirm_invite, _activate_member, edit_member,
     // admin::update_membership_type. We need to perform the check after changing the type.
     OrgPolicy::check_user_allowed(&member, "modify", &conn).await?;
 
     set_member_collections(&member, &data.collections, &org_id, &conn).await?;
-    set_member_groups(&member, &data.groups, &conn).await?;
+    if let Some(group_ids) = &data.groups {
+        set_member_groups(&member, group_ids, &conn).await?;
+    }
 
     member.save(&conn).await?;
 
@@ -757,12 +787,7 @@ async fn delete_member(
         err_code!(format!("Member {member_id} not found in organization"), 404);
     };
 
-    if member.atype == MembershipType::Owner && member.status == MembershipStatus::Confirmed as i32 {
+    deny_owner_target(&member)?;
-        // Removing owner, check that there is at least one other confirmed owner
-        if Membership::count_confirmed_by_org_and_type(&org_id, MembershipType::Owner, &conn).await <= 1 {
-            err!("Can't delete the last owner")
-        }
-    }
 
     log_public_event(EventType::OrganizationUserRemoved as i32, &member.uuid, &org_id, &ip.ip, &conn).await;
 
@@ -854,16 +879,12 @@ async fn post_member_revoke(
         err_code!(format!("Member {member_id} not found in organization"), 404);
     };
 
+    deny_owner_target(&member)?;
+
     if member.status <= MembershipStatus::Revoked as i32 {
         err!("User is already revoked")
     }
 
-    if member.atype == MembershipType::Owner
-        && Membership::count_confirmed_by_org_and_type(&org_id, MembershipType::Owner, &conn).await <= 1
-    {
-        err!("Organization must have at least one confirmed owner")
-    }
-
     member.revoke();
     member.save(&conn).await?;
 
@@ -884,7 +905,11 @@ async fn post_member_restore(
         err_code!(format!("Member {member_id} not found in organization"), 404);
     };
 
-    if member.status >= MembershipStatus::Accepted as i32 {
+    deny_owner_target(&member)?;
+
+    // Anything above Revoked is already active. Testing against Accepted would let an
+    // invited member through, producing a no-op save and a restore event that never happened.
+    if member.status > MembershipStatus::Revoked as i32 {
         err!("User is already active")
     }
 
@@ -914,13 +939,14 @@ async fn post_group(
     let data = data.into_inner();
     validate_collections(&data.collections, &org_id, &conn).await?;
 
-    let mut group = Group::new(org_id.clone(), data.name.clone(), data.access_all, data.external_id.clone());
+    let mut group =
+        Group::new(org_id.clone(), data.name.clone(), data.access_all.unwrap_or(false), data.external_id.clone());
     group.save(&conn).await?;
 
-    set_group_collections(&group, &data.collections, &org_id, &conn).await?;
-
     log_public_event(EventType::GroupCreated 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)))
 }
 
@@ -945,11 +971,15 @@ async fn put_group(
     validate_collections(&data.collections, &org_id, &conn).await?;
 
     group.name.clone_from(&data.name);
-    group.access_all = data.access_all;
+    if let Some(access_all) = data.access_all {
+        group.access_all = access_all;
+    }
     // Unlike the internal endpoint, the external_id is updatable here. The Public API is
     // the directory integration surface, the same one "/public/organization/import" uses
     // to assign external ids in the first place.
+    if data.external_id.is_some() {
         group.set_external_id(data.external_id.clone());
+    }
     group.save(&conn).await?;
 
     // Member assignments are owned by "/public/groups/<group_id>/member-ids" and are