Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions packages/database/features/groupAccess.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
Feature: Group content access
User story:
* As a user of the Obsidian plugin
* Logged in through a given space's anonymous account
* I want to be able to create a group including another user outside my space
* giving that user access to my private content

Acceptance criteria:
* The second user should not have access to the content before I publish my content to the group
* The second user should have access after I publish my content to the group

Background:
Given the database is blank
And the user user1 opens the Roam plugin in space s1
And the user user2 opens the Roam plugin in space s2

Scenario Outline: Creating content
When Document are added to the database:
| $id | source_local_id | created | last_modified | _author_id | _space_id |
| d1 | ld1 | 2025/01/01 | 2025/01/01 | user1 | s1 |
And Content are added to the database:
| $id | source_local_id | _document_id | text | created | last_modified | scale | _author_id | _space_id |
| ct1 | lct1 | d1 | Claim | 2025/01/01 | 2025/01/01 | document | user1 | s1 |
Then a user logged in space s1 should see 2 PlatformAccount in the database
And a user logged in space s1 should see 1 Content in the database
And a user logged in space s2 should see 2 PlatformAccount in the database
But a user logged in space s2 should see 0 Content in the database
When user of space s1 creates group my_group
And user of space s1 adds space s2 to group my_group
Then a user logged in space s1 should see 1 Content in the database
But a user logged in space s2 should see 0 Content in the database
And ContentAccess are added to the database:
| _account_uid | _content_id |
| my_group | ct1 |
Then a user logged in space s1 should see 1 Content in the database
Then a user logged in space s2 should see 1 Content in the database
50 changes: 50 additions & 0 deletions packages/database/features/step-definitions/stepdefs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,19 @@ Given("the database is blank", async () => {
assert.equal(r.error, null);
r = await client.from("AgentIdentifier").delete().neq("account_id", -1);
assert.equal(r.error, null);
const r3 = await client.from("group_membership").select("group_id");
assert.equal(r3.error, null);
const groupIds = new Set((r3.data || []).map(({group_id})=>group_id));
for (const id of groupIds) {
const ur = await client.auth.admin.deleteUser(id);
assert.equal(ur.error, null);
}
Comment on lines +67 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Filter out null group_ids before deletion.

If group_id can be null in the group_membership table, this code will attempt to delete a user with id = null, which could cause an error or unexpected behavior.

🛡️ Proposed fix to filter nulls
  const r3 = await client.from("group_membership").select("group_id");
  assert.equal(r3.error, null);
- const groupIds = new Set((r3.data || []).map(({group_id})=>group_id));
+ const groupIds = new Set((r3.data || []).map(({group_id})=>group_id).filter(id => id !== null));
  for (const id of groupIds) {
    const ur = await client.auth.admin.deleteUser(id);
    assert.equal(ur.error, null);
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const r3 = await client.from("group_membership").select("group_id");
assert.equal(r3.error, null);
const groupIds = new Set((r3.data || []).map(({group_id})=>group_id));
for (const id of groupIds) {
const ur = await client.auth.admin.deleteUser(id);
assert.equal(ur.error, null);
}
const r3 = await client.from("group_membership").select("group_id");
assert.equal(r3.error, null);
const groupIds = new Set((r3.data || []).map(({group_id})=>group_id).filter(id => id !== null));
for (const id of groupIds) {
const ur = await client.auth.admin.deleteUser(id);
assert.equal(ur.error, null);
}
🤖 Prompt for AI Agents
In @packages/database/features/step-definitions/stepdefs.ts around lines 67 -
73, The Set of group IDs built from r3.data may include nulls, so before calling
client.auth.admin.deleteUser you should filter out null/undefined values: when
creating groupIds from (r3.data || []), only map and add entries where group_id
!== null && group_id !== undefined (or use .filter(({group_id})=>group_id !=
null) before mapping), then iterate that filtered set and call
client.auth.admin.deleteUser(id) as before and assert ur.error is null.

const r2 = await client.from("PlatformAccount").select("dg_account").not('dg_account', 'is', 'null');
assert.equal(r2.error, null);
for (const {dg_account} of r2.data || []) {
const r = await client.auth.admin.deleteUser(dg_account!);
assert.equal(r.error, null);
}
Comment on lines +74 to +79
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

Supabase PostgREST .not() method syntax null values documentation

💡 Result:

Use the raw PostgREST filter syntax. To filter "not null" with Supabase's .not(), pass operator "is" and JavaScript/SDK null value, e.g.:

  • JavaScript / TypeScript:
    supabase.from('table').select().not('column', 'is', null). [1]

  • Other SDKs follow the same idea (e.g. Dart example). [2]

Notes/caveats:

  • .not() expects PostgREST-style operators/values (e.g. use '(1,2)' for in, '{"a","b"}' for arrays). [1][2]
  • There are known PostgREST edge-cases/issues around negating IS NULL (discussion in PostgREST repo). If you encounter parsing errors, check PostgREST version/issue tracker. [3]

Sources:
[1] Supabase JavaScript docs — not().
[2] Supabase Dart docs — not().
[3] PostgREST GitHub issue re: negating IS NULL.


Remove quotes around null in the .not() filter.

The syntax .not('dg_account', 'is', 'null') is incorrect. According to Supabase PostgREST documentation, the null value should be passed as a JavaScript null, not a string: .not('dg_account', 'is', null).

🤖 Prompt for AI Agents
In @packages/database/features/step-definitions/stepdefs.ts around lines 74 -
79, The .not filter is using the string 'null' instead of the JavaScript null
which makes the Supabase/PostgREST query incorrect; update the query call on
client.from("PlatformAccount").select(...) to use .not('dg_account', 'is', null)
so the filter checks for actual NULLs, then keep the subsequent loop that calls
client.auth.admin.deleteUser(dg_account!) and the assertions as-is.

r = await client.from("PlatformAccount").delete().neq("id", -1);
assert.equal(r.error, null);
r = await client.from("Space").delete().neq("id", -1);
Expand Down Expand Up @@ -389,3 +402,40 @@ Then("query results should look like this", (table: DataTable) => {
assert.deepEqual(truncatedResults, values);
}
});

When("user of space {word} creates group {word}", async (spaceName: string, name: string)=>{
const localRefs = (world.localRefs || {}) as Record<string, number|string>;
const spaceId = localRefs[spaceName];
if (spaceId === undefined) assert.fail("spaceId");
const client = await getLoggedinDatabase(spaceId as number);
try{
const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}});
assert.equal(response.error, null);
localRefs[name] = response.data!.group_id;
} catch (error) {
console.error((error as any).actual);
throw error;
}
})
Comment on lines +406 to +419
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add null check before accessing response.data.

Line 414 uses a non-null assertion response.data!.group_id without first verifying that response.data exists. While the assertion on line 413 checks for response.error, a successful response could still have a null or undefined data field.

🛡️ Proposed fix to add data validation
  try{
    const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}});
    assert.equal(response.error, null);
-   localRefs[name] = response.data!.group_id;
+   if (!response.data?.group_id) {
+     throw new Error("create-group response missing group_id");
+   }
+   localRefs[name] = response.data.group_id;
  } catch (error) {
    console.error((error as any).actual);
    throw error;
  }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
When("user of space {word} creates group {word}", async (spaceName: string, name: string)=>{
const localRefs = (world.localRefs || {}) as Record<string, number|string>;
const spaceId = localRefs[spaceName];
if (spaceId === undefined) assert.fail("spaceId");
const client = await getLoggedinDatabase(spaceId as number);
try{
const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}});
assert.equal(response.error, null);
localRefs[name] = response.data!.group_id;
} catch (error) {
console.error((error as any).actual);
throw error;
}
})
When("user of space {word} creates group {word}", async (spaceName: string, name: string)=>{
const localRefs = (world.localRefs || {}) as Record<string, number|string>;
const spaceId = localRefs[spaceName];
if (spaceId === undefined) assert.fail("spaceId");
const client = await getLoggedinDatabase(spaceId as number);
try{
const response = await client.functions.invoke<{group_id: string}>("create-group", {body:{name}});
assert.equal(response.error, null);
if (!response.data?.group_id) {
throw new Error("create-group response missing group_id");
}
localRefs[name] = response.data.group_id;
} catch (error) {
console.error((error as any).actual);
throw error;
}
})
🤖 Prompt for AI Agents
In @packages/database/features/step-definitions/stepdefs.ts around lines 406 -
419, The handler for When("user of space {word} creates group {word}") accesses
response.data!.group_id without verifying response.data; add a null/undefined
check on response.data (and on response.data.group_id) after the invoke call and
before assigning to localRefs so you don't use a non-null assertion. If data is
missing, assert.fail or throw a clear error (e.g., referencing response.error
and response) so failures are explicit; update the assignment to localRefs[name]
only after confirming response.data.group_id exists.


When("user of space {word} adds space {word} to group {word}",
async (space1Name: string, space2Name:string, groupName: string)=>{
const localRefs = (world.localRefs || {}) as Record<string, number|string>;
const space1Id = localRefs[space1Name] as number;
const space2Id = localRefs[space2Name] as number;
const groupId = localRefs[groupName] as string;
if (space1Id === undefined) assert.fail("space1Id");
if (space2Id === undefined) assert.fail("space2Id");
if (groupId === undefined) assert.fail("groupId");
const client1 = await getLoggedinDatabase(space1Id as number);
const client2 = await getLoggedinDatabase(space2Id as number);
const r1 = await client2.from("PlatformAccount").select("dg_account").eq("account_local_id", spaceAnonUserEmail("Roam", space2Id)).maybeSingle();
assert.equal(r1.error, null);
const memberId = r1.data?.dg_account;
assert(!!memberId);
const r2 = await client1.from("group_membership").insert({
group_id: groupId,
member_id: memberId!
});
assert.equal(r2.error, null);
})