diff --git a/scripts/smoke_public_api_write.sh b/scripts/smoke_public_api_write.sh index 6b988371..47db57a4 100755 --- a/scripts/smoke_public_api_write.sh +++ b/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 last owner is untouched, status" '.status' "2" +jqcheck "the owner is untouched, type" '.type' "0" +jqcheck "the owner is untouched, status" '.status' "2" echo "" echo "== Organization scoping boundary ==" diff --git a/src/api/core/public.rs b/src/api/core/public.rs index b6f091f1..d790274f 100644 --- a/src/api/core/public.rs +++ b/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, + // 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, - #[serde(default)] - groups: Vec, + groups: Option>, #[serde(default)] permissions: HashMap, } @@ -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. - #[serde(default)] - access_all: bool, + // carries the flag. It is accepted here so it stays reachable, and left untouched + // when omitted so a client following the upstream model cannot silently clear it. + access_all: Option, external_id: Option, #[serde(default)] collections: Vec, @@ -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, 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, token: PublicToken, ip: auth: new_member.access_all = access_all; new_member.atype = new_type as i32; new_member.status = member_status; - new_member.set_external_id(data.external_id.clone()); + // 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 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") - } + if let Some(group_ids) = &data.groups { + validate_groups(group_ids, &org_id, &conn).await?; } member.access_all = access_all; member.atype = new_type as i32; - member.set_external_id(data.external_id.clone()); + 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 { - // 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") - } - } + deny_owner_target(&member)?; 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. - group.set_external_id(data.external_id.clone()); + 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//member-ids" and are