Begin unit testing work

coder · Emyrk · Sep 17, 2024 · Sep 10, 2024 · Sep 10, 2024 · Sep 10, 2024
commit 5d0f729aae82fb5f2a5e7d0567fe36d14a014db1
diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -8630,7 +8630,7 @@ func (q *FakeQuerier) UpdateUserRoles(_ context.Context, arg database.UpdateUser
 		}
 
 		// Set new roles
-		user.RBACRoles = arg.GrantedRoles
+		user.RBACRoles = slice.Unique(arg.GrantedRoles)
 		// Remove duplicates and sort
 		uniqueRoles := make([]string, 0, len(user.RBACRoles))
 		exist := make(map[string]struct{})

diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go
@@ -15,6 +15,7 @@ import (
 	"github.com/coder/coder/v2/coderd/rbac"
 	"github.com/coder/coder/v2/coderd/rbac/rolestore"
 	"github.com/coder/coder/v2/coderd/runtimeconfig"
+	"github.com/coder/coder/v2/coderd/util/slice"
 )
 
 type RoleParams struct {
@@ -159,8 +160,15 @@ func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user data
 				return s == rbac.RoleOrgMember()
 			})
 
+			// Only care about unique roles. So remove all duplicates
+			existingFound = slice.Unique(existingFound)
+			validExpected = slice.Unique(validExpected)
+			// A sort is required for the equality check
+			slices.Sort(existingFound)
+			slices.Sort(validExpected)
 			// Is there a difference between the expected roles and the existing roles?
 			if !slices.Equal(existingFound, validExpected) {
+				// TODO: Write a unit test to verify we do no db call on no diff
 				_, err = tx.UpdateMemberRoles(ctx, database.UpdateMemberRolesParams{
 					GrantedRoles: validExpected,
 					UserID:       user.ID,
@@ -189,6 +197,8 @@ func (s AGPLIDPSync) syncSiteWideRoles(ctx context.Context, tx database.Store, u
 	for _, role := range params.SiteWideRoles {
 		// Because we are only syncing site wide roles, we intentionally will always
 		// omit 'OrganizationID' from the RoleIdentifier.
+		// TODO: If custom site wide roles are introduced, this needs to use the
+		// database to verify the role exists.
 		if _, err := rbac.RoleByName(rbac.RoleIdentifier{Name: role}); err == nil {
 			filtered = append(filtered, role)
 		} else {

diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go
@@ -6,9 +6,11 @@ import (
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/slices"
 
 	"cdr.dev/slog/sloggers/slogtest"
 	"github.com/coder/coder/v2/coderd/database"
+	"github.com/coder/coder/v2/coderd/database/dbauthz"
 	"github.com/coder/coder/v2/coderd/database/dbgen"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/idpsync"
@@ -20,7 +22,6 @@ import (
 func TestRoleSyncTable(t *testing.T) {
 	t.Parallel()
 
-	// Last checked, takes 30s with postgres on a fast machine.
 	if dbtestutil.WillUsePostgres() {
 		t.Skip("Skipping test because it populates a lot of db entries, which is slow on postgres.")
 	}
@@ -33,9 +34,9 @@ func TestRoleSyncTable(t *testing.T) {
 		},
 		// bad-claim is a number, and will fail any role sync
 		"bad-claim": 100,
+		"empty":     []string{},
 	}
 
-	//ids := coderdtest.NewDeterministicUUIDGenerator()
 	testCases := []orgSetupDefinition{
 		{
 			Name:              "NoSync",
@@ -125,6 +126,62 @@ func TestRoleSyncTable(t *testing.T) {
 				},
 			},
 		},
+		{
+			Name:              "NoChange",
+			OrganizationRoles: []string{rbac.RoleOrgAdmin(), rbac.RoleOrgTemplateAdmin(), rbac.RoleOrgAuditor()},
+			RoleSettings: &idpsync.RoleSyncSettings{
+				Field: "roles",
+				Mapping: map[string][]string{
+					"foo": {rbac.RoleOrgAuditor(), rbac.RoleOrgTemplateAdmin()},
+					"bar": {rbac.RoleOrgAdmin()},
+				},
+			},
+			assertRoles: &orgRoleAssert{
+				ExpectedOrgRoles: []string{
+					rbac.RoleOrgAdmin(), rbac.RoleOrgAuditor(), rbac.RoleOrgTemplateAdmin(),
+				},
+			},
+		},
+		{
+			// InvalidOriginalRole starts the user with an invalid role.
+			// In practice, this should not happen, as it means a role was
+			// inserted into the database that does not exist.
+			// For the purposes of syncing, it does not matter, and the sync
+			// should succeed.
+			Name:              "InvalidOriginalRole",
+			OrganizationRoles: []string{"something-bad"},
+			RoleSettings: &idpsync.RoleSyncSettings{
+				Field:   "roles",
+				Mapping: map[string][]string{},
+			},
+			assertRoles: &orgRoleAssert{
+				ExpectedOrgRoles: []string{
+					rbac.RoleOrgAuditor(),
+				},
+			},
+		},
+		{
+			Name:              "NonExistentClaim",
+			OrganizationRoles: []string{rbac.RoleOrgAuditor()},
+			RoleSettings: &idpsync.RoleSyncSettings{
+				Field:   "not-exists",
+				Mapping: map[string][]string{},
+			},
+			assertRoles: &orgRoleAssert{
+				ExpectedOrgRoles: []string{},
+			},
+		},
+		{
+			Name:              "EmptyClaim",
+			OrganizationRoles: []string{rbac.RoleOrgAuditor()},
+			RoleSettings: &idpsync.RoleSyncSettings{
+				Field:   "empty",
+				Mapping: map[string][]string{},
+			},
+			assertRoles: &orgRoleAssert{
+				ExpectedOrgRoles: []string{},
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -148,7 +205,7 @@ func TestRoleSyncTable(t *testing.T) {
 			orgID := uuid.New()
 			SetupOrganization(t, s, db, user, orgID, tc)
 
-			// Do the group sync!
+			// Do the role sync!
 			err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{
 				SyncEnabled:  true,
 				SyncSiteWide: false,
@@ -164,85 +221,72 @@ func TestRoleSyncTable(t *testing.T) {
 	// deployment. This tests all organizations being synced together.
 	// The reason we do them individually, is that it is much easier to
 	// debug a single test case.
-	//t.Run("AllTogether", func(t *testing.T) {
-	//	t.Parallel()
-	//
-	//	db, _ := dbtestutil.NewDB(t)
-	//	manager := runtimeconfig.NewManager()
-	//	s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}),
-	//		manager,
-	//		// Also sync the default org!
-	//		idpsync.DeploymentSyncSettings{
-	//			GroupField: "groups",
-	//			Legacy: idpsync.DefaultOrgLegacySettings{
-	//				GroupField: "groups",
-	//				GroupMapping: map[string]string{
-	//					"foo": "legacy-foo",
-	//					"baz": "legacy-baz",
-	//				},
-	//				GroupFilter:         regexp.MustCompile("^legacy"),
-	//				CreateMissingGroups: true,
-	//			},
-	//		},
-	//	)
-	//
-	//	ctx := testutil.Context(t, testutil.WaitSuperLong)
-	//	user := dbgen.User(t, db, database.User{})
-	//
-	//	var asserts []func(t *testing.T)
-	//	// The default org is also going to do something
-	//	def := orgSetupDefinition{
-	//		Name: "DefaultOrg",
-	//		GroupNames: map[string]bool{
-	//			"legacy-foo": false,
-	//			"legacy-baz": true,
-	//			"random":     true,
-	//		},
-	//		// No settings, because they come from the deployment values
-	//		GroupSettings: nil,
-	//		assertGroups: &orgGroupAssert{
-	//			ExpectedGroupNames: []string{"legacy-foo", "legacy-baz", "legacy-bar"},
-	//		},
-	//	}
-	//
-	//	//nolint:gocritic // testing
-	//	defOrg, err := db.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx))
-	//	require.NoError(t, err)
-	//	SetupOrganization(t, s, db, user, defOrg.ID, def)
-	//	asserts = append(asserts, func(t *testing.T) {
-	//		t.Run(def.Name, func(t *testing.T) {
-	//			t.Parallel()
-	//			def.Assert(t, defOrg.ID, db, user)
-	//		})
-	//	})
-	//
-	//	for _, tc := range testCases {
-	//		tc := tc
-	//
-	//		orgID := uuid.New()
-	//		SetupOrganization(t, s, db, user, orgID, tc)
-	//		asserts = append(asserts, func(t *testing.T) {
-	//			t.Run(tc.Name, func(t *testing.T) {
-	//				t.Parallel()
-	//				tc.Assert(t, orgID, db, user)
-	//			})
-	//		})
-	//	}
-	//
-	//	asserts = append(asserts, func(t *testing.T) {
-	//		t.Helper()
-	//		def.Assert(t, defOrg.ID, db, user)
-	//	})
-	//
-	//	// Do the group sync!
-	//	err = s.SyncGroups(ctx, db, user, idpsync.GroupParams{
-	//		SyncEnabled:  true,
-	//		MergedClaims: userClaims,
-	//	})
-	//	require.NoError(t, err)
-	//
-	//	for _, assert := range asserts {
-	//		assert(t)
-	//	}
-	//})
+	t.Run("AllTogether", func(t *testing.T) {
+		t.Parallel()
+
+		db, _ := dbtestutil.NewDB(t)
+		manager := runtimeconfig.NewManager()
+		s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{
+			IgnoreErrors: true,
+		}),
+			manager,
+			// Also sync some site wide roles
+			idpsync.DeploymentSyncSettings{
+				GroupField:    "groups",
+				SiteRoleField: "roles",
+				// Site sync settings do not matter,
+				// as we are not testing the site parse here.
+				// Only the sync, assuming the parse is correct.
+			},
+		)
+
+		ctx := testutil.Context(t, testutil.WaitSuperLong)
+		user := dbgen.User(t, db, database.User{})
+
+		var asserts []func(t *testing.T)
+
+		for _, tc := range testCases {
+			tc := tc
+
+			orgID := uuid.New()
+			SetupOrganization(t, s, db, user, orgID, tc)
+			asserts = append(asserts, func(t *testing.T) {
+				t.Run(tc.Name, func(t *testing.T) {
+					t.Parallel()
+					tc.Assert(t, orgID, db, user)
+				})
+			})
+		}
+
+		err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{
+			SyncEnabled:  true,
+			SyncSiteWide: true,
+			SiteWideRoles: []string{
+				rbac.RoleTemplateAdmin().Name, // Duplicate this value to test deduplication
+				rbac.RoleTemplateAdmin().Name, rbac.RoleAuditor().Name,
+			},
+			MergedClaims: userClaims,
+		})
+		require.NoError(t, err)
+
+		for _, assert := range asserts {
+			assert(t)
+		}
+
+		// Also assert site wide roles
+		//nolint:gocritic // unit testing assertions
+		allRoles, err := db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), user.ID)
+		require.NoError(t, err)
+
+		allRoleIDs, err := allRoles.RoleNames()
+		require.NoError(t, err)
+
+		siteRoles := slices.DeleteFunc(allRoleIDs, func(r rbac.RoleIdentifier) bool {
+			return r.IsOrgRole()
+		})
+
+		require.ElementsMatch(t, []rbac.RoleIdentifier{
+			rbac.RoleTemplateAdmin(), rbac.RoleAuditor(), rbac.RoleMember(),
+		}, siteRoles)
+	})
 }