Refactor MasterUserRecordController and tests to use only propagated claims

controllers/deactivation/mapper_test.go

@@ -64,7 +64,9 @@ func TestUserSignupToMasterUserRecordMapper(t *testing.T) {
 				CreationTimestamp: metav1.Now(),
 			},
 			Spec: toolchainv1alpha1.MasterUserRecordSpec{
-				UserID: "echo",
+				PropagatedClaims: toolchainv1alpha1.PropagatedClaims{
+					UserID: "echo",
+				},
 			},
 			Status: toolchainv1alpha1.MasterUserRecordStatus{},
 		}

controllers/masteruserrecord/masteruserrecord_controller.go

@@ -230,9 +230,6 @@ func (r *Reconciler) ensureUserAccount(ctx context.Context, mur *toolchainv1alph
 			// does not exist - should create
 			userAccount = newUserAccount(nsdName, mur)
 
-			// Remove this after all users have been migrated to new IdP client
-			userAccount.Spec.OriginalSub = mur.Spec.OriginalSub
-
 			if err := memberCluster.Client.Create(ctx, userAccount); err != nil {
 				return false, r.wrapErrorWithStatusUpdate(ctx, mur, r.setStatusFailed(toolchainv1alpha1.MasterUserRecordUnableToCreateUserAccountReason), err,
 					"failed to create UserAccount in the member cluster '%s'", targetCluster)
@@ -433,30 +430,16 @@ func newUserAccount(nsdName types.NamespacedName, mur *toolchainv1alpha1.MasterU
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      nsdName.Name,
 			Namespace: nsdName.Namespace,
-			Annotations: map[string]string{
-				toolchainv1alpha1.UserEmailAnnotationKey: mur.Annotations[toolchainv1alpha1.MasterUserRecordEmailAnnotationKey],
-			},
 			Labels: map[string]string{
 				toolchainv1alpha1.TierLabelKey: mur.Spec.TierName,
 			},
 		},
 		Spec: toolchainv1alpha1.UserAccountSpec{
-			UserID:           mur.Spec.UserID,
 			Disabled:         mur.Spec.Disabled,
 			PropagatedClaims: mur.Spec.PropagatedClaims,
 		},
 	}
 
-	val, found := mur.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]
-	if found && val != "" {
-		ua.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey] = val
-	}
-
-	val, found = mur.Annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey]
-	if found && val != "" {
-		ua.Annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey] = val
-	}
-
 	return ua
 }
 

controllers/masteruserrecord/masteruserrecord_controller_test.go

@@ -200,10 +200,7 @@ func TestCreateUserAccountSuccessful(t *testing.T) {
 	logf.SetLogger(zap.New(zap.UseDevMode(true)))
 	s := apiScheme(t)
 	mur := murtest.NewMasterUserRecord(t, "john",
-		murtest.WithOwnerLabel("john-123"),
-		murtest.WithAnnotation(toolchainv1alpha1.SSOUserIDAnnotationKey, "123456"),
-		murtest.WithAnnotation(toolchainv1alpha1.SSOAccountIDAnnotationKey, "987654"))
-	mur.Spec.OriginalSub = "original-sub:12345"
+		murtest.WithOwnerLabel("john-123"))
 
 	spaceBinding := spacebindingtest.NewSpaceBinding("john", "john-space", "admin", "john-123")
 	space := spacetest.NewSpace(mur.Namespace, "john-space",
@@ -233,12 +230,8 @@ func TestCreateUserAccountSuccessful(t *testing.T) {
 		Exists().
 		MatchMasterUserRecord(mur).
 		HasLabelWithValue(toolchainv1alpha1.TierLabelKey, "deactivate30").
-		HasAnnotationWithValue(toolchainv1alpha1.SSOUserIDAnnotationKey, "123456").
-		HasAnnotationWithValue(toolchainv1alpha1.SSOAccountIDAnnotationKey, "987654").
 		HasSpec(toolchainv1alpha1.UserAccountSpec{
-			UserID:           mur.Spec.UserID,
 			Disabled:         false,
-			OriginalSub:      mur.Spec.OriginalSub,
 			PropagatedClaims: mur.Spec.PropagatedClaims,
 		})
 
@@ -262,10 +255,7 @@ func TestUserAccountSynchronizeSuccessfulWhenPropagatedClaimsModified(t *testing
 	signup := commonsignup.NewUserSignup(commonsignup.WithName("ricky-123"))
 	mur := murtest.NewMasterUserRecord(t, "ricky",
 		murtest.WithOwnerLabel("ricky-123"),
-		murtest.WithAnnotation(toolchainv1alpha1.SSOUserIDAnnotationKey, "999888"),
-		murtest.WithAnnotation(toolchainv1alpha1.SSOAccountIDAnnotationKey, "777666"),
 		murtest.Finalizer("finalizer.toolchain.dev.openshift.com"))
-	mur.Spec.OriginalSub = "original-sub:ZZZZZ"
 
 	spaceBinding := spacebindingtest.NewSpaceBinding("ricky", "ricky-space", "admin", "ricky-123")
 	space := spacetest.NewSpace(mur.Namespace, "ricky-space",
@@ -302,12 +292,8 @@ func TestUserAccountSynchronizeSuccessfulWhenPropagatedClaimsModified(t *testing
 		Exists().
 		MatchMasterUserRecord(mur).
 		HasLabelWithValue(toolchainv1alpha1.TierLabelKey, "deactivate30").
-		HasAnnotationWithValue(toolchainv1alpha1.SSOUserIDAnnotationKey, "999888").
-		HasAnnotationWithValue(toolchainv1alpha1.SSOAccountIDAnnotationKey, "777666").
 		HasSpec(toolchainv1alpha1.UserAccountSpec{
-			UserID:           mur.Spec.UserID,
 			Disabled:         false,
-			OriginalSub:      mur.Spec.OriginalSub,
 			PropagatedClaims: mur.Spec.PropagatedClaims,
 		})
 
@@ -338,12 +324,8 @@ func TestUserAccountSynchronizeSuccessfulWhenPropagatedClaimsModified(t *testing
 		Exists().
 		MatchMasterUserRecord(mur).
 		HasLabelWithValue(toolchainv1alpha1.TierLabelKey, "deactivate30").
-		HasAnnotationWithValue(toolchainv1alpha1.SSOUserIDAnnotationKey, "999888").
-		HasAnnotationWithValue(toolchainv1alpha1.SSOAccountIDAnnotationKey, "777666").
 		HasSpec(toolchainv1alpha1.UserAccountSpec{
-			UserID:           mur.Spec.UserID,
 			Disabled:         false,
-			OriginalSub:      mur.Spec.OriginalSub,
 			PropagatedClaims: mur.Spec.PropagatedClaims,
 		})
 }

controllers/masteruserrecord/sync.go

@@ -41,8 +41,6 @@ func (s *Synchronizer) synchronizeSpec(ctx context.Context) (bool, error) {
 			return false, err
 		}
 		s.memberUserAcc.Spec.Disabled = s.record.Spec.Disabled
-		s.memberUserAcc.Spec.UserID = s.record.Spec.UserID
-		s.memberUserAcc.Spec.OriginalSub = s.record.Spec.OriginalSub
 		s.memberUserAcc.Spec.PropagatedClaims = s.record.Spec.PropagatedClaims
 
 		// In addition to synchronizing the spec, ensure both the tier label and email annotation are set
@@ -54,7 +52,6 @@ func (s *Synchronizer) synchronizeSpec(ctx context.Context) (bool, error) {
 		if s.memberUserAcc.Annotations == nil {
 			s.memberUserAcc.Annotations = map[string]string{}
 		}
-		s.memberUserAcc.Annotations[toolchainv1alpha1.UserEmailAnnotationKey] = s.record.Annotations[toolchainv1alpha1.MasterUserRecordEmailAnnotationKey]
 
 		err := s.memberCluster.Client.Update(ctx, s.memberUserAcc)
 		if err != nil {
@@ -69,11 +66,10 @@ func (s *Synchronizer) synchronizeSpec(ctx context.Context) (bool, error) {
 
 func (s *Synchronizer) isSynchronized() bool {
 	return s.memberUserAcc.Spec.Disabled == s.record.Spec.Disabled &&
-		s.memberUserAcc.Spec.UserID == s.record.Spec.UserID &&
 		s.memberUserAcc.Spec.PropagatedClaims == s.record.Spec.PropagatedClaims &&
-		s.memberUserAcc.Labels != nil && s.memberUserAcc.Labels[toolchainv1alpha1.TierLabelKey] == s.record.Spec.TierName &&
-		s.memberUserAcc.Annotations != nil && s.memberUserAcc.Annotations[toolchainv1alpha1.UserEmailAnnotationKey] == s.record.Annotations[toolchainv1alpha1.MasterUserRecordEmailAnnotationKey]
+		s.memberUserAcc.Labels != nil && s.memberUserAcc.Labels[toolchainv1alpha1.TierLabelKey] == s.record.Spec.TierName
 }
+
 func (s *Synchronizer) removeAccountFromStatus(ctx context.Context) error {
 	for i := range s.record.Status.UserAccounts {
 		if s.record.Status.UserAccounts[i].Cluster.Name == s.memberCluster.Name {

controllers/masteruserrecord/sync_test.go

@@ -69,30 +69,6 @@ func TestIsSynchronized(t *testing.T) {
 			assert.False(t, s.isSynchronized())
 		})
 
-		t.Run("missing email annotation", func(t *testing.T) {
-			// given
-			record, memberUserAcc := setupSynchronizerItems()
-			delete(memberUserAcc.Annotations, toolchainv1alpha1.UserEmailAnnotationKey)
-			s := Synchronizer{
-				memberUserAcc: &memberUserAcc,
-				record:        &record,
-			}
-			// when/then
-			assert.False(t, s.isSynchronized())
-		})
-
-		t.Run("email annotation does not match", func(t *testing.T) {
-			// given
-			record, memberUserAcc := setupSynchronizerItems()
-			memberUserAcc.Annotations[toolchainv1alpha1.UserEmailAnnotationKey] = "foo"
-			s := Synchronizer{
-				memberUserAcc: &memberUserAcc,
-				record:        &record,
-			}
-			// when/then
-			assert.False(t, s.isSynchronized())
-		})
-
 		t.Run("different disable", func(t *testing.T) {
 			// given
 			record, memberUserAcc := setupSynchronizerItems()
@@ -108,7 +84,7 @@ func TestIsSynchronized(t *testing.T) {
 		t.Run("different userID", func(t *testing.T) {
 			// given
 			record, memberUserAcc := setupSynchronizerItems()
-			record.Spec.UserID = "bar"
+			record.Spec.PropagatedClaims.UserID = "bar"
 			s := Synchronizer{
 				memberUserAcc: &memberUserAcc,
 				record:        &record,
@@ -130,7 +106,9 @@ func setupSynchronizerItems() (toolchainv1alpha1.MasterUserRecord, toolchainv1al
 			},
 		},
 		Spec: toolchainv1alpha1.UserAccountSpec{
-			UserID:   "foo",
+			PropagatedClaims: toolchainv1alpha1.PropagatedClaims{
+				UserID: "foo",
+			},
 			Disabled: false,
 		},
 	}
@@ -141,7 +119,9 @@ func setupSynchronizerItems() (toolchainv1alpha1.MasterUserRecord, toolchainv1al
 			},
 		},
 		Spec: toolchainv1alpha1.MasterUserRecordSpec{
-			UserID:   "foo",
+			PropagatedClaims: toolchainv1alpha1.PropagatedClaims{
+				UserID: "foo",
+			},
 			Disabled: false,
 			TierName: "base1ns",
 		},
@@ -178,31 +158,6 @@ func TestSynchronizeSpec(t *testing.T) {
 		HasConditions(toBeNotReady(toolchainv1alpha1.MasterUserRecordUpdatingReason, ""))
 }
 
-func TestSynchronizeAnnotation(t *testing.T) {
-	// given
-	apiScheme(t)
-	mur := murtest.NewMasterUserRecord(t, "john", murtest.StatusCondition(toBeProvisioned()))
-
-	userAccount := uatest.NewUserAccountFromMur(mur)
-	userAccount.Annotations = nil
-
-	hostClient := test.NewFakeClient(t, mur)
-	sync, memberClient := prepareSynchronizer(t, userAccount, mur, hostClient)
-
-	// when
-	createdOrUpdated, err := sync.synchronizeSpec(context.TODO())
-
-	// then
-	require.NoError(t, err)
-	assert.True(t, createdOrUpdated)
-	uatest.AssertThatUserAccount(t, "john", memberClient).
-		Exists().
-		MatchMasterUserRecord(mur)
-
-	murtest.AssertThatMasterUserRecord(t, "john", hostClient).
-		HasConditions(toBeNotReady(toolchainv1alpha1.MasterUserRecordUpdatingReason, ""))
-}
-
 func TestSynchronizeStatus(t *testing.T) {
 	// given
 	apiScheme(t)

controllers/usersignup/masteruserrecord.go

@@ -9,12 +9,6 @@ import (
 func migrateOrFixMurIfNecessary(mur *toolchainv1alpha1.MasterUserRecord, defaultTier *toolchainv1alpha1.UserTier, userSignup *toolchainv1alpha1.UserSignup) bool {
 	changed := false
 
-	// TODO remove this after all users migrated to new SSO Provider client that does not modify the original subject
-	if mur.Spec.OriginalSub != userSignup.Spec.OriginalSub {
-		mur.Spec.OriginalSub = userSignup.Spec.OriginalSub
-		changed = true
-	}
-
 	// set tierName to default if not set
 	if mur.Spec.TierName == "" {
 		mur.Spec.TierName = defaultTier.Name
@@ -38,9 +32,7 @@ func newMasterUserRecord(userSignup *toolchainv1alpha1.UserSignup, targetCluster
 	labels := map[string]string{
 		toolchainv1alpha1.MasterUserRecordOwnerLabelKey: userSignup.Name,
 	}
-	annotations := map[string]string{
-		toolchainv1alpha1.MasterUserRecordEmailAnnotationKey: userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey],
-	}
+	annotations := map[string]string{}
 	if skipValue, present := userSignup.Annotations[toolchainv1alpha1.SkipAutoCreateSpaceAnnotationKey]; present {
 		annotations[toolchainv1alpha1.SkipAutoCreateSpaceAnnotationKey] = skipValue
 	}
@@ -54,22 +46,10 @@ func newMasterUserRecord(userSignup *toolchainv1alpha1.UserSignup, targetCluster
 		},
 		Spec: toolchainv1alpha1.MasterUserRecordSpec{
 			UserAccounts:     userAccounts,
-			UserID:           userSignup.Spec.Userid,
-			OriginalSub:      userSignup.Spec.OriginalSub,
 			TierName:         userTierName,
 			PropagatedClaims: userSignup.Spec.IdentityClaims.PropagatedClaims,
 		},
 	}
 
-	val, found := userSignup.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]
-	if found && val != "" {
-		annotations[toolchainv1alpha1.SSOUserIDAnnotationKey] = val
-	}
-
-	val, found = userSignup.Annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey]
-	if found && val != "" {
-		annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey] = val
-	}
-
 	return mur
 }

controllers/usersignup/masteruserrecord_test.go

@@ -22,9 +22,7 @@ func TestNewMasterUserRecord(t *testing.T) {
 	// then
 	expectedMUR := commonmur.NewMasterUserRecord(t, "johny",
 		commonmur.WithOwnerLabel(userSignup.Name),
-		commonmur.TierName("deactivate90"),
-		commonmur.UserID("UserID123"),
-		commonmur.WithAnnotation("toolchain.dev.openshift.com/user-email", "[email protected]"))
+		commonmur.TierName("deactivate90"))
 
 	expectedMUR.Spec.PropagatedClaims = userSignup.Spec.IdentityClaims.PropagatedClaims
 
@@ -43,7 +41,6 @@ func TestNewMasterUserRecordWhenSpaceCreationIsSkipped(t *testing.T) {
 		commonmur.WithOwnerLabel(userSignup.Name),
 		commonmur.TierName("deactivate90"),
 		commonmur.UserID("UserID123"),
-		commonmur.WithAnnotation("toolchain.dev.openshift.com/user-email", "[email protected]"),
 		commonmur.WithAnnotation("toolchain.dev.openshift.com/skip-auto-create-space", "true"))
 
 	expectedMUR.Spec.PropagatedClaims = userSignup.Spec.IdentityClaims.PropagatedClaims
@@ -68,9 +65,7 @@ func TestMigrateMurIfNecessary(t *testing.T) {
 			assert.False(t, changed)
 			expectedMUR := commonmur.NewMasterUserRecord(t, "johny",
 				commonmur.WithOwnerLabel(userSignup.Name),
-				commonmur.TierName("deactivate90"),
-				commonmur.UserID("UserID123"),
-				commonmur.WithAnnotation("toolchain.dev.openshift.com/user-email", "[email protected]"))
+				commonmur.TierName("deactivate90"))
 			expectedMUR.Spec.PropagatedClaims = userSignup.Spec.IdentityClaims.PropagatedClaims
 			assert.Equal(t, expectedMUR, mur)
 		})
@@ -90,9 +85,7 @@ func TestMigrateMurIfNecessary(t *testing.T) {
 			assert.True(t, changed)
 			expectedMUR := commonmur.NewMasterUserRecord(t, "johny",
 				commonmur.WithOwnerLabel(userSignup.Name),
-				commonmur.TierName("deactivate90"),
-				commonmur.UserID("UserID123"),
-				commonmur.WithAnnotation("toolchain.dev.openshift.com/user-email", "[email protected]"))
+				commonmur.TierName("deactivate90"))
 			expectedMUR.Spec.PropagatedClaims = userSignup.Spec.IdentityClaims.PropagatedClaims
 			assert.Equal(t, expectedMUR, mur)
 		})