Refactor remaining service and test code to remove references to old identity properties

pkg/controller/signup_test.go

@@ -97,18 +97,16 @@ func (s *TestSignupSuite) TestSignupPostHandler() {
 		expectedUserID := ob.String()
 		ctx.Set(context.SubKey, expectedUserID)
 		ctx.Set(context.EmailKey, expectedUserID+"@test.com")
-		email := ctx.GetString(context.EmailKey)
 		signup := &crtapi.UserSignup{
 			TypeMeta: v1.TypeMeta{},
 			ObjectMeta: v1.ObjectMeta{
 				Name:      userID.String(),
 				Namespace: "namespace-foo",
-				Annotations: map[string]string{
-					crtapi.UserSignupUserEmailAnnotationKey: email,
-				},
 			},
 			Spec: crtapi.UserSignupSpec{
-				Username: "bill",
+				IdentityClaims: crtapi.IdentityClaimsEmbedded{
+					PreferredUsername: "bill",
+				},
 			},
 			Status: crtapi.UserSignupStatus{
 				Conditions: []crtapi.Condition{
@@ -279,7 +277,6 @@ func (s *TestSignupSuite) TestInitVerificationHandler() {
 			Name:      userID,
 			Namespace: configuration.Namespace(),
 			Annotations: map[string]string{
-				crtapi.UserSignupUserEmailAnnotationKey:           "[email protected]",
 				crtapi.UserSignupVerificationCounterAnnotationKey: "0",
 				crtapi.UserSignupVerificationCodeAnnotationKey:    "",
 			},
@@ -392,9 +389,6 @@ func (s *TestSignupSuite) TestInitVerificationHandler() {
 			ObjectMeta: v1.ObjectMeta{
 				Name:      userID,
 				Namespace: configuration.Namespace(),
-				Annotations: map[string]string{
-					crtapi.UserSignupUserEmailAnnotationKey: "[email protected]",
-				},
 			},
 			Spec:   crtapi.UserSignupSpec{},
 			Status: crtapi.UserSignupStatus{},
@@ -436,10 +430,7 @@ func (s *TestSignupSuite) TestInitVerificationHandler() {
 				us := crtapi.UserSignup{
 					TypeMeta: v1.TypeMeta{},
 					ObjectMeta: v1.ObjectMeta{
-						Name: userID,
-						Annotations: map[string]string{
-							crtapi.UserSignupUserEmailAnnotationKey: "[email protected]",
-						},
+						Name:   userID,
 						Labels: map[string]string{},
 					},
 					Spec:   crtapi.UserSignupSpec{},
@@ -483,7 +474,6 @@ func (s *TestSignupSuite) TestVerifyPhoneCodeHandler() {
 			Name:      userID,
 			Namespace: configuration.Namespace(),
 			Annotations: map[string]string{
-				crtapi.UserSignupUserEmailAnnotationKey:        "[email protected]",
 				crtapi.UserVerificationAttemptsAnnotationKey:   "0",
 				crtapi.UserSignupVerificationCodeAnnotationKey: "999888",
 				crtapi.UserVerificationExpiryAnnotationKey:     time.Now().Add(10 * time.Second).Format(service.TimestampLayout),
@@ -671,15 +661,18 @@ func (s *TestSignupSuite) TestVerifyPhoneCodeHandler() {
 				Name:      "jsmith",
 				Namespace: configuration.Namespace(),
 				Annotations: map[string]string{
-					crtapi.UserSignupUserEmailAnnotationKey:        "[email protected]",
 					crtapi.UserVerificationAttemptsAnnotationKey:   "0",
 					crtapi.UserSignupVerificationCodeAnnotationKey: "999127",
 					crtapi.UserVerificationExpiryAnnotationKey:     time.Now().Add(10 * time.Second).Format(service.TimestampLayout),
 				},
 			},
 			Spec: crtapi.UserSignupSpec{
-				Userid:   otherUserID,
-				Username: "jsmith",
+				IdentityClaims: crtapi.IdentityClaimsEmbedded{
+					PreferredUsername: "jsmith",
+					PropagatedClaims: crtapi.PropagatedClaims{
+						Sub: otherUserID,
+					},
+				},
 			},
 			Status: crtapi.UserSignupStatus{},
 		}
@@ -695,7 +688,7 @@ func (s *TestSignupSuite) TestVerifyPhoneCodeHandler() {
 			Key:   "code",
 			Value: "999127",
 		}
-		rr := initPhoneVerification(s.T(), handler, param, nil, "", otherUserSignup.Spec.Username, http.MethodGet, "/api/v1/signup/verification")
+		rr := initPhoneVerification(s.T(), handler, param, nil, "", otherUserSignup.Spec.IdentityClaims.PreferredUsername, http.MethodGet, "/api/v1/signup/verification")
 
 		// Check the status code is what we expect.
 		require.Equal(s.T(), http.StatusOK, rr.Code)

pkg/informers/service/informer_service_test.go

@@ -42,7 +42,9 @@ func (s *TestInformerServiceSuite) TestInformerService() {
 					Object: map[string]interface{}{
 						"spec": map[string]interface{}{
 							"tierName": "deactivate30",
-							"userID":   "john-id",
+							"propagatedClaims": map[string]interface{}{
+								"sub": "john-id",
+							},
 							"userAccounts": []map[string]interface{}{
 								{
 									"targetCluster": "member1",
@@ -55,7 +57,9 @@ func (s *TestInformerServiceSuite) TestInformerService() {
 					Object: map[string]interface{}{
 						"spec": map[string]interface{}{
 							"tierName": "deactivate30",
-							"userID":   "noise-id",
+							"propagatedClaims": map[string]interface{}{
+								"sub": "noise-id",
+							},
 							"userAccounts": []map[string]interface{}{
 								{
 									"targetCluster": "member2",
@@ -89,12 +93,14 @@ func (s *TestInformerServiceSuite) TestInformerService() {
 			expected := &toolchainv1alpha1.MasterUserRecord{
 				Spec: toolchainv1alpha1.MasterUserRecordSpec{
 					TierName: "deactivate30",
-					UserID:   "john-id",
 					UserAccounts: []toolchainv1alpha1.UserAccountEmbedded{
 						{
 							TargetCluster: "member1",
 						},
 					},
+					PropagatedClaims: toolchainv1alpha1.PropagatedClaims{
+						Sub: "john-id",
+					},
 				},
 			}
 
@@ -104,7 +110,7 @@ func (s *TestInformerServiceSuite) TestInformerService() {
 			// then
 			require.NotNil(s.T(), val)
 			require.NoError(s.T(), err)
-			assert.Equal(s.T(), val, expected)
+			assert.Equal(s.T(), expected, val)
 		})
 	})
 
@@ -164,7 +170,7 @@ func (s *TestInformerServiceSuite) TestInformerService() {
 			// then
 			require.NotNil(s.T(), val)
 			require.NoError(s.T(), err)
-			assert.Equal(s.T(), val, expected)
+			assert.Equal(s.T(), expected, val)
 		})
 	})
 
@@ -305,25 +311,29 @@ func (s *TestInformerServiceSuite) TestInformerService() {
 					Object: map[string]interface{}{
 						"spec": map[string]interface{}{
 							"targetCluster": "member2",
-							"username":      "[email protected]",
-							"userid":        "foo",
-							"givenName":     "Foo",
-							"familyName":    "Bar",
-							"company":       "Red Hat",
-							"originalSub":   "sub-key",
+							"identityClaims": map[string]interface{}{
+								"sub":               "foo",
+								"originalSub":       "sub-key",
+								"preferredUsername": "[email protected]",
+								"givenName":         "Foo",
+								"familyName":        "Bar",
+								"company":           "Red Hat",
+							},
 						},
 					},
 				},
 				"noise": {
 					Object: map[string]interface{}{
 						"spec": map[string]interface{}{
 							"targetCluster": "member1",
-							"username":      "[email protected]",
-							"userid":        "noise",
-							"givenName":     "Noise",
-							"familyName":    "Make",
-							"company":       "Noisy",
-							"originalSub":   "noise-key",
+							"identityClaims": map[string]interface{}{
+								"sub":               "noise",
+								"originalSub":       "noise-key",
+								"preferredUsername": "[email protected]",
+								"givenName":         "Noise",
+								"familyName":        "Make",
+								"company":           "Noisy",
+							},
 						},
 					},
 				},
@@ -353,12 +363,16 @@ func (s *TestInformerServiceSuite) TestInformerService() {
 			expected := &toolchainv1alpha1.UserSignup{
 				Spec: toolchainv1alpha1.UserSignupSpec{
 					TargetCluster: "member2",
-					Username:      "[email protected]",
-					Userid:        "foo",
-					GivenName:     "Foo",
-					FamilyName:    "Bar",
-					Company:       "Red Hat",
-					OriginalSub:   "sub-key",
+					IdentityClaims: toolchainv1alpha1.IdentityClaimsEmbedded{
+						PreferredUsername: "[email protected]",
+						GivenName:         "Foo",
+						FamilyName:        "Bar",
+						Company:           "Red Hat",
+						PropagatedClaims: toolchainv1alpha1.PropagatedClaims{
+							Sub:         "foo",
+							OriginalSub: "sub-key",
+						},
+					},
 				},
 			}
 
@@ -368,7 +382,7 @@ func (s *TestInformerServiceSuite) TestInformerService() {
 			// then
 			require.NotNil(s.T(), val)
 			require.NoError(s.T(), err)
-			assert.Equal(s.T(), val, expected)
+			assert.Equal(s.T(), expected, val)
 		})
 	})
 

pkg/signup/service/signup_service.go

@@ -111,31 +111,22 @@ func (s *ServiceImpl) newUserSignup(ctx *gin.Context) (*toolchainv1alpha1.UserSi
 			Name:      EncodeUserIdentifier(ctx.GetString(context.UsernameKey)),
 			Namespace: configuration.Namespace(),
 			Annotations: map[string]string{
-				toolchainv1alpha1.UserSignupUserEmailAnnotationKey:           userEmail,
 				toolchainv1alpha1.UserSignupVerificationCounterAnnotationKey: "0",
-				toolchainv1alpha1.SSOUserIDAnnotationKey:                     userID,
-				toolchainv1alpha1.SSOAccountIDAnnotationKey:                  accountID,
 			},
 			Labels: map[string]string{
 				toolchainv1alpha1.UserSignupUserEmailHashLabelKey: emailHash,
 			},
 		},
 		Spec: toolchainv1alpha1.UserSignupSpec{
 			TargetCluster: "",
-			Userid:        ctx.GetString(context.SubKey),
-			Username:      ctx.GetString(context.UsernameKey),
-			GivenName:     ctx.GetString(context.GivenNameKey),
-			FamilyName:    ctx.GetString(context.FamilyNameKey),
-			Company:       ctx.GetString(context.CompanyKey),
-			OriginalSub:   ctx.GetString(context.OriginalSubKey),
 
 			IdentityClaims: toolchainv1alpha1.IdentityClaimsEmbedded{
 				PropagatedClaims: toolchainv1alpha1.PropagatedClaims{
 					Sub:         ctx.GetString(context.SubKey),
 					UserID:      ctx.GetString(context.UserIDKey),
 					AccountID:   ctx.GetString(context.AccountIDKey),
 					OriginalSub: ctx.GetString(context.OriginalSubKey),
-					Email:       ctx.GetString(context.EmailKey),
+					Email:       userEmail,
 				},
 				PreferredUsername: ctx.GetString(context.UsernameKey),
 				GivenName:         ctx.GetString(context.GivenNameKey),
@@ -536,29 +527,6 @@ func (s *ServiceImpl) auditUserSignupAgainstClaims(ctx *gin.Context, userSignup
 		updated = true
 	}
 
-	// Check the user_id and account_id annotations in the retrieved UserSignup.  If either of them are empty, but the
-	// values exist within the claims of the current user's Access Token then set the values in the UserSignup and update
-	// the resource.
-	// FIXME the following code may be removed after all UserSignup records have their IdentityClaims property fully populated
-	userIDValue, userIDFound := userSignup.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]
-	accountIDValue, accountIDFound := userSignup.Annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey]
-
-	if !userIDFound || userIDValue == "" {
-		userID := ctx.GetString(context.UserIDKey)
-		if userID != "" {
-			userSignup.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey] = userID
-			updated = true
-		}
-	}
-
-	if !accountIDFound || accountIDValue == "" {
-		accountID := ctx.GetString(context.AccountIDKey)
-		if accountID != "" {
-			userSignup.Annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey] = accountID
-			updated = true
-		}
-	}
-
 	return updated
 }
 
@@ -617,7 +585,7 @@ func (s *ServiceImpl) PhoneNumberAlreadyInUse(userID, username, phoneNumberOrHas
 		return errors.NewInternalError(err, "failed listing userSignups")
 	}
 	for _, signup := range userSignups {
-		if signup.Spec.Userid != userID && signup.Spec.Username != username && !states.Deactivated(signup) { // nolint:gosec
+		if signup.Spec.IdentityClaims.Sub != userID && signup.Spec.IdentityClaims.PreferredUsername != username && !states.Deactivated(signup) { // nolint:gosec
 			return errors.NewForbiddenError("cannot re-register with phone number",
 				"phone number already in use")
 		}