From 7d755ce604a7331afbbdb1ce9631458aae075db6 Mon Sep 17 00:00:00 2001 From: binwiederhier Date: Sat, 18 Nov 2023 21:50:01 -0500 Subject: [PATCH] Add comments and another test to ACL fix --- user/manager.go | 6 ++++-- user/manager_test.go | 18 +++++++++++++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/user/manager.go b/user/manager.go index 60d8c392..9f54625f 100644 --- a/user/manager.go +++ b/user/manager.go @@ -833,8 +833,10 @@ func (a *Manager) Authorize(user *User, topic string, perm Permission) error { if user != nil { username = user.Name } - // Select the read/write permissions for this user/topic combo. The query may return two - // rows (one for everyone, and one for the user), but prioritizes the user. + // Select the read/write permissions for this user/topic combo. + // - The query may return two rows (one for everyone, and one for the user), but prioritizes the user. + // - Furthermore, the query prioritizes more specific permissions (longer!) over more generic ones, e.g. "test*" > "*" + // - It also prioritizes write permissions over read permissions rows, err := a.db.Query(selectTopicPermsQuery, Everyone, username, topic) if err != nil { return err diff --git a/user/manager_test.go b/user/manager_test.go index f85d2b33..e9a95b0f 100644 --- a/user/manager_test.go +++ b/user/manager_test.go @@ -98,7 +98,7 @@ func TestManager_FullScenario_Default_DenyAll(t *testing.T) { require.Nil(t, a.Authorize(ben, "announcements", PermissionRead)) require.Equal(t, ErrUnauthorized, a.Authorize(ben, "announcements", PermissionWrite)) - // user john should have + // User john should have // "deny" to mytopic_deny*, // "ro" to mytopic_ro*, // "rw" to mytopic*, @@ -129,6 +129,22 @@ func TestManager_FullScenario_Default_DenyAll(t *testing.T) { require.Nil(t, a.Authorize(nil, "up5678", PermissionWrite)) } +func TestManager_Access_Order_LengthWriteRead(t *testing.T) { + // This test validates issue #914 / #917, i.e. that write permissions are prioritized over read permissions, + // and longer ACL rules are prioritized as well. + + a := newTestManagerFromFile(t, filepath.Join(t.TempDir(), "user.db"), "", PermissionDenyAll, DefaultUserPasswordBcryptCost, DefaultUserStatsQueueWriterInterval) + require.Nil(t, a.AddUser("ben", "ben", RoleUser)) + require.Nil(t, a.AllowAccess("ben", "test*", PermissionReadWrite)) + require.Nil(t, a.AllowAccess("ben", "*", PermissionRead)) + + ben, err := a.Authenticate("ben", "ben") + require.Nil(t, err) + require.Nil(t, a.Authorize(ben, "any-topic-can-be-read", PermissionRead)) + require.Nil(t, a.Authorize(ben, "this-too", PermissionRead)) + require.Nil(t, a.Authorize(ben, "test123", PermissionWrite)) +} + func TestManager_AddUser_Invalid(t *testing.T) { a := newTestManager(t, PermissionDenyAll) require.Equal(t, ErrInvalidArgument, a.AddUser(" invalid ", "pass", RoleAdmin))