Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated API to remove all arbitrary SSO claim properties #872

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -121,3 +121,5 @@ require (
)

go 1.20

replace github.com/codeready-toolchain/api => github.com/sbryzak/api v0.0.0-20240102042357-82134b76d69e
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -123,8 +123,6 @@ github.com/cockroachdb/datadriven v0.0.0-20190809214429-80d97fb3cbaa/go.mod h1:z
github.com/cockroachdb/datadriven v0.0.0-20200714090401-bf6692d28da5/go.mod h1:h6jFvWxBdQXxjopDMZyH2UVceIRfR84bdzbkoKrsWNo=
github.com/cockroachdb/errors v1.2.4/go.mod h1:rQD95gz6FARkaKkQXUksEje/d9a6wBJoCr5oaCLELYA=
github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f/go.mod h1:i/u985jwjWRlyHXQbwatDASoW0RMlZ/3i9yJHE2xLkI=
github.com/codeready-toolchain/api v0.0.0-20231217224957-34f7cb3fcbf7 h1:8Rbzo3EQoQrJakXRKIxcluK0NwHeIWzzG2nDsiKIxsI=
github.com/codeready-toolchain/api v0.0.0-20231217224957-34f7cb3fcbf7/go.mod h1:FO7kgXH1x1LqkF327D5a36u0WIrwjVCbeijPkzgwaZc=
github.com/codeready-toolchain/toolchain-common v0.0.0-20231218221155-9d1179b6a349 h1:0xOPCrdPMWZwyyAO/O8U/FAWrp8NR6lvjtkLAXNavG4=
github.com/codeready-toolchain/toolchain-common v0.0.0-20231218221155-9d1179b6a349/go.mod h1:cEkJH2jz88KIZt5W8wSnK3Gz6OfszzXv74OIndbTlRE=
github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk=
Expand Down Expand Up @@ -595,6 +593,8 @@ github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR
github.com/russross/blackfriday/v2 v2.0.1/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
github.com/ryanuber/columnize v0.0.0-20160712163229-9b3edd62028f/go.mod h1:sm1tb6uqfes/u+d4ooFouqFdy9/2g9QGwK3SQygK0Ts=
github.com/sbryzak/api v0.0.0-20240102042357-82134b76d69e h1:1p4jHPdkwSARcFuJw2o2uQgfZExsm84kuzr+fb9wCLg=
github.com/sbryzak/api v0.0.0-20240102042357-82134b76d69e/go.mod h1:FO7kgXH1x1LqkF327D5a36u0WIrwjVCbeijPkzgwaZc=
github.com/scylladb/go-set v1.0.2/go.mod h1:DkpGd78rljTxKAnTDPFqXSGxvETQnJyuSOQwsHycqfs=
github.com/sean-/seed v0.0.0-20170313163322-e2103e2c3529/go.mod h1:DxrIzT+xaE7yg65j358z/aeFdxmN0P9QXhEzd20vsDc=
github.com/sergi/go-diff v1.1.0/go.mod h1:STckp+ISIX8hZLjrqAeVduY0gWCT9IjLuqbuNXdaHfM=
Expand Down
6 changes: 3 additions & 3 deletions test/e2e/parallel/e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func TestE2EFlow(t *testing.T) {
Execute(t).Resources()

// Confirm the originalSub property has been set during signup
require.Equal(t, originalSubJohnClaim, originalSubJohnSignup.Spec.OriginalSub)
require.Equal(t, originalSubJohnClaim, originalSubJohnSignup.Spec.IdentityClaims.OriginalSub)

VerifyResourcesProvisionedForSignup(t, awaitilities, johnSignup, "deactivate30", "base")
VerifyResourcesProvisionedForSignup(t, awaitilities, johnExtraSignup, "deactivate30", "base")
Expand Down Expand Up @@ -190,7 +190,7 @@ func TestE2EFlow(t *testing.T) {
namespaces := make([]*corev1.Namespace, 0, 2)
templateRefs := tiers.GetTemplateRefs(t, hostAwait, "base")
for _, ref := range templateRefs.Namespaces {
ns, err := memberAwait.WaitForNamespace(t, johnSignup.Spec.Username, ref, "base", wait.UntilNamespaceIsActive())
ns, err := memberAwait.WaitForNamespace(t, johnSignup.Spec.IdentityClaims.PreferredUsername, ref, "base", wait.UntilNamespaceIsActive())
require.NoError(t, err)
namespaces = append(namespaces, ns)
}
Expand All @@ -203,7 +203,7 @@ func TestE2EFlow(t *testing.T) {
// then
// wait for the namespaces to be re-created before validating all other resources to avoid race condition
for _, ref := range templateRefs.Namespaces {
_, err := memberAwait.WaitForNamespace(t, johnSignup.Spec.Username, ref, "base", wait.UntilNamespaceIsActive())
_, err := memberAwait.WaitForNamespace(t, johnSignup.Spec.IdentityClaims.PreferredUsername, ref, "base", wait.UntilNamespaceIsActive())
require.NoError(t, err)
}
VerifyResourcesProvisionedForSignup(t, awaitilities, johnSignup, "deactivate30", "base")
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/parallel/nstemplatetier_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ func TestResetDeactivatingStateWhenPromotingUser(t *testing.T) {
require.NoError(t, err)

// Move the MUR to the user tier with longer deactivation time
tiers.MoveMURToTier(t, hostAwait, updatedUserSignup.Spec.Username, "deactivate90")
tiers.MoveMURToTier(t, hostAwait, updatedUserSignup.Spec.IdentityClaims.PreferredUsername, "deactivate90")

// Ensure the deactivating state is reset after promotion
promotedUserSignup, err := hostAwait.WaitForUserSignup(t, updatedUserSignup.Name)
Expand Down
18 changes: 9 additions & 9 deletions test/e2e/parallel/registration_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ func TestSignupOK(t *testing.T) {
wait.UntilUserSignupHasStateLabel(toolchainv1alpha1.UserSignupStateLabelValuePending))
require.NoError(t, err)
cleanup.AddCleanTasks(t, hostAwait.Client, userSignup)
emailAnnotation := userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey]
emailAnnotation := userSignup.Spec.IdentityClaims.Email
assert.Equal(t, email, emailAnnotation)

// Call get signup endpoint with a valid token and make sure it's pending approval
Expand Down Expand Up @@ -424,8 +424,8 @@ func TestUserSignupFoundWhenNamedWithEncodedUsername(t *testing.T) {
wait.UntilUserSignupHasStateLabel(toolchainv1alpha1.UserSignupStateLabelValuePending))
require.NoError(t, err)
cleanup.AddCleanTasks(t, hostAwait.Client, userSignup)
emailAnnotation := userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey]
assert.Equal(t, emailAddress, emailAnnotation)
email := userSignup.Spec.IdentityClaims.Email
assert.Equal(t, emailAddress, email)

// Call get signup endpoint with a valid token, however we will now override the claims to introduce the original
// sub claim and set username as a separate claim, then we will make sure the UserSignup is returned correctly
Expand Down Expand Up @@ -464,8 +464,8 @@ func TestPhoneVerification(t *testing.T) {
wait.UntilUserSignupHasStateLabel(toolchainv1alpha1.UserSignupStateLabelValueNotReady))
require.NoError(t, err)
cleanup.AddCleanTasks(t, hostAwait.Client, userSignup)
emailAnnotation := userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey]
assert.Equal(t, emailAddress, emailAnnotation)
email := userSignup.Spec.IdentityClaims.Email
assert.Equal(t, emailAddress, email)

// Call get signup endpoint with a valid token and make sure verificationRequired is true
mp, mpStatus := ParseSignupResponse(t, NewHTTPRequest(t).InvokeEndpoint("GET", route+"/api/v1/signup", token0, "", http.StatusOK).UnmarshalMap())
Expand Down Expand Up @@ -550,7 +550,7 @@ func TestPhoneVerification(t *testing.T) {
states.SetApprovedManually(instance, true)
})
require.NoError(t, err)
transformedUsername := commonsignup.TransformUsername(userSignup.Spec.Username, []string{"openshift", "kube", "default", "redhat", "sandbox"}, []string{"admin"})
transformedUsername := commonsignup.TransformUsername(userSignup.Spec.IdentityClaims.PreferredUsername, []string{"openshift", "kube", "default", "redhat", "sandbox"}, []string{"admin"})
// Confirm the MasterUserRecord is provisioned
_, err = hostAwait.WaitForMasterUserRecord(t, transformedUsername, wait.UntilMasterUserRecordHasCondition(wait.Provisioned()))
require.NoError(t, err)
Expand All @@ -575,7 +575,7 @@ func TestPhoneVerification(t *testing.T) {
wait.UntilUserSignupHasStateLabel(toolchainv1alpha1.UserSignupStateLabelValueNotReady))
require.NoError(t, err)
cleanup.AddCleanTasks(t, hostAwait.Client, otherUserSignup)
otherEmailAnnotation := otherUserSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey]
otherEmailAnnotation := otherUserSignup.Spec.IdentityClaims.Email
assert.Equal(t, otherEmailValue, otherEmailAnnotation)

// Initiate the verification process using the same phone number as previously
Expand Down Expand Up @@ -850,8 +850,8 @@ func signup(t *testing.T, hostAwait *wait.HostAwaitility) (*toolchainv1alpha1.Us
wait.UntilUserSignupHasStateLabel(toolchainv1alpha1.UserSignupStateLabelValueNotReady))
require.NoError(t, err)
cleanup.AddCleanTasks(t, hostAwait.Client, userSignup)
emailAnnotation := userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey]
assert.Equal(t, emailValue, emailAnnotation)
email := userSignup.Spec.IdentityClaims.Email
assert.Equal(t, emailValue, email)
return userSignup, token
}

Expand Down
6 changes: 3 additions & 3 deletions test/e2e/user_management_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -530,7 +530,7 @@ func (s *userManagementTestSuite) TestUserBanning() {
Execute(t).Resources()

// Create the BannedUser
CreateBannedUser(t, s.Host(), userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey])
CreateBannedUser(t, s.Host(), userSignup.Spec.IdentityClaims.Email)

// Confirm the user is banned
_, err := hostAwait.WithRetryOptions(wait.TimeoutOption(time.Second*15)).WaitForUserSignup(t, userSignup.Name,
Expand Down Expand Up @@ -632,7 +632,7 @@ func (s *userManagementTestSuite) TestUserBanning() {
Execute(t).Resources()

// Create the BannedUser
bannedUser := CreateBannedUser(t, s.Host(), userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey])
bannedUser := CreateBannedUser(t, s.Host(), userSignup.Spec.IdentityClaims.Email)

// Confirm the user is banned
_, err := hostAwait.WaitForUserSignup(t, userSignup.Name,
Expand Down Expand Up @@ -715,7 +715,7 @@ func (s *userManagementTestSuite) TestUserDisabled() {

// Check the Identity is deleted
identity := &userv1.Identity{}
err = hostAwait.Client.Get(context.TODO(), types.NamespacedName{Name: identitypkg.NewIdentityNamingStandard(userAccount.Spec.UserID, "rhd").IdentityName()}, identity)
err = hostAwait.Client.Get(context.TODO(), types.NamespacedName{Name: identitypkg.NewIdentityNamingStandard(userAccount.Spec.PropagatedClaims.UserID, "rhd").IdentityName()}, identity)
require.Error(s.T(), err)
assert.True(s.T(), apierrors.IsNotFound(err))

Expand Down
2 changes: 1 addition & 1 deletion test/e2e/usersignup_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -414,7 +414,7 @@ func (s *userSignupIntegrationTest) TestUserResourcesCreatedWhenOriginalSubIsSet

func (s *userSignupIntegrationTest) userIsNotProvisioned(t *testing.T, userSignup *toolchainv1alpha1.UserSignup) {
hostAwait := s.Host()
hostAwait.CheckMasterUserRecordIsDeleted(t, userSignup.Spec.Username)
hostAwait.CheckMasterUserRecordIsDeleted(t, userSignup.Spec.IdentityClaims.PreferredUsername)
currentUserSignup, err := hostAwait.WaitForUserSignup(t, userSignup.Name)
require.NoError(t, err)
assert.Equal(t, toolchainv1alpha1.UserSignupStateLabelValuePending, currentUserSignup.Labels[toolchainv1alpha1.UserSignupStateLabelKey])
Expand Down
2 changes: 1 addition & 1 deletion test/metrics/metrics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ func TestMetricsWhenUsersBanned(t *testing.T) {
Execute(t).Resources()

// when creating the BannedUser resource
bannedUser := banUser(t, hostAwait, userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey])
bannedUser := banUser(t, hostAwait, userSignup.Spec.IdentityClaims.Email)

// then
// confirm the user is banned
Expand Down
2 changes: 1 addition & 1 deletion test/migration/setup_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ func (r *SetupMigrationRunner) prepareBannedUser(t *testing.T) {
hostAwait := r.Awaitilities.Host()

// Create the BannedUser
bannedUser := testsupport.NewBannedUser(hostAwait, userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey])
bannedUser := testsupport.NewBannedUser(hostAwait, userSignup.Spec.IdentityClaims.Email)
err := hostAwait.Client.Create(context.TODO(), bannedUser)
require.NoError(t, err)

Expand Down
4 changes: 2 additions & 2 deletions test/migration/verify/verify_migration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ func verifyProvisionedSignup(t *testing.T, awaitilities wait.Awaitilities, signu
func verifySecondMemberProvisionedSignup(t *testing.T, awaitilities wait.Awaitilities, signup *toolchainv1alpha1.UserSignup) {
cleanup.AddCleanTasks(t, awaitilities.Host().Client, signup)
VerifyResourcesProvisionedForSignup(t, awaitilities, signup, "deactivate30", "base")
CreateBannedUser(t, awaitilities.Host(), signup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey])
CreateBannedUser(t, awaitilities.Host(), signup.Spec.IdentityClaims.Email)
}

func verifyAppStudioProvisionedSignup(t *testing.T, awaitilities wait.Awaitilities, signup *toolchainv1alpha1.UserSignup) {
Expand Down Expand Up @@ -212,7 +212,7 @@ func verifyBannedSignup(t *testing.T, awaitilities wait.Awaitilities, signup *to

// get the BannedUser resource
matchEmailHash := client.MatchingLabels{
toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString(signup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey]),
toolchainv1alpha1.BannedUserEmailHashLabelKey: hash.EncodeString(signup.Spec.IdentityClaims.Email),
}
bannedUsers := &toolchainv1alpha1.BannedUserList{}
err = hostAwait.Client.List(context.TODO(), bannedUsers, client.InNamespace(hostAwait.Namespace), matchEmailHash)
Expand Down
19 changes: 9 additions & 10 deletions testsupport/user_assertions.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,15 +126,14 @@ func verifyUserAccount(t *testing.T, awaitilities wait.Awaitilities, userSignup

// Check the originalSub identity
originalSubIdentityName := ""
if userAccount.Spec.OriginalSub != "" {
if userAccount.Spec.PropagatedClaims.OriginalSub != "" {
originalSubIdentityName = identitypkg.NewIdentityNamingStandard(userAccount.Spec.PropagatedClaims.OriginalSub, "rhd").IdentityName()
}

// Check the UserID identity
userIDIdentityName := ""
val, ok := userAccount.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey]
if ok {
userIDIdentityName = identitypkg.NewIdentityNamingStandard(val, "rhd").IdentityName()
if userAccount.Spec.PropagatedClaims.UserID != "" {
userIDIdentityName = identitypkg.NewIdentityNamingStandard(userAccount.Spec.PropagatedClaims.UserID, "rhd").IdentityName()
}

memberConfiguration := memberAwait.GetMemberOperatorConfig(t)
Expand All @@ -145,22 +144,22 @@ func verifyUserAccount(t *testing.T, awaitilities wait.Awaitilities, userSignup
user, err := memberAwait.WaitForUser(t, userAccount.Name,
wait.UntilUserHasLabel(toolchainv1alpha1.ProviderLabelKey, toolchainv1alpha1.ProviderLabelValue),
wait.UntilUserHasLabel(toolchainv1alpha1.OwnerLabelKey, userAccount.Name),
wait.UntilUserHasAnnotation(toolchainv1alpha1.UserEmailAnnotationKey,
userSignup.Annotations[toolchainv1alpha1.UserSignupUserEmailAnnotationKey]))
wait.UntilUserHasAnnotation(toolchainv1alpha1.EmailUserAnnotationKey,
userSignup.Spec.IdentityClaims.Email))
assert.NoError(t, err, fmt.Sprintf("no user with name '%s' found", userAccount.Name))

userID := userSignup.Spec.IdentityClaims.UserID
if userID != "" {
accountID := userSignup.Spec.IdentityClaims.AccountID
if accountID != "" {
require.Equal(t, userID, user.Annotations[toolchainv1alpha1.SSOUserIDAnnotationKey])
require.Equal(t, accountID, user.Annotations[toolchainv1alpha1.SSOAccountIDAnnotationKey])
require.Equal(t, userID, user.Annotations[toolchainv1alpha1.UserIDUserAnnotationKey])
require.Equal(t, accountID, user.Annotations[toolchainv1alpha1.AccountIDUserAnnotationKey])
}
}

if userID == "" {
require.NotContains(t, user.Annotations, toolchainv1alpha1.SSOUserIDAnnotationKey)
require.NotContains(t, user.Annotations, toolchainv1alpha1.SSOAccountIDAnnotationKey)
require.NotContains(t, user.Annotations, toolchainv1alpha1.UserIDUserAnnotationKey)
require.NotContains(t, user.Annotations, toolchainv1alpha1.AccountIDUserAnnotationKey)
}

// Verify provisioned Identity
Expand Down
3 changes: 0 additions & 3 deletions testsupport/user_setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,6 @@ func NewUserSignup(namespace, username string, email string) *toolchainv1alpha1.
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: name,
Annotations: map[string]string{
toolchainv1alpha1.UserSignupUserEmailAnnotationKey: email,
},
Labels: map[string]string{
toolchainv1alpha1.UserSignupUserEmailHashLabelKey: hash.EncodeString(email),
},
Expand Down
5 changes: 3 additions & 2 deletions testsupport/wait/member.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,15 +185,16 @@ func UntilUserAccountMatchesMur(hostAwaitility *HostAwaitility) UserAccountWaitC
if err != nil {
return false
}
return actual.Spec.UserID == mur.Spec.UserID &&
return actual.Spec.PropagatedClaims.Sub == mur.Spec.PropagatedClaims.Sub &&
actual.Spec.Disabled == mur.Spec.Disabled
},
Diff: func(actual *toolchainv1alpha1.UserAccount) string {
mur, err := hostAwaitility.GetMasterUserRecord(actual.Name)
if err != nil {
return fmt.Sprintf("could not find mur for user account '%s'", actual.Name)
}
return fmt.Sprintf("expected mur to match with useraccount:\n\tUserID: %s/%s\n\tDisabled: %t/%t\n", actual.Spec.UserID, mur.Spec.UserID, actual.Spec.Disabled, mur.Spec.Disabled)
return fmt.Sprintf("expected mur to match with useraccount:\n\tUserID: %s/%s\n\tDisabled: %t/%t\n",
actual.Spec.PropagatedClaims.Sub, mur.Spec.PropagatedClaims.Sub, actual.Spec.Disabled, mur.Spec.Disabled)
},
}
}
Expand Down
Loading