From a38640a990d35bda363aad20095c40aaeacf50fe Mon Sep 17 00:00:00 2001 From: Francisc Munteanu Date: Wed, 6 Dec 2023 16:27:06 +0100 Subject: [PATCH] make required captcha score configurable (#379) * add captcha requiredScore and allowLowScoreReactivation configurations --------- Co-authored-by: Alexey Kazakov --- go.mod | 4 +- go.sum | 8 +- pkg/configuration/configuration.go | 18 ++ pkg/configuration/configuration_test.go | 6 + .../service/verification_service.go | 46 ++++- .../service/verification_service_test.go | 158 ++++++++++++++---- 6 files changed, 196 insertions(+), 44 deletions(-) diff --git a/go.mod b/go.mod index 395c26f0..06827187 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index f005578e..43bad059 100644 --- a/go.sum +++ b/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= diff --git a/pkg/configuration/configuration.go b/pkg/configuration/configuration.go index 90652639..28362dd1 100644 --- a/pkg/configuration/configuration.go +++ b/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, "") } diff --git a/pkg/configuration/configuration_test.go b/pkg/configuration/configuration_test.go index cc1b88a7..0134829c 100644 --- a/pkg/configuration/configuration_test.go +++ b/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()) }) } diff --git a/pkg/verification/service/verification_service.go b/pkg/verification/service/verification_service.go index e7b35126..a2f4690e 100644 --- a/pkg/verification/service/verification_service.go +++ b/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 diff --git a/pkg/verification/service/verification_service_test.go b/pkg/verification/service/verification_service_test.go index ef6db081..ad507e01 100644 --- a/pkg/verification/service/verification_service_test.go +++ b/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)) + } + }) + } }) }