mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
fix(channels): restore member invitations to private channels (#5493)
## Summary - restore private-channel invitations for every active member - keep owner/admin-only enforcement for elevated role grants, active role changes, and removals - preserve #4612's unrelated Desktop/mobile failure handling and hardening - add relay coverage for the ordinary actor/target role matrix (`member`, `guest`, `bot`) ## Validation - pre-push hook passed on `7de700e17642ad7e10155f9537033168d9249268`: branch skew, Desktop checks/typecheck/tests/Tauri checks, mobile tests, and Rust tests - `cargo test -p buzz-test-client --test e2e_relay --no-run` - `cargo fmt --all -- --check` - `git diff --check` - Donut and Mongo independently reviewed the cross-layer authorization behavior; Donut's role-matrix coverage finding is addressed in this revision --------- Signed-off-by: Wes <wesbillman@users.noreply.github.com> Co-authored-by: Carl <c7ebe626f000404285d3686e1dc74cc07cc60a9754a150041ba132e14bd3e2ec@buzz.block.builderlab.xyz>
This commit is contained in:
@@ -39,7 +39,7 @@ The relay enforces all access control. Channel membership is the only gate.
|
||||
| Type | Visibility | Join | Create |
|
||||
|------|-----------|------|--------|
|
||||
| **Open channels** | Searchable by all members | Self-join | Any member |
|
||||
| **Private channels** | Hidden, invite-only | Invited by an owner/admin | Any member |
|
||||
| **Private channels** | Hidden, invite-only | Invited by member | Any member |
|
||||
| **DMs** | Participants only | N/A (up to 9) | Any member |
|
||||
| **Guests** | Scoped to specific channels | Invited | N/A |
|
||||
|
||||
|
||||
@@ -371,9 +371,9 @@ async fn acquire_channel_membership_lock(
|
||||
/// Role enforcement:
|
||||
/// - Open channels: `invited_by` is optional; role is forced to `Member` regardless of
|
||||
/// what the caller passes — callers cannot self-assign elevated roles.
|
||||
/// - Private channels: requires an `invited_by` who is an active owner/admin, the channel
|
||||
/// creator bootstrapping their own first membership, or the target adding themselves
|
||||
/// (idempotent re-add — an active member's *role* still cannot change this way).
|
||||
/// - Private channels: requires an `invited_by` who is an active member, or the channel
|
||||
/// creator bootstrapping their own first membership. Any active member may add an
|
||||
/// ordinary member, guest, or bot; only owners/admins may grant elevated roles.
|
||||
/// - Elevated roles (`Owner`, `Admin`) may only be granted by an existing owner/admin,
|
||||
/// even on open channels.
|
||||
///
|
||||
@@ -421,14 +421,12 @@ pub async fn add_member(
|
||||
DbError::InvalidData(format!("invalid role in database: {inviter_role_str}"))
|
||||
})?;
|
||||
|
||||
// Only owners/admins may extend private-channel access to another
|
||||
// identity. `inviter == pubkey` keeps a member's own idempotent
|
||||
// re-add working; it is not a role-escalation hole, because the
|
||||
// active-role-change guard below still rejects a self-targeted
|
||||
// promotion from any non-elevated caller.
|
||||
if !inviter_role.is_elevated() && inviter != pubkey {
|
||||
// Any active member may extend private-channel access with an
|
||||
// ordinary role. Granting owner/admin remains reserved for an
|
||||
// existing owner/admin.
|
||||
if role.is_elevated() && !inviter_role.is_elevated() {
|
||||
return Err(DbError::AccessDenied(
|
||||
"only owners/admins may add private-channel members".to_string(),
|
||||
"only owners/admins may grant elevated roles".to_string(),
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -358,23 +358,22 @@ pub async fn validate_admin_event(
|
||||
let target_pubkey =
|
||||
extract_p_tag(event).ok_or_else(|| anyhow::anyhow!("missing p tag"))?;
|
||||
|
||||
// PUT_USER: open channels allow any authenticated user. Private
|
||||
// channels only let owners/admins add another identity; otherwise
|
||||
// any compromised member could extend access to channel history.
|
||||
//
|
||||
// A self-targeted add skips this check so an idempotent re-add
|
||||
// still works. That is not a way into a private channel: ingest's
|
||||
// `check_channel_membership` rejects a non-member (and a
|
||||
// soft-removed member) before this validator runs, and `add_member`
|
||||
// independently requires the self-inviter to hold an active role.
|
||||
// Self-promotion is caught by the role-change guard below.
|
||||
if channel.visibility == "private"
|
||||
&& target_pubkey != actor_bytes
|
||||
&& !actor_role.is_some_and(|r| r.is_elevated())
|
||||
{
|
||||
return Err(anyhow::anyhow!(
|
||||
"only owners/admins may add private-channel members"
|
||||
));
|
||||
// PUT_USER: open channels allow any authenticated user; private channels
|
||||
// require the actor to be an existing active member. Any active member may
|
||||
// add an ordinary member, guest, or bot, but only owners/admins may grant
|
||||
// an elevated role.
|
||||
if channel.visibility == "private" {
|
||||
if actor_role.is_none() {
|
||||
return Err(anyhow::anyhow!("actor not authorized"));
|
||||
}
|
||||
|
||||
if requested_role.is_some_and(|role| role.is_elevated())
|
||||
&& !actor_role.is_some_and(|role| role.is_elevated())
|
||||
{
|
||||
return Err(anyhow::anyhow!(
|
||||
"only owners/admins may grant elevated roles"
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
// Changing an ACTIVE existing member's role is privileged in both
|
||||
|
||||
@@ -2249,14 +2249,17 @@ async fn add_member_with_role_ws(
|
||||
(ok.accepted, ok.message)
|
||||
}
|
||||
|
||||
/// Only owners/admins can add another identity to a private channel.
|
||||
/// Any active member can add any ordinary role to a private channel.
|
||||
#[tokio::test]
|
||||
#[ignore]
|
||||
async fn test_private_channel_member_cannot_invite() {
|
||||
async fn test_private_channel_any_member_can_invite() {
|
||||
let url = relay_url();
|
||||
let owner_keys = Keys::generate();
|
||||
let member_keys = Keys::generate();
|
||||
let invitee_keys = Keys::generate();
|
||||
let actors = [
|
||||
("member", Keys::generate()),
|
||||
("guest", Keys::generate()),
|
||||
("bot", Keys::generate()),
|
||||
];
|
||||
|
||||
// Connect as owner and create a private channel.
|
||||
let mut owner_client = BuzzTestClient::connect(&url, &owner_keys)
|
||||
@@ -2264,54 +2267,70 @@ async fn test_private_channel_member_cannot_invite() {
|
||||
.expect("connect as owner");
|
||||
let channel_id = create_private_channel_ws(&mut owner_client, &owner_keys).await;
|
||||
|
||||
// Owner adds member_keys as a regular member.
|
||||
let (accepted, msg) = add_member_ws(
|
||||
&mut owner_client,
|
||||
&channel_id,
|
||||
&member_keys.public_key().to_hex(),
|
||||
&owner_keys,
|
||||
)
|
||||
.await;
|
||||
assert!(accepted, "owner should add member, got: {msg}");
|
||||
// Seed one actor for each ordinary active role.
|
||||
for (role, keys) in &actors {
|
||||
let (accepted, msg) = add_member_with_role_ws(
|
||||
&mut owner_client,
|
||||
&channel_id,
|
||||
&keys.public_key().to_hex(),
|
||||
role,
|
||||
&owner_keys,
|
||||
)
|
||||
.await;
|
||||
assert!(accepted, "owner should add {role} actor, got: {msg}");
|
||||
}
|
||||
|
||||
// Connect as the regular member.
|
||||
let mut member_client = BuzzTestClient::connect(&url, &member_keys)
|
||||
.await
|
||||
.expect("connect as member");
|
||||
// Exercise the full ordinary-role target matrix. Relay and DB authorization
|
||||
// both run here, unlike the Desktop/mobile policy-unit-test mirrors.
|
||||
for (actor_role, actor_keys) in &actors {
|
||||
let mut actor_client = BuzzTestClient::connect(&url, actor_keys)
|
||||
.await
|
||||
.unwrap_or_else(|err| panic!("connect as {actor_role}: {err}"));
|
||||
|
||||
// Regular member tries to invite a third user.
|
||||
let (accepted, msg) = add_member_ws(
|
||||
&mut member_client,
|
||||
&channel_id,
|
||||
&invitee_keys.public_key().to_hex(),
|
||||
&member_keys,
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
!accepted,
|
||||
"regular member must not add another private-channel identity: {msg}"
|
||||
);
|
||||
assert!(
|
||||
msg.contains("owners/admins"),
|
||||
"rejection should name the owner/admin requirement, got: {msg}"
|
||||
);
|
||||
for target_role in ["member", "guest", "bot"] {
|
||||
let target_keys = Keys::generate();
|
||||
let target_pubkey_hex = target_keys.public_key().to_hex();
|
||||
let (accepted, msg) = add_member_with_role_ws(
|
||||
&mut actor_client,
|
||||
&channel_id,
|
||||
&target_pubkey_hex,
|
||||
target_role,
|
||||
actor_keys,
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
accepted,
|
||||
"private-channel {actor_role} should add {target_role}, got: {msg}"
|
||||
);
|
||||
assert_eq!(
|
||||
member_role(&url, &owner_keys, &channel_id, &target_pubkey_hex).await,
|
||||
Some(target_role.to_string()),
|
||||
"private-channel {actor_role} add must persist the {target_role} role"
|
||||
);
|
||||
}
|
||||
|
||||
// The same member re-adding *themselves* stays idempotent — the huddle
|
||||
// bot-add and kind:9021 paths depend on a self-targeted PUT_USER working.
|
||||
let (accepted, msg) = add_member_ws(
|
||||
&mut member_client,
|
||||
&channel_id,
|
||||
&member_keys.public_key().to_hex(),
|
||||
&member_keys,
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
accepted,
|
||||
"self-targeted re-add must stay idempotent, got: {msg}"
|
||||
);
|
||||
// Re-adding oneself stays idempotent — the huddle bot-add and kind:9021
|
||||
// paths depend on a self-targeted PUT_USER working.
|
||||
let (accepted, msg) = add_member_with_role_ws(
|
||||
&mut actor_client,
|
||||
&channel_id,
|
||||
&actor_keys.public_key().to_hex(),
|
||||
actor_role,
|
||||
actor_keys,
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
accepted,
|
||||
"self-targeted {actor_role} re-add must stay idempotent, got: {msg}"
|
||||
);
|
||||
|
||||
actor_client
|
||||
.disconnect()
|
||||
.await
|
||||
.unwrap_or_else(|err| panic!("disconnect {actor_role}: {err}"));
|
||||
}
|
||||
|
||||
owner_client.disconnect().await.expect("disconnect owner");
|
||||
member_client.disconnect().await.expect("disconnect member");
|
||||
}
|
||||
|
||||
/// An admin — not just the owner — can still add to a private channel.
|
||||
|
||||
@@ -22,8 +22,8 @@ test("open channels accept adds from anyone, member or not", () => {
|
||||
);
|
||||
});
|
||||
|
||||
test("private channels accept adds only from owners/admins", () => {
|
||||
for (const selfRole of ["owner", "admin"]) {
|
||||
test("private channels accept adds from every active member role", () => {
|
||||
for (const selfRole of ["owner", "admin", "member", "bot", "guest"]) {
|
||||
assert.equal(
|
||||
canAddChannelMembers({
|
||||
channelType: "stream",
|
||||
@@ -35,17 +35,15 @@ test("private channels accept adds only from owners/admins", () => {
|
||||
);
|
||||
}
|
||||
|
||||
for (const selfRole of ["member", "bot", "guest", null]) {
|
||||
assert.equal(
|
||||
canAddChannelMembers({
|
||||
channelType: "stream",
|
||||
visibility: "private",
|
||||
selfRole,
|
||||
}),
|
||||
false,
|
||||
`${selfRole} must not be able to add`,
|
||||
);
|
||||
}
|
||||
assert.equal(
|
||||
canAddChannelMembers({
|
||||
channelType: "stream",
|
||||
visibility: "private",
|
||||
selfRole: null,
|
||||
}),
|
||||
false,
|
||||
"a non-member must not be able to add",
|
||||
);
|
||||
});
|
||||
|
||||
test("DMs never accept adds, even from an owner", () => {
|
||||
@@ -67,13 +65,13 @@ test("DMs never accept adds, even from an owner", () => {
|
||||
);
|
||||
});
|
||||
|
||||
test("unknown visibility fails closed for non-elevated callers", () => {
|
||||
test("unknown visibility fails closed", () => {
|
||||
assert.equal(
|
||||
canAddChannelMembers({ channelType: "stream", selfRole: "member" }),
|
||||
false,
|
||||
);
|
||||
assert.equal(
|
||||
canAddChannelMembers({ channelType: "stream", selfRole: "owner" }),
|
||||
true,
|
||||
false,
|
||||
);
|
||||
});
|
||||
|
||||
@@ -4,9 +4,8 @@
|
||||
*
|
||||
* - DMs: nobody — membership is fixed at creation.
|
||||
* - Open channels: anyone, member or not.
|
||||
* - Private channels: owners/admins only. A plain member extending access to
|
||||
* channel history is exactly what the relay now rejects, so the affordance
|
||||
* must not be offered.
|
||||
* - Private channels: any active member. The relay separately reserves elevated
|
||||
* role grants and active-role changes for owners/admins.
|
||||
*
|
||||
* Unknown visibility fails closed — the relay is the authority and a hidden
|
||||
* button is cheaper than an opaque rejection.
|
||||
@@ -28,9 +27,13 @@ export function canAddChannelMembers({
|
||||
return true;
|
||||
}
|
||||
|
||||
return selfRole === "owner" || selfRole === "admin";
|
||||
if (visibility === "private") {
|
||||
return selfRole != null;
|
||||
}
|
||||
|
||||
return false;
|
||||
}
|
||||
|
||||
/** Explains a denied add so the user isn't left guessing at a missing button. */
|
||||
export const PRIVATE_CHANNEL_ADD_DENIED_MESSAGE =
|
||||
"Only channel owners and admins can add people to a private channel.";
|
||||
"Only channel members can add people to a private channel.";
|
||||
|
||||
@@ -251,8 +251,8 @@ export function MembersSidebar({
|
||||
visibility: channel?.visibility,
|
||||
selfRole: selfMember?.role,
|
||||
});
|
||||
// Distinguish "you can't add here" from "nothing to add" so a plain member of
|
||||
// a private channel gets the reason instead of a silently missing affordance.
|
||||
// Distinguish "you can't add here" from "nothing to add" so a non-member
|
||||
// viewing a private channel gets the reason instead of a silently missing affordance.
|
||||
const showPrivateAddDeniedNotice =
|
||||
!canAddMembers &&
|
||||
selfMember !== null &&
|
||||
|
||||
@@ -3828,7 +3828,7 @@ test("members sidebar retains distinct same-persona managed agents", async ({
|
||||
await expect(page.getByText("Pinky", { exact: true })).toHaveCount(2);
|
||||
});
|
||||
|
||||
test("private-channel members cannot add people without owner/admin", async ({
|
||||
test("private-channel members can add people and managed agents without admin", async ({
|
||||
page,
|
||||
}) => {
|
||||
await installMockBridge(page, {
|
||||
@@ -3842,26 +3842,22 @@ test("private-channel members cannot add people without owner/admin", async ({
|
||||
});
|
||||
await page.goto("/");
|
||||
// secret-projects is a private (non-DM) channel where the current user is a
|
||||
// plain member. The relay rejects their kind:9000, so the affordance is
|
||||
// withheld and the reason shown instead of failing after the fact.
|
||||
// plain member. Active members may add ordinary members and bots; only
|
||||
// elevated-role grants and role changes require owner/admin authority.
|
||||
await openMembersSidebar(page, "secret-projects");
|
||||
|
||||
await expect(page.getByTestId("members-sidebar-add-denied")).toBeVisible();
|
||||
// The field stays, but only as a filter over existing members.
|
||||
await expect(page.getByTestId("members-sidebar-add-denied")).toHaveCount(0);
|
||||
await expect(
|
||||
page.getByTestId("channel-management-search-users"),
|
||||
).toHaveAttribute("placeholder", "Search people and agents");
|
||||
).toHaveAttribute("placeholder", "Add people and agents");
|
||||
|
||||
await page.getByTestId("channel-management-search-users").fill("char");
|
||||
await expect(page.getByText("Not in this channel")).toHaveCount(0);
|
||||
await expect(
|
||||
page.getByTestId(
|
||||
`channel-user-search-result-${TEST_IDENTITIES.charlie.pubkey}`,
|
||||
),
|
||||
).toHaveCount(0);
|
||||
await page
|
||||
.getByTestId(`channel-user-search-result-${TEST_IDENTITIES.charlie.pubkey}`)
|
||||
.click();
|
||||
await expect(
|
||||
page.getByTestId(`sidebar-member-${TEST_IDENTITIES.charlie.pubkey}`),
|
||||
).toHaveCount(0);
|
||||
).toContainText("charlie");
|
||||
});
|
||||
|
||||
test("open-channel members can add people and managed agents without admin", async ({
|
||||
|
||||
@@ -5,7 +5,7 @@ const Object _sentinel = Object();
|
||||
/// Shown when a private-channel add is refused, so a missing Invite action
|
||||
/// reads as a rule rather than a bug.
|
||||
const privateChannelAddDeniedMessage =
|
||||
'Only channel owners and admins can add people to a private channel.';
|
||||
'Only channel members can add people to a private channel.';
|
||||
|
||||
@immutable
|
||||
class Channel {
|
||||
@@ -85,12 +85,13 @@ class Channel {
|
||||
|
||||
/// Whether [selfRole] may add *another* identity here, mirroring the relay's
|
||||
/// kind:9000 authority (`validate_admin_event` + `add_member`): DMs never,
|
||||
/// open channels always, private channels owners/admins only. An unknown
|
||||
/// visibility fails closed — the relay is the authority.
|
||||
/// open channels always, private channels for any active member. Elevated
|
||||
/// grants remain reserved for owners/admins. Unknown visibility fails closed.
|
||||
bool canAddMembers(String? selfRole) {
|
||||
if (isDm) return false;
|
||||
if (visibility == 'open') return true;
|
||||
return selfRole == 'owner' || selfRole == 'admin';
|
||||
if (visibility == 'private') return selfRole != null;
|
||||
return false;
|
||||
}
|
||||
|
||||
bool get isArchived => archivedAt != null;
|
||||
|
||||
@@ -279,7 +279,7 @@ Future<_NonMemberAddOutcome> _addMentionedNonMembers(
|
||||
];
|
||||
if (pending.isEmpty) return _NonMemberAddOutcome.empty;
|
||||
|
||||
// A plain member of a private channel cannot add anyone: skip the doomed
|
||||
// A non-member cannot add anyone to a private channel: skip the doomed
|
||||
// kind:9000 rather than trading it for a relay rejection.
|
||||
if (!canAddMembers) {
|
||||
return _NonMemberAddOutcome(
|
||||
|
||||
@@ -215,12 +215,13 @@ void main() {
|
||||
expect(channel.canAddMembers('member'), isTrue);
|
||||
});
|
||||
|
||||
test('private channels accept adds only from owners/admins', () {
|
||||
test('private channels accept adds from any active member role', () {
|
||||
final channel = make(channelType: 'stream', visibility: 'private');
|
||||
expect(channel.canAddMembers('owner'), isTrue);
|
||||
expect(channel.canAddMembers('admin'), isTrue);
|
||||
expect(channel.canAddMembers('member'), isFalse);
|
||||
expect(channel.canAddMembers('bot'), isFalse);
|
||||
expect(channel.canAddMembers('member'), isTrue);
|
||||
expect(channel.canAddMembers('bot'), isTrue);
|
||||
expect(channel.canAddMembers('guest'), isTrue);
|
||||
expect(channel.canAddMembers(null), isFalse);
|
||||
});
|
||||
|
||||
@@ -235,10 +236,10 @@ void main() {
|
||||
);
|
||||
});
|
||||
|
||||
test('unknown visibility fails closed for non-elevated callers', () {
|
||||
test('unknown visibility fails closed', () {
|
||||
final channel = make(channelType: 'stream', visibility: 'mystery');
|
||||
expect(channel.canAddMembers('member'), isFalse);
|
||||
expect(channel.canAddMembers('owner'), isTrue);
|
||||
expect(channel.canAddMembers('owner'), isFalse);
|
||||
});
|
||||
});
|
||||
}
|
||||
|
||||
@@ -3139,7 +3139,7 @@ void main() {
|
||||
});
|
||||
|
||||
testWidgets(
|
||||
'skips the agent add in a private channel when not owner/admin',
|
||||
'adds the agent in a private channel when the sender is a plain member',
|
||||
(tester) async {
|
||||
final agentPubkey = 'a' * 64;
|
||||
final signer = nostr.Keys.generate();
|
||||
@@ -3152,8 +3152,8 @@ void main() {
|
||||
_buildComposeBar(
|
||||
uploadService: _testUploadService(signer.nsec),
|
||||
currentPubkey: signer.public,
|
||||
// Plain member of a private channel: the relay rejects any add, so
|
||||
// the composer must not attempt one — and must still send.
|
||||
// Plain member of a private channel: ordinary member and bot
|
||||
// additions are permitted; elevated-role grants still are not.
|
||||
members: [
|
||||
ChannelMember(
|
||||
pubkey: signer.public,
|
||||
@@ -3201,15 +3201,14 @@ void main() {
|
||||
expect(didSend, isTrue);
|
||||
expect(
|
||||
publishedEvents.where((event) => event['kind'] == 9000),
|
||||
isEmpty,
|
||||
hasLength(1),
|
||||
);
|
||||
// The un-added agent is demoted from p-tag to a reference mention.
|
||||
expect(sentMentionPubkeys, isEmpty);
|
||||
expect(sentMentionPubkeys, contains(agentPubkey));
|
||||
expect(
|
||||
sentMediaTags,
|
||||
contains(orderedEquals(['mention', agentPubkey])),
|
||||
isNot(contains(orderedEquals(['mention', agentPubkey]))),
|
||||
);
|
||||
expect(find.text(privateChannelAddDeniedMessage), findsOneWidget);
|
||||
expect(find.text(privateChannelAddDeniedMessage), findsNothing);
|
||||
},
|
||||
);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user