Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotate captcha assessments #1043

Merged
merged 9 commits into from
Jun 26, 2024
Merged

Annotate captcha assessments #1043

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a936530
Annotate captcha assessments
rajivnathan Jun 18, 2024
3ef1fe0
Merge branch 'master' of github.com:codeready-toolchain/host-operator…
rajivnathan Jun 18, 2024
94e1342
Check whether assessment was already annotated
rajivnathan Jun 18, 2024
3e1681d
Do assessment annotation in go routine
rajivnathan Jun 18, 2024
24f3dc8
Update api dependency
rajivnathan Jun 18, 2024
c4fb455
Rename func to getCaptchaAssessmentAnnotation
rajivnathan Jun 19, 2024
d0e0d02
Setup captcha credentials for client and gate assessment annotations …
rajivnathan Jun 19, 2024
80ccd44
Add unit tests
rajivnathan Jun 25, 2024
a8b3ca6
Merge branch 'master' of github.com:codeready-toolchain/host-operator…
rajivnathan Jun 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 75 additions & 1 deletion controllers/usersignup/usersignup_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import (
"context"
"fmt"

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"github.com/codeready-toolchain/host-operator/controllers/toolchainconfig"
"github.com/codeready-toolchain/host-operator/pkg/capacity"
Expand All @@ -22,6 +23,11 @@
"github.com/go-logr/logr"
errs "github.com/pkg/errors"
"github.com/redhat-cop/operator-utils/pkg/util"

"strconv"

recaptcha "cloud.google.com/go/recaptchaenterprise/v2/apiv1"
recaptchapb "cloud.google.com/go/recaptchaenterprise/v2/apiv1/recaptchaenterprisepb"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -34,7 +40,6 @@
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"
"strconv"
)

type StatusUpdaterFunc func(ctx context.Context, userAcc *toolchainv1alpha1.UserSignup, message string) error
Expand Down Expand Up @@ -532,6 +537,10 @@
if state == toolchainv1alpha1.UserSignupStateLabelValueApproved {
activations = r.updateActivationCounterAnnotation(logger, userSignup)
}

// annotate the original captcha assessment, if possible
r.annotateCaptchaAssessment(ctx, userSignup, state)

if err := r.Client.Update(ctx, userSignup); err != nil {
return r.wrapErrorWithStatusUpdate(ctx,
userSignup, r.setStatusFailedToUpdateStateLabel, err,
Expand Down Expand Up @@ -756,6 +765,71 @@
return nil
}

// annotateCaptchaAssessment attempts to annotate the original assessment (if any) to provide feedback so that future scores can be improved
// the attempt to annotate the previous assessment is a best-effort, we do not want to reconcile again if the request fails because captcha is a separate service
// and should not have any impact to the UserSignup flows if there are issues with this request
func (r *Reconciler) annotateCaptchaAssessment(ctx context.Context, userSignup *toolchainv1alpha1.UserSignup, newState string) {
logger := log.FromContext(ctx)
assessmentID, assessmentIDFound := userSignup.Annotations[toolchainv1alpha1.UserSignupCaptchaAssessmentIDAnnotationKey]
if !assessmentIDFound {
// there's no assessment ID to annotate
return
}

newAssessmentAnnotation := r.shouldAnnotateCaptchaAssessment(userSignup, newState)
if newAssessmentAnnotation == recaptchapb.AnnotateAssessmentRequest_ANNOTATION_UNSPECIFIED {

Check warning on line 780 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L779-L780 

Added lines #L779 - L780 were not covered by tests
alexeykazakov marked this conversation as resolved.
Show resolved Hide resolved
// no need to annotate the previous assessment
return

Check warning on line 782 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L782 

Added line #L782 was not covered by tests
}

gctx := context.Background()
rclient, err := recaptcha.NewClient(gctx)
if err != nil {
logger.Error(err, "error creating reCAPTCHA client, cannot annotate assessment")
return

Check warning on line 789 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L785-L789 

Added lines #L785 - L789 were not covered by tests
}
defer rclient.Close()

Check warning on line 791 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L791 

Added line #L791 was not covered by tests

annotateRequest := &recaptchapb.AnnotateAssessmentRequest{
Name: assessmentID,
Annotation: newAssessmentAnnotation,

Check warning on line 795 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L793-L795 

Added lines #L793 - L795 were not covered by tests
}

response, err := rclient.AnnotateAssessment(gctx, annotateRequest)
alexeykazakov marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
logger.Error(err, "error annotating assessment")
return

Check warning on line 801 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L798-L801 

Added lines #L798 - L801 were not covered by tests
}

newAnnotationName, isFound := recaptchapb.AnnotateAssessmentRequest_Annotation_name[int32(newAssessmentAnnotation)]
if isFound {

Check warning on line 805 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L804-L805 

Added lines #L804 - L805 were not covered by tests
// set the annotated assessment value: FRAUDULENT or LEGITIMATE
userSignup.Annotations[toolchainv1alpha1.UserSignupCaptchaAnnotatedAssessmentAnnotationKey] = newAnnotationName

Check warning on line 807 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L807 

Added line #L807 was not covered by tests
}
logger.Info("Assessment annotated successfully", "assessment_annotation", newAnnotationName, "response", response.String())

Check warning on line 809 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L809 

Added line #L809 was not covered by tests
}

// shouldAnnotateCaptchaAssessment determines whether the previous assessment should be annotated
// returns LEGITIMATE or FRAUDULENT if the assessment should be annotated
// returns ANNOTATION_UNSPECIFIED otherwise
func (r *Reconciler) shouldAnnotateCaptchaAssessment(userSignup *toolchainv1alpha1.UserSignup, newState string) recaptchapb.AnnotateAssessmentRequest_Annotation {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name is a bit misleading, isn't it? It doesn't determine if it should be annotated or not but rather always returns a new/expected assessment annotation, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using ANNOTATION_UNSPECIFIED as the base case where we don't actually do the annotation. In the other function we check whether it's ANNOTATION_UNSPECIFIED and if so we skip annotating the assessment. But I'm fine to change it if it's confusing. Do you have a suggestion to make it more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed the function to getCaptchaAssessmentAnnotation, let me know if you think another name would be better

oldAnnotatedAssessment := userSignup.Annotations[toolchainv1alpha1.UserSignupCaptchaAnnotatedAssessmentAnnotationKey]
fraudulentAnnotationName := recaptchapb.AnnotateAssessmentRequest_Annotation_name[int32(recaptchapb.AnnotateAssessmentRequest_FRAUDULENT)]

Check warning on line 817 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L815-L817 

Added lines #L815 - L817 were not covered by tests
// user was incorrectly banned if the previous assessment was annotated as fraudulent and the user is now being approved
wasUserIncorrectlyBanned := oldAnnotatedAssessment == fraudulentAnnotationName && newState == toolchainv1alpha1.UserSignupStateLabelValueApproved

Check warning on line 819 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L819 

Added line #L819 was not covered by tests

// determine whether the previous assessment needs to be annotated
newAnnotation := recaptchapb.AnnotateAssessmentRequest_ANNOTATION_UNSPECIFIED
if newState == toolchainv1alpha1.UserSignupStateLabelValueBanned {

Check warning on line 823 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L822-L823 

Added lines #L822 - L823 were not covered by tests
// user is banned so send an annotation response to provide feedback that this user was fraudulent
newAnnotation = recaptchapb.AnnotateAssessmentRequest_FRAUDULENT
} else if wasUserIncorrectlyBanned {

Check warning on line 826 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L825-L826 

Added lines #L825 - L826 were not covered by tests
// user was mistakenly banned and is now approved so send an annotation response to provide feedback that this user was legitimate
newAnnotation = recaptchapb.AnnotateAssessmentRequest_LEGITIMATE

Check warning on line 828 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L828 

Added line #L828 was not covered by tests
}
return newAnnotation

Check warning on line 830 in controllers/usersignup/usersignup_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/usersignup/usersignup_controller.go#L830 

Added line #L830 was not covered by tests
}

// updateActivationCounterAnnotation increments the 'toolchain.dev.openshift.com/activation-counter' annotation value on the given UserSignup
func (r *Reconciler) updateActivationCounterAnnotation(logger logr.Logger, userSignup *toolchainv1alpha1.UserSignup) int {
if activations, exists := userSignup.Annotations[toolchainv1alpha1.UserSignupActivationCounterAnnotationKey]; exists {
Expand Down
47 changes: 32 additions & 15 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
module github.com/codeready-toolchain/host-operator

require (
github.com/codeready-toolchain/api v0.0.0-20240530120602-c11598ccffb7
cloud.google.com/go/recaptchaenterprise/v2 v2.13.0
github.com/codeready-toolchain/api v0.0.0-20240607180719-368c7afbaebe
github.com/codeready-toolchain/toolchain-common v0.0.0-20240613121043-7e6ef858cdff
github.com/davecgh/go-spew v1.1.1 // indirect
github.com/go-bindata/go-bindata v3.1.2+incompatible
github.com/go-logr/logr v1.2.3
github.com/go-logr/logr v1.4.1
github.com/gofrs/uuid v4.4.0+incompatible
github.com/mailgun/mailgun-go/v4 v4.8.1
// using latest commit from 'github.com/openshift/api branch release-4.12'
Expand All @@ -30,10 +31,12 @@ require (
sigs.k8s.io/controller-runtime v0.13.0
)

require github.com/google/uuid v1.6.0 // indirect
replace github.com/codeready-toolchain/api => github.com/rajivnathan/api v0.0.0-20240617210722-9ea15855d168

require (
cloud.google.com/go v0.97.0 // indirect
cloud.google.com/go/auth v0.3.0 // indirect
cloud.google.com/go/auth/oauth2adapt v0.2.2 // indirect
cloud.google.com/go/compute/metadata v0.3.0 // indirect
github.com/Azure/go-autorest v14.2.0+incompatible // indirect
github.com/Azure/go-autorest/autorest v0.11.27 // indirect
github.com/Azure/go-autorest/autorest/adal v0.9.20 // indirect
Expand All @@ -49,25 +52,30 @@ require (
github.com/PuerkitoBio/urlesc v0.0.0-20170810143723-de5bf2ad4578 // indirect
github.com/beorn7/perks v1.0.1 // indirect
github.com/bmizerany/assert v0.0.0-20160611221934-b7ed37b82869 // indirect
github.com/cespare/xxhash/v2 v2.1.2 // indirect
github.com/cespare/xxhash/v2 v2.2.0 // indirect
github.com/cloudflare/circl v1.3.7 // indirect
github.com/emicklei/go-restful/v3 v3.8.0 // indirect
github.com/evanphx/json-patch v5.6.0+incompatible // indirect
github.com/evanphx/json-patch/v5 v5.6.0 // indirect
github.com/fsnotify/fsnotify v1.5.4 // indirect
github.com/go-logr/stdr v1.2.2 // indirect
github.com/go-logr/zapr v1.2.3 // indirect
github.com/go-openapi/jsonpointer v0.19.5 // indirect
github.com/go-openapi/jsonreference v0.19.5 // indirect
github.com/go-openapi/swag v0.19.14 // indirect
github.com/gogo/protobuf v1.3.2 // indirect
github.com/golang-jwt/jwt/v4 v4.2.0 // indirect
github.com/golang/groupcache v0.0.0-20210331224755-41bb18bfe9da // indirect
github.com/golang/protobuf v1.5.2 // indirect
github.com/golang/protobuf v1.5.4 // indirect
github.com/google/gnostic v0.5.7-v3refs // indirect
github.com/google/go-cmp v0.5.9 // indirect
github.com/google/go-cmp v0.6.0 // indirect
github.com/google/go-github/v52 v52.0.0 // indirect
github.com/google/go-querystring v1.1.0 // indirect
github.com/google/gofuzz v1.1.0 // indirect
github.com/google/s2a-go v0.1.7 // indirect
github.com/google/uuid v1.6.0 // indirect
github.com/googleapis/enterprise-certificate-proxy v0.3.2 // indirect
github.com/googleapis/gax-go/v2 v2.12.3 // indirect
github.com/gorilla/mux v1.8.0 // indirect
github.com/h2non/parth v0.0.0-20190131123155-b4df798d6542 // indirect
github.com/huandu/xstrings v1.3.1 // indirect
Expand All @@ -89,18 +97,27 @@ require (
github.com/segmentio/backo-go v1.0.0 // indirect
github.com/shopspring/decimal v1.2.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
go.opencensus.io v0.24.0 // indirect
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.49.0 // indirect
go.opentelemetry.io/otel v1.24.0 // indirect
go.opentelemetry.io/otel/metric v1.24.0 // indirect
go.opentelemetry.io/otel/trace v1.24.0 // indirect
go.uber.org/atomic v1.7.0 // indirect
go.uber.org/multierr v1.6.0 // indirect
golang.org/x/crypto v0.21.0 // indirect
golang.org/x/net v0.23.0 // indirect
golang.org/x/oauth2 v0.7.0 // indirect
golang.org/x/sys v0.18.0 // indirect
golang.org/x/term v0.18.0 // indirect
golang.org/x/crypto v0.22.0 // indirect
golang.org/x/net v0.24.0 // indirect
golang.org/x/oauth2 v0.19.0 // indirect
golang.org/x/sync v0.7.0 // indirect
golang.org/x/sys v0.19.0 // indirect
golang.org/x/term v0.19.0 // indirect
golang.org/x/text v0.14.0 // indirect
golang.org/x/time v0.3.0 // indirect
golang.org/x/time v0.5.0 // indirect
gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/protobuf v1.33.0 // indirect
google.golang.org/api v0.177.0 // indirect
google.golang.org/genproto/googleapis/api v0.0.0-20240429193739-8cf5692501f6 // indirect
google.golang.org/genproto/googleapis/rpc v0.0.0-20240429193739-8cf5692501f6 // indirect
google.golang.org/grpc v1.63.2 // indirect
google.golang.org/protobuf v1.34.0 // indirect
gopkg.in/inf.v0 v0.9.1 // indirect
gopkg.in/yaml.v3 v3.0.1 // indirect
k8s.io/component-base v0.25.0 // indirect
Expand Down
Loading
Loading