diff --git a/pkg/application/service/services.go b/pkg/application/service/services.go index c9b4f45e..0812d9ad 100644 --- a/pkg/application/service/services.go +++ b/pkg/application/service/services.go @@ -9,7 +9,6 @@ import ( type SignupService interface { Signup(ctx *gin.Context) (*toolchainv1alpha1.UserSignup, error) GetSignup(ctx *gin.Context, username string, checkUserSignupCompleted bool) (*signup.Signup, error) - PhoneNumberAlreadyInUse(username, phoneNumberOrHash string) error } type VerificationService interface { diff --git a/pkg/signup/service/signup_service.go b/pkg/signup/service/signup_service.go index dd070ecf..8f6f1a90 100644 --- a/pkg/signup/service/signup_service.go +++ b/pkg/signup/service/signup_service.go @@ -12,7 +12,6 @@ import ( "github.com/codeready-toolchain/registration-service/pkg/application/service" "github.com/codeready-toolchain/registration-service/pkg/configuration" "github.com/codeready-toolchain/registration-service/pkg/context" - "github.com/codeready-toolchain/registration-service/pkg/errors" "github.com/codeready-toolchain/registration-service/pkg/log" "github.com/codeready-toolchain/registration-service/pkg/namespaced" "github.com/codeready-toolchain/registration-service/pkg/signup" @@ -494,50 +493,6 @@ func (s *ServiceImpl) auditUserSignupAgainstClaims(ctx *gin.Context, userSignup return updated } -var ( - md5Matcher = regexp.MustCompile("(?i)[a-f0-9]{32}$") -) - -// PhoneNumberAlreadyInUse checks if the phone number has been banned. If so, return -// an internal server error. If not, check if an approved UserSignup with a different username -// and email address exists. If so, return an internal server error. Otherwise, return without error. -// Either the actual phone number, or the md5 hash of the phone number may be provided here. -func (s *ServiceImpl) PhoneNumberAlreadyInUse(username, phoneNumberOrHash string) error { - labelValue := hash.EncodeString(phoneNumberOrHash) - if md5Matcher.Match([]byte(phoneNumberOrHash)) { - labelValue = phoneNumberOrHash - } - - bannedUserList := &toolchainv1alpha1.BannedUserList{} - if err := s.List(gocontext.TODO(), bannedUserList, client.InNamespace(s.Namespace), - client.MatchingLabels{toolchainv1alpha1.BannedUserPhoneNumberHashLabelKey: labelValue}); err != nil { - return errors.NewInternalError(err, "failed listing banned users") - } - - if len(bannedUserList.Items) > 0 { - return errors.NewForbiddenError("cannot re-register with phone number", "phone number already in use") - } - - labelSelector := client.MatchingLabels{ - toolchainv1alpha1.UserSignupStateLabelKey: toolchainv1alpha1.UserSignupStateLabelValueApproved, - toolchainv1alpha1.BannedUserPhoneNumberHashLabelKey: labelValue, - } - userSignups := &toolchainv1alpha1.UserSignupList{} - if err := s.List(gocontext.TODO(), userSignups, client.InNamespace(s.Namespace), labelSelector); err != nil { - return errors.NewInternalError(err, "failed listing userSignups") - } - - for _, signup := range userSignups.Items { - userSignup := signup // drop with go 1.22 - if userSignup.Spec.IdentityClaims.PreferredUsername != username && !states.Deactivated(&userSignup) { - return errors.NewForbiddenError("cannot re-register with phone number", - "phone number already in use") - } - } - - return nil -} - // GetDefaultUserTarget retrieves the target cluster and the default namespace from the Space a user has access to. // If no spaceName is provided (assuming that this is the home space the target information should be taken from) // then the logic lists all Spaces user has access to and picks the first one. diff --git a/pkg/signup/service/signup_service_test.go b/pkg/signup/service/signup_service_test.go index bfc82ef1..cf8c9b04 100644 --- a/pkg/signup/service/signup_service_test.go +++ b/pkg/signup/service/signup_service_test.go @@ -474,51 +474,6 @@ func (s *TestSignupServiceSuite) TestFailsIfUserBanned() { require.Equal(s.T(), v1.StatusReasonForbidden, e.ErrStatus.Reason) } -func (s *TestSignupServiceSuite) TestPhoneNumberAlreadyInUseBannedUser() { - s.ServiceConfiguration(true, "redhat.com", 5) - - bannedUser := &toolchainv1alpha1.BannedUser{ - TypeMeta: v1.TypeMeta{}, - ObjectMeta: v1.ObjectMeta{ - Name: "banneduser", - Namespace: commontest.HostOperatorNs, - Labels: map[string]string{ - toolchainv1alpha1.BannedUserEmailHashLabelKey: "a7b1b413c1cbddbcd19a51222ef8e20a", - toolchainv1alpha1.BannedUserPhoneNumberHashLabelKey: "fd276563a8232d16620da8ec85d0575f", - }, - }, - Spec: toolchainv1alpha1.BannedUserSpec{ - Email: "jane.doe@gmail.com", - }, - } - - _, application := testutil.PrepareInClusterApp(s.T(), bannedUser) - - // when - err := application.SignupService().PhoneNumberAlreadyInUse("jsmith", "+12268213044") - - // then - require.EqualError(s.T(), err, "cannot re-register with phone number: phone number already in use") -} - -func (s *TestSignupServiceSuite) TestPhoneNumberAlreadyInUseUserSignup() { - s.ServiceConfiguration(true, "", 5) - - userSignup := testusersignup.NewUserSignup( - testusersignup.WithEncodedName("johnny@kubesaw"), - testusersignup.WithLabel(toolchainv1alpha1.UserSignupUserEmailHashLabelKey, "a7b1b413c1cbddbcd19a51222ef8e20a"), - testusersignup.WithLabel(toolchainv1alpha1.UserSignupUserPhoneHashLabelKey, "fd276563a8232d16620da8ec85d0575f"), - testusersignup.WithLabel(toolchainv1alpha1.UserSignupStateLabelKey, toolchainv1alpha1.UserSignupStateLabelValueApproved)) - - _, application := testutil.PrepareInClusterApp(s.T(), userSignup) - - // when - err := application.SignupService().PhoneNumberAlreadyInUse("jsmith", "+12268213044") - - // then - require.EqualError(s.T(), err, "cannot re-register with phone number: phone number already in use") -} - func (s *TestSignupServiceSuite) TestOKIfOtherUserBanned() { s.ServiceConfiguration(true, "", 5) diff --git a/pkg/verification/service/verification_service.go b/pkg/verification/service/verification_service.go index b73bc8b4..6d9fe51c 100644 --- a/pkg/verification/service/verification_service.go +++ b/pkg/verification/service/verification_service.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "net/http" + "regexp" "strconv" "time" @@ -13,6 +14,7 @@ import ( signuppkg "github.com/codeready-toolchain/registration-service/pkg/signup" "github.com/codeready-toolchain/registration-service/pkg/verification/sender" signupcommon "github.com/codeready-toolchain/toolchain-common/pkg/usersignup" + "sigs.k8s.io/controller-runtime/pkg/client" toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" "github.com/codeready-toolchain/registration-service/pkg/application/service" @@ -86,7 +88,7 @@ func (s *ServiceImpl) InitVerification(ctx *gin.Context, username, e164PhoneNumb } // Check if the provided phone number is already being used by another user - err := s.Services().SignupService().PhoneNumberAlreadyInUse(username, e164PhoneNumber) + err := PhoneNumberAlreadyInUse(s.Client, username, e164PhoneNumber) if err != nil { e := &crterrors.Error{} switch { @@ -263,7 +265,7 @@ func (s *ServiceImpl) VerifyPhoneCode(ctx *gin.Context, username, code string) ( annotationsToDelete := []string{} unsetVerificationRequired := false - err := s.Services().SignupService().PhoneNumberAlreadyInUse(username, signup.Labels[toolchainv1alpha1.UserSignupUserPhoneHashLabelKey]) + err := PhoneNumberAlreadyInUse(s.Client, username, signup.Labels[toolchainv1alpha1.UserSignupUserPhoneHashLabelKey]) if err != nil { log.Error(ctx, err, "phone number to verify already in use") return crterrors.NewBadRequest("phone number already in use", @@ -472,6 +474,50 @@ func (s *ServiceImpl) VerifyActivationCode(ctx *gin.Context, username, code stri return nil } +var ( + md5Matcher = regexp.MustCompile("(?i)[a-f0-9]{32}$") +) + +// PhoneNumberAlreadyInUse checks if the phone number has been banned. If so, return +// an internal server error. If not, check if an approved UserSignup with a different username +// and email address exists. If so, return an internal server error. Otherwise, return without error. +// Either the actual phone number, or the md5 hash of the phone number may be provided here. +func PhoneNumberAlreadyInUse(cl namespaced.Client, username, phoneNumberOrHash string) error { + labelValue := hash.EncodeString(phoneNumberOrHash) + if md5Matcher.Match([]byte(phoneNumberOrHash)) { + labelValue = phoneNumberOrHash + } + + bannedUserList := &toolchainv1alpha1.BannedUserList{} + if err := cl.List(gocontext.TODO(), bannedUserList, client.InNamespace(cl.Namespace), + client.MatchingLabels{toolchainv1alpha1.BannedUserPhoneNumberHashLabelKey: labelValue}); err != nil { + return crterrors.NewInternalError(err, "failed listing banned users") + } + + if len(bannedUserList.Items) > 0 { + return crterrors.NewForbiddenError("cannot re-register with phone number", "phone number already in use") + } + + labelSelector := client.MatchingLabels{ + toolchainv1alpha1.UserSignupStateLabelKey: toolchainv1alpha1.UserSignupStateLabelValueApproved, + toolchainv1alpha1.BannedUserPhoneNumberHashLabelKey: labelValue, + } + userSignups := &toolchainv1alpha1.UserSignupList{} + if err := cl.List(gocontext.TODO(), userSignups, client.InNamespace(cl.Namespace), labelSelector); err != nil { + return crterrors.NewInternalError(err, "failed listing userSignups") + } + + for _, signup := range userSignups.Items { + userSignup := signup // drop with go 1.22 + if userSignup.Spec.IdentityClaims.PreferredUsername != username && !states.Deactivated(&userSignup) { + return crterrors.NewForbiddenError("cannot re-register with phone number", + "phone number already in use") + } + } + + return nil +} + func checkAttempts(signup *toolchainv1alpha1.UserSignup) (int, error) { cfg := configuration.GetRegistrationServiceConfig() v, found := signup.Annotations[toolchainv1alpha1.UserVerificationAttemptsAnnotationKey] diff --git a/pkg/verification/service/verification_service_test.go b/pkg/verification/service/verification_service_test.go index 909c02d1..3086fc1b 100644 --- a/pkg/verification/service/verification_service_test.go +++ b/pkg/verification/service/verification_service_test.go @@ -13,6 +13,7 @@ import ( "testing" "time" + "github.com/codeready-toolchain/registration-service/pkg/namespaced" senderpkg "github.com/codeready-toolchain/registration-service/pkg/verification/sender" testutil "github.com/codeready-toolchain/registration-service/test/util" "sigs.k8s.io/controller-runtime/pkg/client" @@ -921,3 +922,121 @@ func (s *TestVerificationServiceSuite) testVerifyActivationCode(targetCluster st assert.Empty(s.T(), signup.Spec.TargetCluster) }) } + +func (s *TestVerificationServiceSuite) TestPhoneNumberAlreadyInUse() { + bannedUser := &toolchainv1alpha1.BannedUser{ + ObjectMeta: metav1.ObjectMeta{ + Name: "banneduser", + Namespace: commontest.HostOperatorNs, + Labels: map[string]string{ + toolchainv1alpha1.BannedUserEmailHashLabelKey: "a7b1b413c1cbddbcd19a51222ef8e20a", + toolchainv1alpha1.BannedUserPhoneNumberHashLabelKey: "fd276563a8232d16620da8ec85d0575f", + }, + }, + Spec: toolchainv1alpha1.BannedUserSpec{ + Email: "jane.doe@gmail.com", + }, + } + + userSignupApproved := testusersignup.NewUserSignup( + testusersignup.WithEncodedName("johnny@kubesaw"), + testusersignup.WithLabel(toolchainv1alpha1.UserSignupUserEmailHashLabelKey, "a7b1b413c1cbddbcd19a51222ef8e20a"), + testusersignup.WithLabel(toolchainv1alpha1.UserSignupUserPhoneHashLabelKey, "fd276563a8232d16620da8ec85d0575f"), + testusersignup.WithLabel(toolchainv1alpha1.UserSignupStateLabelKey, toolchainv1alpha1.UserSignupStateLabelValueApproved)) + + s.Run("when phone is not used yet", func() { + // given + fakeClient := commontest.NewFakeClient(s.T(), userSignupApproved, bannedUser) + nsdClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) + + // when + err := verificationservice.PhoneNumberAlreadyInUse(nsdClient, "jsmith", "+987654321") + + // then + require.NoError(s.T(), err) + }) + + s.Run("when phone is used but not by approved user", func() { + for _, state := range []string{"", toolchainv1alpha1.StateLabelValuePending, toolchainv1alpha1.UserSignupStateLabelValueDeactivated, toolchainv1alpha1.UserSignupStateLabelValueNotReady} { + s.Run(fmt.Sprintf("state: %s", state), func() {}) + // given + userSignup := testusersignup.NewUserSignup( + testusersignup.WithEncodedName("johnny@kubesaw"), + testusersignup.WithLabel(toolchainv1alpha1.UserSignupUserEmailHashLabelKey, "a7b1b413c1cbddbcd19a51222ef8e20a"), + testusersignup.WithLabel(toolchainv1alpha1.UserSignupUserPhoneHashLabelKey, "fd276563a8232d16620da8ec85d0575f"), + testusersignup.WithLabel(toolchainv1alpha1.UserSignupStateLabelKey, state)) + + fakeClient := commontest.NewFakeClient(s.T(), userSignup) + nsdClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) + + // when + err := verificationservice.PhoneNumberAlreadyInUse(nsdClient, "jsmith", "+987654321") + + // then + require.NoError(s.T(), err) + } + }) + + s.Run("when used by banned user", func() { + // given + fakeClient := commontest.NewFakeClient(s.T(), bannedUser) + nsdClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) + + // when + err := verificationservice.PhoneNumberAlreadyInUse(nsdClient, "jsmith", "+12268213044") + + // then + require.EqualError(s.T(), err, "cannot re-register with phone number: phone number already in use") + }) + + s.Run("when used by another user", func() { + // given + fakeClient := commontest.NewFakeClient(s.T(), userSignupApproved) + nsdClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) + + // when + err := verificationservice.PhoneNumberAlreadyInUse(nsdClient, "jsmith", "+12268213044") + + // then + require.EqualError(s.T(), err, "cannot re-register with phone number: phone number already in use") + }) + + s.Run("failures", func() { + s.Run("fails when lists banned users", func() { + // given + fakeClient := commontest.NewFakeClient(s.T(), userSignupApproved) + fakeClient.MockList = func(ctx gocontext.Context, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*toolchainv1alpha1.BannedUserList); ok { + return fmt.Errorf("list error") + } + return fakeClient.Client.List(ctx, list, opts...) + } + nsdClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) + + // when + err := verificationservice.PhoneNumberAlreadyInUse(nsdClient, "jsmith", "+12268213044") + + // then + require.EqualError(s.T(), err, "list error: failed listing banned users") + }) + + s.Run("fails when lists usersignups", func() { + // given + fakeClient := commontest.NewFakeClient(s.T(), userSignupApproved) + fakeClient.MockList = func(ctx gocontext.Context, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*toolchainv1alpha1.UserSignupList); ok { + return fmt.Errorf("list error") + } + return fakeClient.Client.List(ctx, list, opts...) + } + nsdClient := namespaced.NewClient(fakeClient, commontest.HostOperatorNs) + + // when + err := verificationservice.PhoneNumberAlreadyInUse(nsdClient, "jsmith", "+12268213044") + + // then + require.EqualError(s.T(), err, "list error: failed listing userSignups") + }) + }) + +} diff --git a/test/fake/proxy.go b/test/fake/proxy.go index c60c9155..3a210a7c 100644 --- a/test/fake/proxy.go +++ b/test/fake/proxy.go @@ -51,6 +51,3 @@ func (m *SignupService) Signup(_ *gin.Context) (*toolchainv1alpha1.UserSignup, e func (m *SignupService) UpdateUserSignup(_ *toolchainv1alpha1.UserSignup) (*toolchainv1alpha1.UserSignup, error) { return nil, nil } -func (m *SignupService) PhoneNumberAlreadyInUse(_, _ string) error { - return nil -}