move PhoneNumberAlreadyInUse to verification_service.go

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 {

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.

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)
 

pkg/verification/service/verification_service.go

@@ -6,13 +6,15 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"regexp"
 	"strconv"
 	"time"
 
 	"github.com/codeready-toolchain/registration-service/pkg/namespaced"
 	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]

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")
+		})
+	})
+
+}