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

add automatic verification threshold field #390

Merged
merged 3 commits into from
Nov 29, 2023
Merged

add automatic verification threshold field #390

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
94a8f68
add automatic verification threshold field
mfrancisc Nov 28, 2023
baa8ab5
add allowLowScoreReactivation field
mfrancisc Nov 29, 2023
9247ee8
rename to RequiredScore
mfrancisc Nov 29, 2023
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
5 changes: 5 additions & 0 deletions api/v1alpha1/toolchainconfig_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,11 @@ type CaptchaConfig struct {
// +optional
ScoreThreshold *string `json:"scoreThreshold,omitempty"`

// AutomaticVerificationThreshold defines the lowest captcha score for automatic approval to be enabled.
// If the captcha score is below this threshold then manual approval is required in order for the user to be provisioned.
// +optional
AutomaticVerificationThreshold *string `json:"automaticVerificationThreshold,omitempty"`
Copy link
Contributor

@MatousJobanek MatousJobanek Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would maybe call it
MinimalRequiredScoreThreshold or just MinimalRequiredScore because if the user gets a low score, then we don't let the user even try to complete the phone verification. In other words, it's not only about automatic approval - that's what the ScoreThreshold field is used for actually:

// ScoreThreshold defines the captcha assessment score threshold. A score equal to or above the threshold means the user is most likely human and
// can proceed signing up but a score below the threshold means the score is suspicious and further verification may be required.
// +optional
ScoreThreshold *string `json:"scoreThreshold,omitempty"`

but even not letting users try to proceed with additional verification steps nor the signup process at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion @MatousJobanek !

I was unsure about the naming (as usual 😅 ) , I thought about something using the words minimal required . But TBH to me it doesn't explain the purpose of the configuration. It may not be clear what happens if the score is lower then the config. While the "automatic verification" implies that the user can proceed with the verification (phone, email, 2fa ... ) without an admin approval/intervention. Maybe autonomous or self verification would be better words 🤔

But again I don't have a strong opinion on the naming, whatever makes more sense to people 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, it's not only about automatic approval - that's what the ScoreThreshold field is used for actually

Well... automatic approval is configured via AutomaticApprovalConfig. Not via ScoreThrershold. Even if PhoneVerification is required it's still auto approval if it's enabled in AutomaticApprovalConfiguration. Vs. a manual approval by an admin.
We could move this new field to AutomaticApprovalConfig and call it something like MinimalRequiredCaptchaScore or keep it under the Captcha configuration and call it something like MinimalRequiredScoreForAutoApproval if it's not too long :)
Otherwise I would probably prefer @mfrancisc's variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm idiot, I read "AutomaticApprovalThreshold" instead of what is there "AutomaticVerificationThreshold". I have some issues with my eyes in the last two days, so it's sometimes hard to focus on the words on the display, or I just maybe need a coffee.

I got maybe also confused by the comment talking about "automatic approval:

// AutomaticVerificationThreshold defines the lowest captcha score for automatic approval to be enabled.
// If the captcha score is below this threshold then manual approval is required in order for the user to be provisioned.

Because as Alexey mentioned, this threshold doesn't have anything to do with the automatic/manual approval, it's about proceeding with the signup process.
I would at least drop the "Automatic" prefix to not confuse more people.

so other ideas:

  • SignupProcessThreshold
  • RequiredScore
  • HardStopThreshold
  • GatekeeperScore
  • SignupConfidenceLimit

Or just go with your name and update the comments so it doesn't talk about the automatic/manual approval, but rather that the user cannot proceed with the signup process at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, and yeah the comment it's unclear.

Thanks for your suggestions, I like both SignupProcessThreshold and RequiredScore.
Maybe we can go with RequiredScore since it's also closer to what @alexeykazakov is suggesting ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will be config.Spec.Host.RegistrationService.Verification.Captcha.RequiredScore

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed in 9247ee8

I've also added a new field in baa8ab5 as per slack discussion.


// SiteKey defines the recaptcha site key to use when making recaptcha requests. There can be different ones for different environments. eg. dev, stage, prod
// +optional
SiteKey *string `json:"siteKey,omitempty"`
Expand Down
5 changes: 5 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions api/v1alpha1/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.