test enterprise parse

coder · Emyrk · Sep 17, 2024 · Sep 10, 2024 · Sep 10, 2024 · Sep 10, 2024
commit 601652cd88c9ae7db3329ed9c662872b87a5185d
diff --git a/coderd/idpsync/role.go b/coderd/idpsync/role.go
@@ -214,12 +214,19 @@ func (s AGPLIDPSync) syncSiteWideRoles(ctx context.Context, tx database.Store, u
 		)
 	}
 
-	_, err := tx.UpdateUserRoles(ctx, database.UpdateUserRolesParams{
-		GrantedRoles: filtered,
-		ID:           user.ID,
-	})
-	if err != nil {
-		return xerrors.Errorf("set site wide roles: %w", err)
+	filtered = slice.Unique(filtered)
+	slices.Sort(filtered)
+
+	existing := slice.Unique(user.RBACRoles)
+	slices.Sort(existing)
+	if !slices.Equal(existing, filtered) {
+		_, err := tx.UpdateUserRoles(ctx, database.UpdateUserRolesParams{
+			GrantedRoles: filtered,
+			ID:           user.ID,
+		})
+		if err != nil {
+			return xerrors.Errorf("set site wide roles: %w", err)
+		}
 	}
 	return nil
 }

diff --git a/coderd/idpsync/role_test.go b/coderd/idpsync/role_test.go
@@ -1,17 +1,22 @@
 package idpsync_test
 
 import (
+	"context"
+	"database/sql"
+	"encoding/json"
 	"testing"
 
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/google/uuid"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	"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/dbmock"
 	"github.com/coder/coder/v2/coderd/database/dbtestutil"
 	"github.com/coder/coder/v2/coderd/idpsync"
 	"github.com/coder/coder/v2/coderd/rbac"
@@ -281,6 +286,7 @@ func TestRoleSyncTable(t *testing.T) {
 		allRoleIDs, err := allRoles.RoleNames()
 		require.NoError(t, err)
 
+		// Remove the org roles
 		siteRoles := slices.DeleteFunc(allRoleIDs, func(r rbac.RoleIdentifier) bool {
 			return r.IsOrgRole()
 		})
@@ -290,3 +296,76 @@ func TestRoleSyncTable(t *testing.T) {
 		}, siteRoles)
 	})
 }
+
+// TestNoopNoDiff verifies if no role change occurs, no database call is taken
+// per organization. This limits the number of db calls to O(1) if there
+// are no changes. Which is the usual case, as user's roles do not change often.
+func TestNoopNoDiff(t *testing.T) {
+	ctx := context.Background()
+	ctrl := gomock.NewController(t)
+	mDB := dbmock.NewMockStore(ctrl)
+
+	mgr := runtimeconfig.NewManager()
+	s := idpsync.NewAGPLSync(slogtest.Make(t, &slogtest.Options{}), mgr, idpsync.DeploymentSyncSettings{
+		SiteRoleField:    "",
+		SiteRoleMapping:  nil,
+		SiteDefaultRoles: nil,
+	})
+
+	userID := uuid.New()
+	orgID := uuid.New()
+	siteRoles := []string{rbac.RoleTemplateAdmin().Name, rbac.RoleAuditor().Name}
+	orgRoles := []string{rbac.RoleOrgAuditor(), rbac.RoleOrgAdmin()}
+	// The DB mock expects.
+	// If this test fails, feel free to add more expectations.
+	// The primary expectations to avoid is 'UpdateUserRoles'
+	// and 'UpdateMemberRoles'.
+	mDB.EXPECT().InTx(
+		gomock.Any(), gomock.Any(),
+	).DoAndReturn(func(f func(database.Store) error, _ *sql.TxOptions) error {
+		err := f(mDB)
+		return err
+	})
+
+	mDB.EXPECT().OrganizationMembers(gomock.Any(), database.OrganizationMembersParams{
+		UserID: userID,
+	}).Return([]database.OrganizationMembersRow{
+		{
+			OrganizationMember: database.OrganizationMember{
+				UserID:         userID,
+				OrganizationID: orgID,
+				Roles:          orgRoles,
+			},
+		},
+	}, nil)
+
+	mDB.EXPECT().GetRuntimeConfig(gomock.Any(), gomock.Any()).Return(
+		string(must(json.Marshal(idpsync.RoleSyncSettings{
+			Field:   "roles",
+			Mapping: nil,
+		}))), nil)
+
+	err := s.SyncRoles(ctx, mDB, database.User{
+		ID:        userID,
+		Email:     "alice@email.com",
+		Username:  "alice",
+		Status:    database.UserStatusActive,
+		RBACRoles: siteRoles,
+		LoginType: database.LoginTypePassword,
+	}, idpsync.RoleParams{
+		SyncEnabled:   true,
+		SyncSiteWide:  true,
+		SiteWideRoles: siteRoles,
+		MergedClaims: jwt.MapClaims{
+			"roles": orgRoles,
+		},
+	})
+	require.NoError(t, err)
+}
+
+func must[T any](value T, err error) T {
+	if err != nil {
+		panic(err)
+	}
+	return value
+}
diff --git a/enterprise/coderd/enidpsync/role.go b/enterprise/coderd/enidpsync/role.go
@@ -9,6 +9,7 @@ import (
 
 	"cdr.dev/slog"
 	"github.com/coder/coder/v2/coderd/idpsync"
+	"github.com/coder/coder/v2/coderd/util/slice"
 	"github.com/coder/coder/v2/codersdk"
 )
 
@@ -61,7 +62,7 @@ func (e EnterpriseIDPSync) ParseRoleClaims(ctx context.Context, mergedClaims jwt
 	return idpsync.RoleParams{
 		SyncEnabled:   e.RoleSyncEnabled(),
 		SyncSiteWide:  e.AGPLIDPSync.SiteRoleField != "",
-		SiteWideRoles: siteRoles,
+		SiteWideRoles: slice.Unique(siteRoles),
 		MergedClaims:  mergedClaims,
 	}, nil
 }
diff --git a/enterprise/coderd/enidpsync/role_test.go b/enterprise/coderd/enidpsync/role_test.go
@@ -0,0 +1,144 @@
+package enidpsync_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/golang-jwt/jwt/v4"
+	"github.com/stretchr/testify/require"
+
+	"cdr.dev/slog/sloggers/slogtest"
+	"github.com/coder/coder/v2/coderd/entitlements"
+	"github.com/coder/coder/v2/coderd/idpsync"
+	"github.com/coder/coder/v2/coderd/rbac"
+	"github.com/coder/coder/v2/coderd/runtimeconfig"
+	"github.com/coder/coder/v2/codersdk"
+	"github.com/coder/coder/v2/enterprise/coderd/enidpsync"
+)
+
+func TestEnterpriseParseRoleClaims(t *testing.T) {
+	t.Parallel()
+
+	entitled := entitlements.New()
+	entitled.Update(func(en *codersdk.Entitlements) {
+		en.Features[codersdk.FeatureUserRoleManagement] = codersdk.Feature{
+			Entitlement: codersdk.EntitlementEntitled,
+			Enabled:     true,
+		}
+	})
+
+	t.Run("NotEntitled", func(t *testing.T) {
+		t.Parallel()
+
+		mgr := runtimeconfig.NewManager()
+		s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitlements.New(), idpsync.DeploymentSyncSettings{})
+
+		params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{})
+		require.Nil(t, err)
+		require.False(t, params.SyncEnabled)
+		require.False(t, params.SyncSiteWide)
+	})
+
+	t.Run("NotEntitledButEnabled", func(t *testing.T) {
+		t.Parallel()
+		// Since it is not entitled, it should not be enabled
+
+		mgr := runtimeconfig.NewManager()
+		s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitlements.New(), idpsync.DeploymentSyncSettings{
+			SiteRoleField: "roles",
+		})
+
+		params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{})
+		require.Nil(t, err)
+		require.False(t, params.SyncEnabled)
+		require.False(t, params.SyncSiteWide)
+	})
+
+	t.Run("SiteDisabled", func(t *testing.T) {
+		t.Parallel()
+
+		mgr := runtimeconfig.NewManager()
+		s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitled, idpsync.DeploymentSyncSettings{})
+
+		params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{})
+		require.Nil(t, err)
+		require.True(t, params.SyncEnabled)
+		require.False(t, params.SyncSiteWide)
+	})
+
+	t.Run("SiteEnabled", func(t *testing.T) {
+		t.Parallel()
+
+		mgr := runtimeconfig.NewManager()
+		s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitled, idpsync.DeploymentSyncSettings{
+			SiteRoleField:    "roles",
+			SiteRoleMapping:  map[string][]string{},
+			SiteDefaultRoles: []string{rbac.RoleTemplateAdmin().Name},
+		})
+
+		params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{
+			"roles": []string{rbac.RoleAuditor().Name},
+		})
+		require.Nil(t, err)
+		require.True(t, params.SyncEnabled)
+		require.True(t, params.SyncSiteWide)
+		require.ElementsMatch(t, []string{
+			rbac.RoleTemplateAdmin().Name,
+			rbac.RoleAuditor().Name,
+		}, params.SiteWideRoles)
+	})
+
+	t.Run("SiteMapping", func(t *testing.T) {
+		t.Parallel()
+
+		mgr := runtimeconfig.NewManager()
+		s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitled, idpsync.DeploymentSyncSettings{
+			SiteRoleField: "roles",
+			SiteRoleMapping: map[string][]string{
+				"foo": {rbac.RoleAuditor().Name, rbac.RoleUserAdmin().Name},
+				"bar": {rbac.RoleOwner().Name},
+			},
+			SiteDefaultRoles: []string{rbac.RoleTemplateAdmin().Name},
+		})
+
+		params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{
+			"roles": []string{"foo", "bar", "random"},
+		})
+		require.Nil(t, err)
+		require.True(t, params.SyncEnabled)
+		require.True(t, params.SyncSiteWide)
+		require.ElementsMatch(t, []string{
+			rbac.RoleTemplateAdmin().Name,
+			rbac.RoleAuditor().Name,
+			rbac.RoleUserAdmin().Name,
+			rbac.RoleOwner().Name,
+			// Invalid claims are still passed at this point
+			"random",
+		}, params.SiteWideRoles)
+	})
+
+	t.Run("DuplicateRoles", func(t *testing.T) {
+		t.Parallel()
+
+		mgr := runtimeconfig.NewManager()
+		s := enidpsync.NewSync(slogtest.Make(t, nil), mgr, entitled, idpsync.DeploymentSyncSettings{
+			SiteRoleField: "roles",
+			SiteRoleMapping: map[string][]string{
+				"foo": {rbac.RoleOwner().Name, rbac.RoleAuditor().Name},
+				"bar": {rbac.RoleOwner().Name},
+			},
+			SiteDefaultRoles: []string{rbac.RoleAuditor().Name},
+		})
+
+		params, err := s.ParseRoleClaims(context.Background(), jwt.MapClaims{
+			"roles": []string{"foo", "bar", rbac.RoleAuditor().Name, rbac.RoleOwner().Name},
+		})
+		require.Nil(t, err)
+		require.True(t, params.SyncEnabled)
+		require.True(t, params.SyncSiteWide)
+		require.ElementsMatch(t, []string{
+			rbac.RoleAuditor().Name,
+			rbac.RoleOwner().Name,
+		}, params.SiteWideRoles)
+	})
+}