make required captcha score configurable (#379)

* add captcha requiredScore and allowLowScoreReactivation configurations
---------
Co-authored-by: Alexey Kazakov <alkazako@redhat.com>

go.mod

@@ -4,8 +4,8 @@ go 1.20
 
 require (
 	github.com/aws/aws-sdk-go v1.44.100
-	github.com/codeready-toolchain/api v0.0.0-20231107202930-b028ae440a26
-	github.com/codeready-toolchain/toolchain-common v0.0.0-20231117145902-3e7430ae48bb
+	github.com/codeready-toolchain/api v0.0.0-20231129193441-f6c9b7feee01
+	github.com/codeready-toolchain/toolchain-common v0.0.0-20231206125932-41dd47e8aa56
 	github.com/go-logr/logr v1.2.3
 	github.com/gofrs/uuid v4.2.0+incompatible
 	github.com/pkg/errors v0.9.1

go.sum

@@ -115,10 +115,10 @@ github.com/cncf/xds/go v0.0.0-20210805033703-aa0b78936158/go.mod h1:eXthEFrGJvWH
 github.com/cncf/xds/go v0.0.0-20210922020428-25de7278fc84/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
 github.com/cncf/xds/go v0.0.0-20211001041855-01bcc9b48dfe/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
 github.com/cncf/xds/go v0.0.0-20211011173535-cb28da3451f1/go.mod h1:eXthEFrGJvWHgFFCl3hGmgk+/aYT6PnTQLykKQRLhEs=
-github.com/codeready-toolchain/api v0.0.0-20231107202930-b028ae440a26 h1:7l/9jcykzh/Qq93EnPYQYpaejPkWaaHB6BLCwTKngJE=
-github.com/codeready-toolchain/api v0.0.0-20231107202930-b028ae440a26/go.mod h1:bImSKnxrpNmCmW/YEGiiZnZqJm3kAmfP5hW4YndK0hE=
-github.com/codeready-toolchain/toolchain-common v0.0.0-20231117145902-3e7430ae48bb h1:TVgy4tO2oy2YwWTZXXYytY9soshylURDzmdkY6ch1+o=
-github.com/codeready-toolchain/toolchain-common v0.0.0-20231117145902-3e7430ae48bb/go.mod h1:fmMAvkwt/OfANx8l/BDbuaHkjInlRJ3uYmIjjVmjd9w=
+github.com/codeready-toolchain/api v0.0.0-20231129193441-f6c9b7feee01 h1:Pl8UIl/m2/n9C4pXzaR6A3vxRFX+4QRw90x8RDhCdXw=
+github.com/codeready-toolchain/api v0.0.0-20231129193441-f6c9b7feee01/go.mod h1:FO7kgXH1x1LqkF327D5a36u0WIrwjVCbeijPkzgwaZc=
+github.com/codeready-toolchain/toolchain-common v0.0.0-20231206125932-41dd47e8aa56 h1:W/VrNwL++P6TRw6BqhPfnzaq3YmytBIvestbLDvVv1E=
+github.com/codeready-toolchain/toolchain-common v0.0.0-20231206125932-41dd47e8aa56/go.mod h1:cEkJH2jz88KIZt5W8wSnK3Gz6OfszzXv74OIndbTlRE=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=

pkg/configuration/configuration.go

@@ -281,6 +281,24 @@ func (r VerificationConfig) CaptchaScoreThreshold() float32 {
 	return float32(thresholdFloat)
 }
 
+func (r VerificationConfig) CaptchaRequiredScore() float32 {
+	const defaultAutomaticVerificationThreshold float32 = 0
+	threshold := commonconfig.GetString(r.c.Captcha.RequiredScore, "0")
+	thresholdFloat, err := strconv.ParseFloat(threshold, 32)
+	if err != nil {
+
+		if threshold != "" {
+			log.Error(nil, err, fmt.Sprintf("unable to parse automatic verification threshold, using default value '%.1f'", defaultAutomaticVerificationThreshold))
+		}
+		return defaultAutomaticVerificationThreshold
+	}
+	return float32(thresholdFloat)
+}
+
+func (r VerificationConfig) CaptchaAllowLowScoreReactivation() bool {
+	return commonconfig.GetBool(r.c.Captcha.AllowLowScoreReactivation, true)
+}
+
 func (r VerificationConfig) CaptchaSiteKey() string {
 	return commonconfig.GetString(r.c.Captcha.SiteKey, "")
 }

pkg/configuration/configuration_test.go

@@ -63,6 +63,8 @@ func TestRegistrationService(t *testing.T) {
 		assert.Empty(t, regServiceCfg.Verification().CaptchaProjectID())
 		assert.Empty(t, regServiceCfg.Verification().CaptchaSiteKey())
 		assert.Equal(t, float32(0.9), regServiceCfg.Verification().CaptchaScoreThreshold())
+		assert.Equal(t, float32(0), regServiceCfg.Verification().CaptchaRequiredScore())
+		assert.Equal(t, true, regServiceCfg.Verification().CaptchaAllowLowScoreReactivation())
 		assert.Empty(t, regServiceCfg.Verification().CaptchaServiceAccountFileContents())
 	})
 	t.Run("non-default", func(t *testing.T) {
@@ -89,6 +91,8 @@ func TestRegistrationService(t *testing.T) {
 			Verification().CaptchaProjectID("test-project").
 			Verification().CaptchaSiteKey("site-key").
 			Verification().CaptchaScoreThreshold("0.7").
+			Verification().CaptchaRequiredScore("0.5").
+			Verification().CaptchaAllowLowScoreReactivation(false).
 			Verification().Secret().Ref("verification-secrets").
 			TwilioAccountSID("twilio.sid").
 			TwilioAuthToken("twilio.token").
@@ -138,6 +142,8 @@ func TestRegistrationService(t *testing.T) {
 		assert.Equal(t, "test-project", regServiceCfg.Verification().CaptchaProjectID())
 		assert.Equal(t, "site-key", regServiceCfg.Verification().CaptchaSiteKey())
 		assert.Equal(t, float32(0.7), regServiceCfg.Verification().CaptchaScoreThreshold())
+		assert.Equal(t, float32(0.5), regServiceCfg.Verification().CaptchaRequiredScore())
+		assert.Equal(t, false, regServiceCfg.Verification().CaptchaAllowLowScoreReactivation())
 		assert.Equal(t, "example-content", regServiceCfg.Verification().CaptchaServiceAccountFileContents())
 	})
 }

pkg/verification/service/verification_service.go

@@ -228,12 +228,27 @@ func (s *ServiceImpl) VerifyPhoneCode(ctx *gin.Context, userID, username, code s
 		return crterrors.NewInternalError(lookupErr, fmt.Sprintf("error retrieving usersignup: %s", userID))
 	}
 
-	// require manual approval if captcha score below automatic verification threshold
-	captchaScore, found := signup.Annotations[toolchainv1alpha1.UserSignupCaptchaScoreAnnotationKey]
-	fscore, parseErr := strconv.ParseFloat(captchaScore, 32)
-	if found && parseErr == nil && fscore < 0.6 {
-		log.Error(ctx, errors.New("captcha score too low"), "automatic verification disabled, manual approval required for user")
-		return crterrors.NewForbiddenError("verification failed", "verification is not available at this time")
+	// check if it's a reactivation
+	if activationCounterString, foundActivationCounter := signup.Annotations[toolchainv1alpha1.UserSignupActivationCounterAnnotationKey]; foundActivationCounter && cfg.Verification().CaptchaAllowLowScoreReactivation() {
+		activationCounter, err := strconv.Atoi(activationCounterString)
+		if err != nil {
+			log.Error(ctx, err, "activation counter is not an integer value, checking required captcha score")
+			// require manual approval if captcha score below automatic verification threshold
+			if err = checkRequiredManualApproval(ctx, signup, cfg); err != nil {
+				return err
+			}
+		} else if activationCounter == 1 {
+			// check required captcha score if it's not a reactivation
+			if err = checkRequiredManualApproval(ctx, signup, cfg); err != nil {
+				return err
+			}
+		}
+	} else {
+		// when allowLowScoreReactivation is not enabled or no activation counter found
+		// require manual approval if captcha score below automatic verification threshold for all users
+		if err := checkRequiredManualApproval(ctx, signup, cfg); err != nil {
+			return err
+		}
 	}
 
 	annotationValues := map[string]string{}
@@ -336,6 +351,25 @@ func (s *ServiceImpl) VerifyPhoneCode(ctx *gin.Context, userID, username, code s
 	return
 }
 
+// checkRequiredManualApproval compares the user captcha score with the configured required captcha score.
+// When the user score is lower than the required score an error is returned meaning that the user is considered "suspicious" and manual approval of the signup is required.
+func checkRequiredManualApproval(ctx *gin.Context, signup *toolchainv1alpha1.UserSignup, cfg configuration.RegistrationServiceConfig) error {
+	captchaScore, found := signup.Annotations[toolchainv1alpha1.UserSignupCaptchaScoreAnnotationKey]
+	if found {
+		fscore, parseErr := strconv.ParseFloat(captchaScore, 32)
+		if parseErr != nil {
+			// let's just log the parsing error and return
+			log.Error(ctx, parseErr, "error while parsing captchaScore")
+			return nil
+		}
+		if parseErr == nil && float32(fscore) < cfg.Verification().CaptchaRequiredScore() {
+			log.Info(ctx, fmt.Sprintf("captcha score %v is too low, automatic verification disabled, manual approval required for user", float32(fscore)))
+			return crterrors.NewForbiddenError("verification failed", "verification is not available at this time")
+		}
+	}
+	return nil
+}
+
 // VerifyActivationCode verifies the activation code:
 // - checks that the SocialEvent resource named after the activation code exists
 // - checks that the SocialEvent has enough capacity to approve the user

pkg/verification/service/verification_service_test.go

@@ -896,41 +896,135 @@ func (s *TestVerificationServiceSuite) TestVerifyPhoneCode() {
 		require.EqualError(s.T(), err, "parsing time \"ABC\" as \"2006-01-02T15:04:05.000Z07:00\": cannot parse \"ABC\" as \"2006\": error parsing expiry timestamp", err.Error())
 	})
 
-	s.T().Run("when captcha score below automatic verification threshold", func(t *testing.T) {
-
-		userSignup := &toolchainv1alpha1.UserSignup{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      "123",
-				Namespace: configuration.Namespace(),
-				Annotations: map[string]string{
-					toolchainv1alpha1.UserSignupUserEmailAnnotationKey:        "sbryzak@redhat.com",
-					toolchainv1alpha1.UserVerificationAttemptsAnnotationKey:   "0",
-					toolchainv1alpha1.UserSignupCaptchaScoreAnnotationKey:     "0.5",
-					toolchainv1alpha1.UserSignupVerificationCodeAnnotationKey: "123456",
-					toolchainv1alpha1.UserVerificationExpiryAnnotationKey:     now.Add(10 * time.Second).Format(verificationservice.TimestampLayout),
-				},
-				Labels: map[string]string{
-					toolchainv1alpha1.UserSignupUserPhoneHashLabelKey: "+1NUMBER",
-				},
+	s.T().Run("captcha configuration ", func(t *testing.T) {
+		tests := map[string]struct {
+			activationCounterAnnotationValue       string
+			captchaScoreAnnotationValue            string
+			allowLowScoreReactivationConfiguration bool
+			expectedErr                            string
+		}{
+			"captcha score below required score but it's a reactivation": {
+				activationCounterAnnotationValue:       "2",   // user is reactivating
+				captchaScoreAnnotationValue:            "0.5", // and captcha score is low
+				allowLowScoreReactivationConfiguration: true,
 			},
-			Spec: toolchainv1alpha1.UserSignupSpec{
-				Username: "sbryzak@redhat.com",
+			"captcha score below required score but it's not a reactivation": {
+				activationCounterAnnotationValue:       "1",   // first time user
+				captchaScoreAnnotationValue:            "0.5", // and captcha score is low
+				allowLowScoreReactivationConfiguration: true,
+				expectedErr:                            "verification failed: verification is not available at this time",
+			},
+			"activation counter is invalid and captcha score is low": {
+				activationCounterAnnotationValue:       "x",   // something wrong happened
+				captchaScoreAnnotationValue:            "0.5", // and captcha score is low
+				allowLowScoreReactivationConfiguration: true,
+				expectedErr:                            "verification failed: verification is not available at this time",
+			},
+			"activation counter is invalid and captcha score is ok": {
+				activationCounterAnnotationValue:       "x",   // something wrong happened
+				captchaScoreAnnotationValue:            "0.6", // but captcha score is ok
+				allowLowScoreReactivationConfiguration: true,
+			},
+			"allow low score reactivation disabled - captcha score below required score and it's a reactivation": {
+				activationCounterAnnotationValue:       "2",   // user is reactivating
+				captchaScoreAnnotationValue:            "0.5", //  captcha score is low
+				allowLowScoreReactivationConfiguration: false,
+				expectedErr:                            "verification failed: verification is not available at this time",
+			},
+			"allow low score reactivation disabled - captcha score below required score and it's not a reactivation": {
+				activationCounterAnnotationValue:       "1",   // first time user
+				captchaScoreAnnotationValue:            "0.5", //  captcha score is low
+				allowLowScoreReactivationConfiguration: false,
+				expectedErr:                            "verification failed: verification is not available at this time",
+			},
+			"allow low score reactivation disabled - captcha score ok": {
+				activationCounterAnnotationValue:       "1",   // first time user
+				captchaScoreAnnotationValue:            "0.6", //  captcha score is ok
+				allowLowScoreReactivationConfiguration: false,
+			},
+			"no score annotation": {
+				activationCounterAnnotationValue:       "1", // first time user
+				captchaScoreAnnotationValue:            "",  // score annotation is missing
+				allowLowScoreReactivationConfiguration: true,
+				// no error is expected in this case and the verification should proceed
+			},
+			"score annotation is invalid": {
+				activationCounterAnnotationValue:       "1",   // first time user
+				captchaScoreAnnotationValue:            "xxx", // score annotation is invalid
+				allowLowScoreReactivationConfiguration: true,
+				// no error is expected in this case and the verification should proceed
+			},
+			"no activation counter annotation and low captcha score": {
+				activationCounterAnnotationValue:       "",    // activation counter is missing thus required score will be compared with captcha score
+				captchaScoreAnnotationValue:            "0.5", // score is low thus verification will fail
+				allowLowScoreReactivationConfiguration: true,
+				expectedErr:                            "verification failed: verification is not available at this time",
+			},
+			"no activation counter annotation and captcha score ok": {
+				activationCounterAnnotationValue:       "",    // activation counter is missing thus required score will be compared with captcha score
+				captchaScoreAnnotationValue:            "0.6", // score is ok thus verification will succeed
+				allowLowScoreReactivationConfiguration: true,
 			},
 		}
-		states.SetVerificationRequired(userSignup, true)
-
-		err := s.FakeUserSignupClient.Delete(userSignup.Name, nil)
-		require.NoError(s.T(), err)
-
-		err = s.FakeUserSignupClient.Tracker.Add(userSignup)
-		require.NoError(s.T(), err)
-
-		ctx, _ := gin.CreateTestContext(httptest.NewRecorder())
-		err = s.Application.VerificationService().VerifyPhoneCode(ctx, userSignup.Name, userSignup.Spec.Username, "123456")
-		require.EqualError(s.T(), err, "verification failed: verification is not available at this time")
-
-		_, err = s.FakeUserSignupClient.Get(userSignup.Name)
-		require.NoError(s.T(), err)
+		for k, tc := range tests {
+			t.Run(k, func(t *testing.T) {
+				// when
+				s.OverrideApplicationDefault(
+					testconfig.RegistrationService().Verification().CaptchaRequiredScore("0.6"),
+					testconfig.RegistrationService().Verification().CaptchaAllowLowScoreReactivation(tc.allowLowScoreReactivationConfiguration),
+				)
+				userSignup := &toolchainv1alpha1.UserSignup{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "123",
+						Namespace: configuration.Namespace(),
+						Annotations: map[string]string{
+							toolchainv1alpha1.UserSignupUserEmailAnnotationKey:        "sbryzak@redhat.com",
+							toolchainv1alpha1.UserVerificationAttemptsAnnotationKey:   "0",
+							toolchainv1alpha1.UserSignupVerificationCodeAnnotationKey: "123456",
+							toolchainv1alpha1.UserVerificationExpiryAnnotationKey:     now.Add(10 * time.Minute).Format(verificationservice.TimestampLayout),
+						},
+						Labels: map[string]string{
+							toolchainv1alpha1.UserSignupUserPhoneHashLabelKey: "+1NUMBER",
+						},
+					},
+					Spec: toolchainv1alpha1.UserSignupSpec{
+						Username: "sbryzak@redhat.com",
+					},
+				}
+				if tc.activationCounterAnnotationValue != "" {
+					userSignup.Annotations[toolchainv1alpha1.UserSignupActivationCounterAnnotationKey] = tc.activationCounterAnnotationValue
+				}
+				if tc.captchaScoreAnnotationValue != "" {
+					userSignup.Annotations[toolchainv1alpha1.UserSignupCaptchaScoreAnnotationKey] = tc.captchaScoreAnnotationValue
+				}
+				states.SetVerificationRequired(userSignup, true)
+
+				_, err := s.FakeUserSignupClient.Get(userSignup.Name)
+				if err == nil {
+					// delete the usersignup, if exists, before adding the new one
+					err = s.FakeUserSignupClient.Delete(userSignup.Name, nil)
+					require.NoError(s.T(), err)
+				}
+
+				err = s.FakeUserSignupClient.Tracker.Add(userSignup)
+				require.NoError(s.T(), err)
+
+				ctx, _ := gin.CreateTestContext(httptest.NewRecorder())
+				err = s.Application.VerificationService().VerifyPhoneCode(ctx, userSignup.Name, userSignup.Spec.Username, "123456")
+
+				// then
+				if tc.expectedErr != "" {
+					require.EqualError(s.T(), err, tc.expectedErr)
+					_, err = s.FakeUserSignupClient.Get(userSignup.Name)
+					require.NoError(s.T(), err)
+				} else {
+					require.NoError(s.T(), err)
+					userSignup, err = s.FakeUserSignupClient.Get(userSignup.Name)
+					require.NoError(s.T(), err)
+					require.False(s.T(), states.VerificationRequired(userSignup))
+				}
+			})
+		}
 	})
 }