From 435016f49c6fe5c870386e9b2ef7aa9afd6762f5 Mon Sep 17 00:00:00 2001 From: Shane Bryzak Date: Thu, 23 Nov 2023 12:48:09 +1000 Subject: [PATCH] refactored member operator to only use properties from propagated claims --- .../useraccount/useraccount_controller.go | 66 ++++++++----------- .../useraccount_controller_test.go | 62 +++++++++-------- 2 files changed, 63 insertions(+), 65 deletions(-) diff --git a/controllers/useraccount/useraccount_controller.go b/controllers/useraccount/useraccount_controller.go index 05ae129b..7ac54f48 100644 --- a/controllers/useraccount/useraccount_controller.go +++ b/controllers/useraccount/useraccount_controller.go @@ -237,19 +237,17 @@ func (r *Reconciler) ensureUser(ctx context.Context, config membercfg.Configurat } // ensure mapping - expectedIdentities := []string{commonidentity.NewIdentityNamingStandard(userAcc.Spec.UserID, config.Auth().Idp()).IdentityName()} + expectedIdentities := []string{commonidentity.NewIdentityNamingStandard(userAcc.Spec.PropagatedClaims.Sub, config.Auth().Idp()).IdentityName()} // If the OriginalSub property has been set also, then an additional identity is required to be created - if userAcc.Spec.OriginalSub != "" { - expectedIdentities = append(expectedIdentities, commonidentity.NewIdentityNamingStandard(userAcc.Spec.OriginalSub, config.Auth().Idp()).IdentityName()) + if userAcc.Spec.PropagatedClaims.OriginalSub != "" { + expectedIdentities = append(expectedIdentities, commonidentity.NewIdentityNamingStandard(userAcc.Spec.PropagatedClaims.OriginalSub, config.Auth().Idp()).IdentityName()) } // Also if the sso-user-id annotation is set, then another additional identity is required if it is a different value to the Spec.UserID - if val, ok := userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]; ok { - if val != userAcc.Spec.UserID { - expectedIdentities = append(expectedIdentities, commonidentity.NewIdentityNamingStandard( - userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey], config.Auth().Idp()).IdentityName()) - } + if userAcc.Spec.PropagatedClaims.UserID != "" && userAcc.Spec.PropagatedClaims.UserID != userAcc.Spec.UserID { + expectedIdentities = append(expectedIdentities, commonidentity.NewIdentityNamingStandard( + userAcc.Spec.PropagatedClaims.UserID, config.Auth().Idp()).IdentityName()) } stringSlicesEqual := func(a, b []string) bool { @@ -290,23 +288,22 @@ func (r *Reconciler) ensureIdentity(ctx context.Context, config membercfg.Config } // Check if the OriginalSub property is set, and if it is create additional identity/s as required - if userAcc.Spec.OriginalSub != "" { + if userAcc.Spec.PropagatedClaims.OriginalSub != "" { // Encoded the OriginalSub as an unpadded Base64 value - _, createdOrUpdated, err := r.loadIdentityAndEnsureMapping(ctx, config, userAcc.Spec.OriginalSub, userAcc, user) + _, createdOrUpdated, err := r.loadIdentityAndEnsureMapping(ctx, config, userAcc.Spec.PropagatedClaims.OriginalSub, userAcc, user) if createdOrUpdated || err != nil { return nil, createdOrUpdated, err } } - // Check if the sso-user-id annotation is set, and if it is create an additional identity if it is a different value. - // So we always have an identity with the name generated out of SSO UserID (stored as sso_userid annotation) in addition to the identity with the name generated out of the SSO Token sub claim (stored as UserAccount.Spec.UserID). + // Check if the userID property is set, and if it is create an additional identity if it is a different value. + // So we always have an identity with the name generated out of SSO UserID (stored in the userID property) in + // addition to the identity with the name generated out of the SSO Token sub claim (stored as UserAccount.Spec.PropagatedClaims.Sub). // This additional Identity is not created if the SSO UserID == SSO Token sub claim. - if val, ok := userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]; ok { - if val != userAcc.Spec.UserID { - _, createdOrUpdated, err := r.loadIdentityAndEnsureMapping(ctx, config, val, userAcc, user) - if createdOrUpdated || err != nil { - return nil, createdOrUpdated, err - } + if userAcc.Spec.PropagatedClaims.UserID != "" && userAcc.Spec.PropagatedClaims.UserID != userAcc.Spec.PropagatedClaims.Sub { + _, createdOrUpdated, err := r.loadIdentityAndEnsureMapping(ctx, config, userAcc.Spec.PropagatedClaims.UserID, userAcc, user) + if createdOrUpdated || err != nil { + return nil, createdOrUpdated, err } } @@ -404,21 +401,16 @@ func setLabelsAndAnnotations(object metav1.Object, userAcc *toolchainv1alpha1.Us annotations = object.GetAnnotations() set := false - userID, ok := userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey] - if ok { - accountID, ok := userAcc.Annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey] - // *IF* both userID and accountID properties are set, then set them on the User resource, otherwise clear - // the values if they exist - if ok { - set = true - if annotations == nil { - annotations = map[string]string{} - } - annotations[toolchainv1alpha1.SSOUserIDAnnotationKey] = userID - annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey] = accountID - object.SetAnnotations(annotations) - changed = true + if userAcc.Spec.PropagatedClaims.UserID != "" && userAcc.Spec.PropagatedClaims.AccountID != "" { + set = true + if annotations == nil { + annotations = map[string]string{} } + annotations[toolchainv1alpha1.SSOUserIDAnnotationKey] = userAcc.Spec.PropagatedClaims.UserID + annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey] = userAcc.Spec.PropagatedClaims.AccountID + object.SetAnnotations(annotations) + changed = true + } // Delete the UserID and AccountID annotations if they don't exist in the UserAccount @@ -635,13 +627,11 @@ func (r *Reconciler) updateStatusConditions(ctx context.Context, userAcc *toolch func newUser(userAcc *toolchainv1alpha1.UserAccount, config membercfg.Configuration) *userv1.User { identities := []string{commonidentity.NewIdentityNamingStandard(userAcc.Spec.UserID, config.Auth().Idp()).IdentityName()} - if userAcc.Spec.OriginalSub != "" { - identities = append(identities, commonidentity.NewIdentityNamingStandard(userAcc.Spec.OriginalSub, config.Auth().Idp()).IdentityName()) + if userAcc.Spec.PropagatedClaims.OriginalSub != "" { + identities = append(identities, commonidentity.NewIdentityNamingStandard(userAcc.Spec.PropagatedClaims.OriginalSub, config.Auth().Idp()).IdentityName()) } - if val, ok := userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]; ok { - if val != userAcc.Spec.UserID { - identities = append(identities, commonidentity.NewIdentityNamingStandard(userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey], config.Auth().Idp()).IdentityName()) - } + if userAcc.Spec.PropagatedClaims.UserID != "" && userAcc.Spec.PropagatedClaims.UserID != userAcc.Spec.PropagatedClaims.OriginalSub { + identities = append(identities, commonidentity.NewIdentityNamingStandard(userAcc.Spec.PropagatedClaims.UserID, config.Auth().Idp()).IdentityName()) } user := &userv1.User{ diff --git a/controllers/useraccount/useraccount_controller_test.go b/controllers/useraccount/useraccount_controller_test.go index fa767220..e3387df4 100644 --- a/controllers/useraccount/useraccount_controller_test.go +++ b/controllers/useraccount/useraccount_controller_test.go @@ -52,7 +52,7 @@ func TestReconcile(t *testing.T) { userUID := types.UID(username + "user") preexistingIdentity := &userv1.Identity{ ObjectMeta: metav1.ObjectMeta{ - Name: ToIdentityName(userAcc.Spec.UserID, config.Auth().Idp()), + Name: ToIdentityName(userAcc.Spec.PropagatedClaims.Sub, config.Auth().Idp()), UID: types.UID(userAcc.Name + "identity"), Labels: map[string]string{ toolchainv1alpha1.OwnerLabelKey: username, @@ -67,7 +67,7 @@ func TestReconcile(t *testing.T) { preexistingIdentityForSsoUserAnnotation := &userv1.Identity{ ObjectMeta: metav1.ObjectMeta{ - Name: ToIdentityName(userAcc.ObjectMeta.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey], config.Auth().Idp()), + Name: ToIdentityName(userAcc.Spec.PropagatedClaims.UserID, config.Auth().Idp()), UID: types.UID(userAcc.Name + "identity"), Labels: map[string]string{ "toolchain.dev.openshift.com/owner": username, @@ -92,8 +92,8 @@ func TestReconcile(t *testing.T) { }, }, Identities: []string{ - ToIdentityName(userAcc.Spec.UserID, config.Auth().Idp()), - ToIdentityName(userAcc.ObjectMeta.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey], config.Auth().Idp()), + ToIdentityName(userAcc.Spec.PropagatedClaims.Sub, config.Auth().Idp()), + ToIdentityName(userAcc.Spec.PropagatedClaims.UserID, config.Auth().Idp()), }, } @@ -142,9 +142,9 @@ func TestReconcile(t *testing.T) { // Check the identity is not created yet assertIdentityNotFound(t, r, userAcc, config.Auth().Idp()) - t.Run("test missing UserID annotation propagates to User", func(t *testing.T) { - // Remove the UserID annotation from the UserAccount and reconcile again - delete(userAcc.Annotations, toolchainv1alpha1.SSOUserIDAnnotationKey) + t.Run("test missing UserID property propagates to User", func(t *testing.T) { + // Remove the UserID from the UserAccount and reconcile again + userAcc.Spec.PropagatedClaims.UserID = "" r, req, _, _ = prepareReconcile(t, username, userAcc) //when res, err = r.Reconcile(context.TODO(), req) @@ -157,8 +157,8 @@ func TestReconcile(t *testing.T) { require.NotContains(t, user.Annotations, toolchainv1alpha1.SSOUserIDAnnotationKey) require.NotContains(t, user.Annotations, toolchainv1alpha1.SSOAccountIDAnnotationKey) - t.Run("reset UserID annotation and reconcile again", func(t *testing.T) { - userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey] = "123456" + t.Run("reset UserID and reconcile again", func(t *testing.T) { + userAcc.Spec.PropagatedClaims.UserID = "123456" r, req, _, _ = prepareReconcile(t, username, userAcc) //when res, err = r.Reconcile(context.TODO(), req) @@ -173,7 +173,7 @@ func TestReconcile(t *testing.T) { t.Run("test missing AccountID annotation propagates to User", func(t *testing.T) { // Remove the AccountID annotation from the UserAccount and reconcile again - delete(userAcc.Annotations, toolchainv1alpha1.SSOAccountIDAnnotationKey) + userAcc.Spec.PropagatedClaims.AccountID = "" r, req, _, _ = prepareReconcile(t, username, userAcc) //when res, err = r.Reconcile(context.TODO(), req) @@ -187,7 +187,7 @@ func TestReconcile(t *testing.T) { require.NotContains(t, user.Annotations, toolchainv1alpha1.SSOAccountIDAnnotationKey) t.Run("reset AccountID annotation and reconcile again", func(t *testing.T) { - userAcc.Annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey] = "987654" + userAcc.Spec.PropagatedClaims.AccountID = "987654" r, req, _, _ = prepareReconcile(t, username, userAcc) //when res, err = r.Reconcile(context.TODO(), req) @@ -385,7 +385,7 @@ func TestReconcile(t *testing.T) { HasConditions(provisioning()) // Check the created identity - identity := assertIdentityForUserID(t, r, userAcc, userAcc.ObjectMeta.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey], config.Auth().Idp()) + identity := assertIdentityForUserID(t, r, userAcc, userAcc.Spec.PropagatedClaims.UserID, config.Auth().Idp()) // Check the user identity mapping checkMapping(t, preexistingUser, preexistingIdentity, identity) @@ -862,8 +862,8 @@ func TestReconcile(t *testing.T) { // Test UserAccount with OriginalSub property set // TODO remove this test after migration is complete t.Run("create or update identities from OriginalSub OK", func(t *testing.T) { - userAcc := newUserAccount(username, userAcc.ObjectMeta.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]) // Sub Claim == SSO UserID - userAcc.Spec.OriginalSub = fmt.Sprintf("original-sub:%s", userID) + userAcc := newUserAccount(username, userAcc.Spec.PropagatedClaims.UserID) // Sub Claim == SSO UserID + userAcc.Spec.PropagatedClaims.OriginalSub = fmt.Sprintf("original-sub:%s", userID) t.Run("create user identity mapping", func(t *testing.T) { r, req, _, _ := prepareReconcile(t, username, userAcc, preexistingUser) @@ -911,10 +911,11 @@ func TestReconcile(t *testing.T) { // Check the second created/updated identity identity2 := &userv1.Identity{} err = r.Client.Get(context.TODO(), types.NamespacedName{Name: ToIdentityName( - fmt.Sprintf("b64:%s", base64.RawStdEncoding.EncodeToString([]byte(userAcc.Spec.OriginalSub))), + fmt.Sprintf("b64:%s", base64.RawStdEncoding.EncodeToString([]byte(userAcc.Spec.PropagatedClaims.OriginalSub))), config.Auth().Idp())}, identity2) require.NoError(t, err) - assert.Equal(t, fmt.Sprintf("%s:b64:%s", config.Auth().Idp(), base64.RawStdEncoding.EncodeToString([]byte(userAcc.Spec.OriginalSub))), identity2.Name) + assert.Equal(t, fmt.Sprintf("%s:b64:%s", config.Auth().Idp(), base64.RawStdEncoding. + EncodeToString([]byte(userAcc.Spec.PropagatedClaims.OriginalSub))), identity2.Name) require.Equal(t, userAcc.Name, identity2.Labels[toolchainv1alpha1.OwnerLabelKey]) assert.Empty(t, identity2.OwnerReferences) // Identity has no explicit owner reference. @@ -938,6 +939,7 @@ func TestReconcile(t *testing.T) { t.Run("create or update identities from UserID with invalid chars OK", func(t *testing.T) { userAcc := newUserAccount(username, userID) userAcc.Spec.UserID = "01234:ABCDEF" + userAcc.Spec.PropagatedClaims.Sub = userAcc.Spec.UserID t.Run("create user identity mapping", func(t *testing.T) { r, req, _, _ := prepareReconcile(t, username, userAcc, preexistingUser) @@ -1096,9 +1098,9 @@ func TestCreateIdentitiesOKWhenOriginalSubPresent(t *testing.T) { username := "kjones" userAcc := newUserAccount(username, userID) - // Unset the sso-user-id annotation so that a third identity isn't created - delete(userAcc.Annotations, toolchainv1alpha1.SSOUserIDAnnotationKey) - userAcc.Spec.OriginalSub = fmt.Sprintf("original-sub:%s", userID) + // Unset the UserID property so that a third identity isn't created + userAcc.Spec.PropagatedClaims.UserID = "" + userAcc.Spec.PropagatedClaims.OriginalSub = fmt.Sprintf("original-sub:%s", userID) // Reconcile to create the user r, req, _, _ := prepareReconcile(t, username, userAcc) @@ -1137,10 +1139,11 @@ func TestCreateIdentitiesOKWhenOriginalSubPresent(t *testing.T) { // Check the second created/updated identity identity2 := &userv1.Identity{} err = r.Client.Get(context.TODO(), types.NamespacedName{Name: ToIdentityName( - fmt.Sprintf("b64:%s", base64.RawStdEncoding.EncodeToString([]byte(userAcc.Spec.OriginalSub))), + fmt.Sprintf("b64:%s", base64.RawStdEncoding.EncodeToString([]byte(userAcc.Spec.PropagatedClaims.OriginalSub))), config.Auth().Idp())}, identity2) require.NoError(t, err) - assert.Equal(t, fmt.Sprintf("%s:b64:%s", config.Auth().Idp(), base64.RawStdEncoding.EncodeToString([]byte(userAcc.Spec.OriginalSub))), identity2.Name) + assert.Equal(t, fmt.Sprintf("%s:b64:%s", config.Auth().Idp(), base64.RawStdEncoding. + EncodeToString([]byte(userAcc.Spec.PropagatedClaims.OriginalSub))), identity2.Name) require.Equal(t, userAcc.Name, identity2.Labels["toolchain.dev.openshift.com/owner"]) assert.Empty(t, identity2.OwnerReferences) // Identity has no explicit owner reference. @@ -1162,7 +1165,7 @@ func TestCreateIdentitiesOKWhenSSOUserIDAnnotationPresent(t *testing.T) { username := "hcollins" userAcc := newUserAccount(username, userID) - userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey] = "ABCDE:98765" + userAcc.Spec.PropagatedClaims.UserID = "ABCDE:98765" userUID := types.UID(username + "user") preexistingUser := &userv1.User{ @@ -1208,11 +1211,12 @@ func TestCreateIdentitiesOKWhenSSOUserIDAnnotationPresent(t *testing.T) { // Check the second created/updated identity identity2 := &userv1.Identity{} err = r.Client.Get(context.TODO(), types.NamespacedName{Name: ToIdentityName( - fmt.Sprintf("b64:%s", base64.RawStdEncoding.EncodeToString([]byte(userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]))), + fmt.Sprintf("b64:%s", base64.RawStdEncoding. + EncodeToString([]byte(userAcc.Spec.PropagatedClaims.UserID))), config.Auth().Idp())}, identity2) require.NoError(t, err) assert.Equal(t, fmt.Sprintf("%s:b64:%s", config.Auth().Idp(), base64.RawStdEncoding.EncodeToString( - []byte(userAcc.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]))), identity2.Name) + []byte(userAcc.Spec.PropagatedClaims.UserID))), identity2.Name) require.Equal(t, userAcc.Name, identity2.Labels["toolchain.dev.openshift.com/owner"]) assert.Empty(t, identity2.OwnerReferences) // Identity has no explicit owner reference. @@ -1604,13 +1608,17 @@ func newUserAccount(userName, userID string, opts ...userAccountOption) *toolcha toolchainv1alpha1.TierLabelKey: "basic", }, Annotations: map[string]string{ - toolchainv1alpha1.UserEmailAnnotationKey: userName + "@acme.com", - toolchainv1alpha1.SSOUserIDAnnotationKey: "123456", - toolchainv1alpha1.SSOAccountIDAnnotationKey: "987654", + toolchainv1alpha1.UserEmailAnnotationKey: userName + "@acme.com", }, }, Spec: toolchainv1alpha1.UserAccountSpec{ UserID: userID, + PropagatedClaims: toolchainv1alpha1.PropagatedClaims{ + Email: userName + "@acme.com", + UserID: "123456", + AccountID: "987654", + Sub: userID, + }, }, } for _, apply := range opts {