Skip to content

Commit

Permalink
One more join for deleted groups (#108)
Browse files Browse the repository at this point in the history 
* The all memberships query needed one more join to fully account for deleted groups

* Must check group deletion both up and down the hierarchy chain. Add tests for the GetAllGroupMemberships function. Switch to the ElementsMatch assertion since the order in the slice isn't important
  • Loading branch information
@jacobsee
jacobsee authored Mar 25, 2024
1 parent b49db88 commit 5b3c14e
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 25 deletions.
14 changes: 8 additions & 6 deletions internal/dbtools/membership_enumeration.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ const (
TRUE AS direct
FROM
group_memberships
INNER JOIN groups ON groups.id = group_memberships.group_id
WHERE groups.deleted_at IS NULL
UNION ALL
SELECT
b.parent_group_id,
Expand All @@ -57,8 +59,8 @@ const (
FROM
membership_query AS a
INNER JOIN group_hierarchies AS b ON a.group_id = b.member_group_id
INNER JOIN groups ON groups.id = b.member_group_id
WHERE groups.deleted_at IS NULL
INNER JOIN groups as parentgroup ON parentgroup.id = b.parent_group_id AND parentgroup.deleted_at IS NULL
INNER JOIN groups as membergroup ON membergroup.id = b.member_group_id AND membergroup.deleted_at IS NULL
)
SELECT
group_id,
Expand Down Expand Up @@ -90,8 +92,8 @@ const (
TRUE AS direct
FROM
group_memberships
INNER JOIN groups ON groups.id = group_memberships.group_id
WHERE user_id = $1 AND groups.deleted_at IS NULL
INNER JOIN groups ON groups.id = group_memberships.group_id
WHERE user_id = $1 AND groups.deleted_at IS NULL
UNION ALL
SELECT
b.parent_group_id,
Expand All @@ -103,8 +105,8 @@ const (
FROM
membership_query AS a
INNER JOIN group_hierarchies AS b ON a.group_id = b.member_group_id
INNER JOIN groups ON groups.id = b.member_group_id
WHERE groups.deleted_at IS NULL
INNER JOIN groups as parentgroup ON parentgroup.id = b.parent_group_id AND parentgroup.deleted_at IS NULL
INNER JOIN groups as membergroup ON membergroup.id = b.member_group_id AND membergroup.deleted_at IS NULL
)
SELECT
group_id,
Expand Down
140 changes: 121 additions & 19 deletions internal/dbtools/membership_enumeration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,87 @@ func TestServerRunning(t *testing.T) {
assert.Equal(t, c, 5)
}

func TestGetAllGroupMemberships(t *testing.T) {
expect := []EnumeratedMembership{
{
UserID: "00000001-0000-0000-0000-000000000001",
GroupID: "00000002-0000-0000-0000-000000000001",
IsAdmin: true,
Direct: true,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000002",
GroupID: "00000002-0000-0000-0000-000000000002",
IsAdmin: false,
Direct: true,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000002",
GroupID: "00000002-0000-0000-0000-000000000001",
IsAdmin: false,
Direct: false,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000003",
GroupID: "00000002-0000-0000-0000-000000000003",
IsAdmin: false,
Direct: true,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000003",
GroupID: "00000002-0000-0000-0000-000000000002",
IsAdmin: false,
Direct: false,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000003",
GroupID: "00000002-0000-0000-0000-000000000001",
IsAdmin: false,
Direct: false,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000004",
GroupID: "00000002-0000-0000-0000-000000000001",
IsAdmin: false,
Direct: true,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000004",
GroupID: "00000002-0000-0000-0000-000000000003",
IsAdmin: false,
Direct: true,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000004",
GroupID: "00000002-0000-0000-0000-000000000002",
IsAdmin: false,
Direct: false,
ExpiresAt: null.Time{},
},
{
UserID: "00000001-0000-0000-0000-000000000005",
GroupID: "00000002-0000-0000-0000-000000000005",
IsAdmin: false,
Direct: true,
ExpiresAt: null.Time{},
},
}

allMemberships, err := GetAllGroupMemberships(context.TODO(), db, false)

if assert.NoError(t, err) {
assert.True(t, assert.ElementsMatch(t, allMemberships, expect))
}
}

func TestGetMembershipsForUser(t *testing.T) {
testCases := map[string][]EnumeratedMembership{
"00000001-0000-0000-0000-000000000001": {
Expand Down Expand Up @@ -127,15 +208,23 @@ func TestGetMembershipsForUser(t *testing.T) {
ExpiresAt: null.Time{},
},
},
"00000001-0000-0000-0000-000000000005": {},
"00000001-0000-0000-0000-000000000005": {
{
UserID: "00000001-0000-0000-0000-000000000005",
GroupID: "00000002-0000-0000-0000-000000000005",
IsAdmin: false,
Direct: true,
ExpiresAt: null.Time{},
},
},
}

for user, expect := range testCases {
t.Run(fmt.Sprintf("groups for user: %s", user), func(t *testing.T) {
enumeratedMemberships, err := GetMembershipsForUser(context.TODO(), db, user, false)

if assert.NoError(t, err) {
assert.True(t, assert.ObjectsAreEqualValues(enumeratedMemberships, expect))
assert.True(t, assert.ElementsMatch(t, enumeratedMemberships, expect))
}
})
}
Expand Down Expand Up @@ -212,14 +301,23 @@ func TestGetMembersOfGroup(t *testing.T) {
ExpiresAt: null.Time{},
},
},
"00000002-0000-0000-0000-000000000005": {
{
UserID: "00000001-0000-0000-0000-000000000005",
GroupID: "00000002-0000-0000-0000-000000000005",
IsAdmin: false,
Direct: true,
ExpiresAt: null.Time{},
},
},
}

for user, expect := range testCases {
t.Run(fmt.Sprintf("users for group: %s", user), func(t *testing.T) {
enumeratedMemberships, err := GetMembersOfGroup(context.TODO(), db, user, false)

if assert.NoError(t, err) {
assert.True(t, assert.ObjectsAreEqualValues(enumeratedMemberships, expect))
assert.True(t, assert.ElementsMatch(t, enumeratedMemberships, expect))
}
})
}
Expand Down Expand Up @@ -253,22 +351,24 @@ func TestHierarchyWouldCreateCycle(t *testing.T) {

// nolint:all
// Sets this up:
// ┌──────┐
// ┌───┤Group1│
// │ └─┬─┬──┘
// ▼ │ │
// ┌──────┐ │ ▼
// ┌─────────────────┬───┤Group2│ │User1
// │ │ └───┬──┘ │
// ▼ ▼ │ │
// ┌────────────────┐ ┌──────┐ ▼ │
// │Group4 (Deleted)│ │Group3│ User2 │
// └───┬────────┬───┘ └┬──┬──┘ │
// │ │ │ │ │
// ▼ ▼ │ ▼ │
// ┌──────┐ User5 │ User3 ▼
// │Group5│ └────────────► User4
// └──────┘
// ┌──────┐
// ┌───┤Group1│
// │ └─┬─┬──┘
// ▼ │ │
// ┌──────┐ │ ▼
// ┌─────────────────┬───┤Group2│ │User1
// │ │ └───┬──┘ │
// ▼ ▼ │ │
// ┌────────────────┐ ┌──────┐ ▼ │
// │Group4 (Deleted)│ │Group3│ User2 │
// └───┬────────┬───┘ └┬──┬──┘ │
// │ │ │ │ │
// ▼ │ │ ▼ │
// ┌──────┐ │ │ User3 ▼
// │Group5│ │ └────────────► User4
// └───┬──┘ │
// │ ▼
// └────► User5

func seedTestDB(db *sql.DB) error {
testData := []string{
Expand Down Expand Up @@ -306,6 +406,8 @@ func seedTestDB(db *sql.DB) error {
('00000003-0000-0000-0000-000000000005', '00000002-0000-0000-0000-000000000003', '00000001-0000-0000-0000-000000000004', 'f', '2023-07-12 12:00:00.000000+00', '2023-07-12 12:00:00.000000+00', NULL);`,
`INSERT INTO "group_memberships" ("id", "group_id", "user_id", "is_admin", "created_at", "updated_at", "expires_at") VALUES
('00000003-0000-0000-0000-000000000006', '00000002-0000-0000-0000-000000000004', '00000001-0000-0000-0000-000000000005', 'f', '2023-07-12 12:00:00.000000+00', '2023-07-12 12:00:00.000000+00', NULL);`,
`INSERT INTO "group_memberships" ("id", "group_id", "user_id", "is_admin", "created_at", "updated_at", "expires_at") VALUES
('00000003-0000-0000-0000-000000000007', '00000002-0000-0000-0000-000000000005', '00000001-0000-0000-0000-000000000005', 'f', '2023-07-12 12:00:00.000000+00', '2023-07-12 12:00:00.000000+00', NULL);`,

`INSERT INTO "group_hierarchies" ("id", "parent_group_id", "member_group_id", "created_at", "updated_at", "expires_at") VALUES
('00000004-0000-0000-0000-000000000001', '00000002-0000-0000-0000-000000000001', '00000002-0000-0000-0000-000000000002', '2023-07-12 12:00:00.000000+00', '2023-07-12 12:00:00.000000+00', NULL);`,
Expand Down

0 comments on commit 5b3c14e

Please sign in to comment.