-
Notifications
You must be signed in to change notification settings - Fork 145
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.
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
Allow Google ReCaptcha Enterprise #239
Conversation
Modify readme to include usage.
In general, I was also wondering can we not just simply replace the this.grecaptcha method with this.grecaptcha.enterprise in the render method (if it exists)? I'm yet to try that out, but should mostly work. |
If this works, can we get it merged in? My team is using a fork for enterprise, but it'd be great to see it adopted into this lib instead. Let me know if there's anything I could do to help. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd make sure to double-check the modules diligently, and perhaps remove the hostname interpolation when generating the URL
@@ -11,6 +11,9 @@ function getOptions() { | |||
function getURL() { | |||
const dynamicOptions = getOptions(); | |||
const hostname = dynamicOptions.useRecaptchaNet ? "recaptcha.net" : "www.google.com"; | |||
if (dynamicOptions.enterprise) { | |||
return `https://${hostname}/recaptcha/enterprise.js?onload=${callbackName}&render=explicit`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is recaptcha.net a valid domain for enterprise? I've never seen this myself. Might be safer to assume not unless you can confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@moonkid196 I would think both options would work because recaptcha.net is really just a redirect on the other domain to avoid blocks on google's domain
let getResponse = this.props.grecaptcha && this.props.grecaptcha.getResponse; | ||
if (this.props.grecaptcha.enterprise) { | ||
getResponse = this.props.grecaptcha.enterprise.getResponse(); | ||
} | ||
if (getResponse && this._widgetId !== undefined) { | ||
return getResponse(this._widgetId); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, you're trying to figure out the method to call? I feel like this is more complex than it needs to be. The biggest condition is that the widget is undefined, so maybe rule that out first?
let getResponse = this.props.grecaptcha && this.props.grecaptcha.getResponse; | |
if (this.props.grecaptcha.enterprise) { | |
getResponse = this.props.grecaptcha.enterprise.getResponse(); | |
} | |
if (getResponse && this._widgetId !== undefined) { | |
return getResponse(this._widgetId); | |
} | |
if (this._widgetId === undefined || this.props.grecaptcha === undefined) { | |
return null; | |
} | |
if (this.props.grecaptcha.enterprise) { | |
return this.props.grecaptcha.enterprise.getResponse(this._widgetId); | |
} | |
return this.props.grecaptcha.getResponse(this._widgetId); | |
} |
Either way, you may want to remove the parentheses on line 16, I think
let resetter = this.props.grecaptcha.reset; | ||
if (this.props.grecaptcha.enterprise.reset) { | ||
resetter = this.props.grecaptcha.enterprise.reset; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Think you need to check the intervening values here too:
let resetter = this.props.grecaptcha.reset; | |
if (this.props.grecaptcha.enterprise.reset) { | |
resetter = this.props.grecaptcha.enterprise.reset; | |
} | |
if (this._widgetId === undefined || this.props.grecaptcha === undefined) { | |
this._executeRequested = true; | |
else if (this.props.grecaptcha.enterprise) { | |
this.props.grecaptcha.enterprise.reset(this._widgetId); | |
else { | |
this.props.grecaptcha.reset(this._widgetId); | |
} |
this.props.grecaptcha.enterprise && | ||
this.props.grecaptcha.enterprise.render | ||
) { | ||
render = this.props.grecaptcha.enterprise.render; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably don't need this last one:
this.props.grecaptcha.enterprise && | |
this.props.grecaptcha.enterprise.render | |
) { | |
render = this.props.grecaptcha.enterprise.render; | |
this.props.grecaptcha.enterprise | |
) { | |
render = this.props.grecaptcha.enterprise.render; |
... although the original code has this shrug
Hi, is there a workaround to make this library work with enterprise without this PR being merged as of now? |
When is this change expected to be merged? |
I borrowed some ideas from this PR and implemented it myself with the following notes, steps: First, I use the flow from create-react-app; ymmv Next, added a line like this to the anchoring/static + <script src="https://www.google.com/recaptcha/enterprise.js?render=XXXXXXXXXXXX" async defer></script> Created a stripped-down version of this PR, with my own elements (I use firebase, so I have a function to call assessment API referenced here and return the result): class Recaptcha {
constructor() {
this.recaptcha = window.grecaptcha;
this.checkLoaded = this.checkLoaded.bind(this);
this.isLoaded = this.isLoaded.bind(this);
this.justVerify = this.justVerify.bind(this);
this.timerID = setInterval(this.checkLoaded, 100);
}
async checkLoaded() {
if (!this.recaptcha && window.grecaptcha) {
this.recaptcha = window.grecaptcha;
console.info("recaptcha library loaded");
}
}
isLoaded() {
return (this.recaptcha !== undefined);
}
async getToken(action) {
if (!this.isLoaded()) {
throw new Error("recaptcha libraries not yet loaded");
}
const token = await this.recaptcha.enterprise.execute(
RecaptchaSiteKey, { action: action }
);
return token;
}
async justVerify(action) {
if (!this.isLoaded()) {
throw new Error("recaptcha libraries not yet loaded");
}
this.recaptcha.enterprise.ready(async () => {
const token = await this.recaptcha.enterprise.execute(
RecaptchaSiteKey, { action: action }
);
const result = await justVerify(
{ token: token, action: action }
);
console.debug(
`recaptcha result: ${action}: ${JSON.stringify(result)}`
);
});
}
} Import it and pass it into routes (using react router here): +const recaptcha = new Recaptcha();
const root = ReactDOM.createRoot(document.getElementById('root'));
root.render(
<React.StrictMode>
<BrowserRouter>
<Routes>
- <Route path="/" element={<App />} />
+ <Route path="/" element={<App recaptcha={recaptcha} />} /> Set up some state in App: + this.state = { loaded: true };
+ this.pageUtils = new PageUtils(this); And finally, the definition of page utils that handles the loading, kicks off the assessment: class PageUtils {
constructor(component) {
this.component = component;
this.firstLoad = true;
this.ensureLoaded = this.ensureLoaded.bind(this);
this.navClick = this.navClick.bind(this);
this.timerID = setInterval(this.ensureLoaded, 100);
if (!this.component.props.recaptcha.isLoaded()) {
this.component.state.loaded = false;
}
}
async ensureLoaded() {
if (
!this.component.state.loaded && this.component.props.recaptcha.isLoaded()
) {
this.component.setState({ loaded: true });
}
if (this.component.state.loaded && this.firstLoad) {
await this.component.props.recaptcha.justVerify(RecaptchaActions.PageLoad);
if (this.timerID) {
clearInterval(this.timerID);
}
}
}
async navClick(event) {
await this.component.props.recaptcha.justVerify(RecaptchaActions.UserClick);
}
} It's not the best code, and when I have time, I'll probably improve it in a few ways (use the lifecycle methods to trigger the page-load assessments, maybe turn PageUtils into a (hidden) component, for example), but it may help get you going |
What's holding back this PR from being merged? |
@dozoisch Can you please merge this, if it has no conflicts? Thanks! |
This change allows Google ReCaptcha Enterprise usage. You can set the windows global dynamic option, enterprise to true to get this functionality.
I have an Enterprise account and I tested there.
Please let me know if you have any questions/comments.
Thanks.