From d965acf502ba992eb91577ec12e34f8798794ff8 Mon Sep 17 00:00:00 2001 From: Jeff Mahoney Date: Tue, 8 Nov 2022 16:10:17 -0500 Subject: [PATCH 1/3] users: ensure baseline user state is correct Every test case calls self.makeUsers but nothing checks to ensure that the initial state is correct. --- users/users_test.go | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/users/users_test.go b/users/users_test.go index 1e0beed5e85..533a06ee7de 100644 --- a/users/users_test.go +++ b/users/users_test.go @@ -3,11 +3,15 @@ package users_test import ( "testing" + "github.com/Velocidex/ordereddict" + "github.com/sebdah/goldie" "github.com/stretchr/testify/suite" api_proto "www.velocidex.com/golang/velociraptor/api/proto" "www.velocidex.com/golang/velociraptor/file_store/test_utils" + "www.velocidex.com/golang/velociraptor/json" "www.velocidex.com/golang/velociraptor/services" "www.velocidex.com/golang/velociraptor/services/orgs" + "www.velocidex.com/golang/velociraptor/users" "www.velocidex.com/golang/velociraptor/vtesting/assert" ) @@ -53,6 +57,43 @@ func (self *UserManagerTestSuite) makeUsers() { self.makeUserWithRoles("UserO2", "O2", "reader") } +// The rest of the tests depend on this state being correct. Make sure it is. +func (self *UserManagerTestSuite) TestMakeUsers() { + self.makeUsers() + + golden := ordereddict.NewDict() + + user_record, err := users.GetUser(self.Ctx, "OrgAdmin", "OrgAdmin") + assert.NoError(self.T(), err) + golden.Set("OrgAdmin OrgAdmin", user_record) + + user_record, err = users.GetUser(self.Ctx, "OrgAdmin", "UserO1") + assert.NoError(self.T(), err) + golden.Set("OrgAdmin UserO1", user_record) + + user_record, err = users.GetUser(self.Ctx, "AdminO1", "UserO1") + assert.NoError(self.T(), err) + golden.Set("AdminO1 UserO1", user_record) + + user_record, err = users.GetUser(self.Ctx, "AdminO2", "UserO1") + assert.ErrorContains(self.T(), err, "PermissionDenied") + golden.Set("AdminO2 UserO1", err.Error()) + + user_record, err = users.GetUser(self.Ctx, "OrgAdmin", "UserO2") + assert.NoError(self.T(), err) + golden.Set("OrgAdmin UserO2", user_record) + + user_record, err = users.GetUser(self.Ctx, "AdminO2", "UserO2") + assert.NoError(self.T(), err) + golden.Set("AdminO2 UserO2", user_record) + + user_record, err = users.GetUser(self.Ctx, "AdminO1", "UserO2") + assert.ErrorContains(self.T(), err, "PermissionDenied") + golden.Set("AdminO1 UserO2", err.Error()) + + goldie.Assert(self.T(), "TestMakeUsers", json.MustMarshalIndent(golden)) +} + func TestUserManger(t *testing.T) { orgs.NonceForTest = "Nonce" From a69b1985c0dd62f0ffaffcf5ec3ee32680775c45 Mon Sep 17 00:00:00 2001 From: Jeff Mahoney Date: Tue, 8 Nov 2022 16:10:47 -0500 Subject: [PATCH 2/3] users: extend DeleteUser testcase to ensure org membership was dropped The TestDeleteUser testcase takes a user which is a member of one org, adds it to another, removes it from the first, and checks to see if it still exists and remains in the second org. It never checks to see if the membership in the first org was actually completed. --- users/delete_test.go | 27 +++++++++++++++++++++-- users/fixtures/TestDeleteUser.golden | 32 ++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/users/delete_test.go b/users/delete_test.go index c95ced4969e..0c9ee6e8658 100644 --- a/users/delete_test.go +++ b/users/delete_test.go @@ -42,9 +42,20 @@ func (self *UserManagerTestSuite) TestDeleteUser() { "OrgAdmin", "UserO2", []string{"O1", "O2"}, reader_policy) assert.NoError(self.T(), err) + // Lookup using ORG_ADMIN user_record, err = users.GetUser(self.Ctx, "OrgAdmin", "UserO2") assert.NoError(self.T(), err) - golden.Set("UserO2 is in O1 and O2", user_record) + golden.Set("OrgAdmin UserO2 is in O1 and O2", user_record) + + // Lookup using O1's SERVER_ADMIN + user_record, err = users.GetUser(self.Ctx, "AdminO1", "UserO2") + assert.NoError(self.T(), err) + golden.Set("AdminO1 UserO2 is in O1", user_record) + + // Lookup using O2's SERVER_ADMIN + user_record, err = users.GetUser(self.Ctx, "AdminO2", "UserO2") + assert.NoError(self.T(), err) + golden.Set("AdminO2 UserO2 is in O2", user_record) // AdminO2 will remove the user from all orgs, but they remain in // O1 because AdminO2 has no accesss to O1 @@ -52,9 +63,21 @@ func (self *UserManagerTestSuite) TestDeleteUser() { self.Ctx, "AdminO2", "UserO2", users.LIST_ALL_ORGS) assert.NoError(self.T(), err) + // GetUser returns PermissionDenied if the user requesting does + // not have OrgAdmin and does not belong to any of the same orgs + user_record, err = users.GetUser(self.Ctx, "AdminO2", "UserO2") + assert.ErrorContains(self.T(), err, "PermissionDenied") + golden.Set("AdminO2 UserO2 removed from O2", err.Error()) + + // If the user was added to O1 and removed from O2, it should + // still exist in O1 + user_record, err = users.GetUser(self.Ctx, "AdminO1", "UserO2") + assert.NoError(self.T(), err) + golden.Set("AdminO1 UserO2 still in O1", user_record) + user_record, err = users.GetUser(self.Ctx, "OrgAdmin", "UserO2") assert.NoError(self.T(), err) - golden.Set("UserO2 removed from O2", user_record) + golden.Set("OrgAdmin UserO2 removed from O2", user_record) goldie.Assert(self.T(), "TestDeleteUser", json.MustMarshalIndent(golden)) diff --git a/users/fixtures/TestDeleteUser.golden b/users/fixtures/TestDeleteUser.golden index 7df19a9048a..5678e32e602 100644 --- a/users/fixtures/TestDeleteUser.golden +++ b/users/fixtures/TestDeleteUser.golden @@ -8,7 +8,7 @@ } ] }, - "UserO2 is in O1 and O2": { + "OrgAdmin UserO2 is in O1 and O2": { "name": "UserO2", "orgs": [ { @@ -21,7 +21,35 @@ } ] }, - "UserO2 removed from O2": { + "AdminO1 UserO2 is in O1": { + "name": "UserO2", + "orgs": [ + { + "name": "O1", + "id": "O1" + } + ] + }, + "AdminO2 UserO2 is in O2": { + "name": "UserO2", + "orgs": [ + { + "name": "O2", + "id": "O2" + } + ] + }, + "AdminO2 UserO2 removed from O2": "PermissionDenied", + "AdminO1 UserO2 still in O1": { + "name": "UserO2", + "orgs": [ + { + "name": "O1", + "id": "O1" + } + ] + }, + "OrgAdmin UserO2 removed from O2": { "name": "UserO2", "orgs": [ { From 9e99399e2f31480695edb4a889a15e1b647f0eeb Mon Sep 17 00:00:00 2001 From: Mike Cohen Date: Tue, 12 Sep 2023 13:23:16 +1000 Subject: [PATCH 3/3] Fixed tests --- users/fixtures/TestMakeUsers.golden | 8 ++++++++ users/users_test.go | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/users/fixtures/TestMakeUsers.golden b/users/fixtures/TestMakeUsers.golden index 4b4d1749641..76f569dd56c 100644 --- a/users/fixtures/TestMakeUsers.golden +++ b/users/fixtures/TestMakeUsers.golden @@ -5,6 +5,14 @@ { "name": "\u003croot\u003e", "id": "root" + }, + { + "name": "O1", + "id": "O1" + }, + { + "name": "O2", + "id": "O2" } ] }, diff --git a/users/users_test.go b/users/users_test.go index 533a06ee7de..0e214d98afe 100644 --- a/users/users_test.go +++ b/users/users_test.go @@ -19,6 +19,14 @@ type UserManagerTestSuite struct { test_utils.TestSuite } +func (self *UserManagerTestSuite) SetupTest() { + self.TestSuite.SetupTest() + + self.LoadArtifacts(`name: Server.Audit.Logs +type: SERVER_EVENT +`) +} + func (self *UserManagerTestSuite) makeUserWithRoles(username, org_id, role string) { org_manager, err := services.GetOrgManager() assert.NoError(self.T(), err)