Skip to content

Commit

Permalink
Refactor remaining service and test code to remove references to old …
Browse files Browse the repository at this point in the history 
…identity properties (#399)

* updated api

* updated toolchain-common

* updated dependencies

* fixed go.sum

* fixed some tests

* fixed some more tests

* fixed more tests

* fixed test

* minor

* fix merge

* removed replaced dependencies

* fixed test

---------

Co-authored-by: Francisc Munteanu <[email protected]>
  • Loading branch information
@sbryzak @mfrancisc
sbryzak and mfrancisc authored Feb 6, 2024
1 parent 545695d commit b4a49ca
Show file tree
Hide file tree
Showing 6 changed files with 146 additions and 312 deletions.
29 changes: 11 additions & 18 deletions pkg/controller/signup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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: "",
},
Expand Down Expand Up @@ -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{},
Expand Down Expand Up @@ -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{},
Expand Down Expand Up @@ -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),
Expand Down Expand Up @@ -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{},
}
Expand All @@ -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)
Expand Down
62 changes: 38 additions & 24 deletions pkg/informers/service/informer_service_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
},
},
}

Expand All @@ -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)
})
})

Expand Down Expand Up @@ -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)
})
})

Expand Down Expand Up @@ -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",
},
},
},
},
Expand Down Expand Up @@ -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",
},
},
},
}

Expand All @@ -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)
})
})

Expand Down
36 changes: 2 additions & 34 deletions pkg/signup/service/signup_service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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")
}
Expand Down
Loading

0 comments on commit b4a49ca

Please sign in to comment.