feat(recaptcha): add mfa sample #8665
Conversation
|
Here is the summary of changes. You are about to add 1 region tag.
This comment is generated by snippet-bot.
|
product-auto-label
bot
added
samples
There was a problem hiding this comment.
The tag recaptcha_enterprise_mfa_assessment is not used in the documentation.
I would challenge the need for this code sample at this point or to reference a relevant bug that justifies the change.
Given the volume of nit comments, can you please go through them and see if there is anything relevant before we LGTM this PR?
recaptcha_enterprise/snippets/src/main/java/recaptcha/AnnotateAssessment.java
Outdated
Show resolved
Hide resolved
recaptcha_enterprise/snippets/src/main/java/recaptcha/CreateAssessment.java
Outdated
Show resolved
Hide resolved
recaptcha_enterprise/snippets/src/main/java/recaptcha/CreateSiteKey.java
Outdated
Show resolved
Hide resolved
recaptcha_enterprise/snippets/src/main/java/recaptcha/UpdateSiteKey.java
Outdated
Show resolved
Hide resolved
| final String HMAC_KEY = "SOME_INTERNAL_UNSHARED_KEY"; | ||
| final String hmacKey = "SOME_INTERNAL_UNSHARED_KEY"; |
There was a problem hiding this comment.
It is a constant. The original naming was correct. Please, rollback.
There was a problem hiding this comment.
Agreed about the naming convention. But having a static string format within main() throws lint as its not the norm.
...rise/snippets/src/main/java/recaptcha/accountdefender/AnnotateAccountDefenderAssessment.java
Outdated
Show resolved
Hide resolved
| @@ -45,7 +45,7 @@ public static void searchRelatedAccountGroupMemberships( | |||
|
|
|||
| SearchRelatedAccountGroupMembershipsRequest request = | |||
| SearchRelatedAccountGroupMembershipsRequest.newBuilder() | |||
| .setProject(projectId) | |||
| .setProject("projects/" + projectId) | |||
There was a problem hiding this comment.
nit: is it necessary? how did it work before this change?
There was a problem hiding this comment.
Yes, This was a recent API change from the product!
| // Creates an assessment to obtain Multi-Factor Authentication result. | ||
| // If the result is unspecified, sends the request token to the caller to initiate MFA challenge. | ||
| // See, https://cloud.google.com/recaptcha-enterprise/docs/integrate-account-verification#understanding_the_configuration_process_of_mfa | ||
| public static void createMfaAssessment( |
There was a problem hiding this comment.
nit: for better testability, I propose to return a status true or false instead of throwing exceptions. The printing to stdout is nice visual but testing the code sample by validating the return value is much simpler than hijacking stdout and matching the strings.
There was a problem hiding this comment.
Returning a boolean in this case might be a bit confusing as the sample by itself has two different possible usecases and paths.
- reCAPTCHA is initialized for the first time and the result is unspecified which then triggers the MFA.
- reCAPTCHA gets the MFA token and returns the result.
Both these flows use this sample and returning a boolean wouldn't be ideal to capture these.
But I agree that in general, it's good to use boolean for testing. Will follow up with internal discussions.
|
Blocked by googleapis/testing-infra-docker#342 |
# Conflicts: # recaptcha_enterprise/snippets/pom.xml
reconcile branch
Add reCAPTCHA Enterprise MFA sample