DethAriel / ng-recaptcha

Angular component for Google reCAPTCHA
https://dethariel.github.io/ng-recaptcha/
MIT License
473 stars 123 forks source link

Reactive forms refresh #201

Closed Jordan-Hall closed 3 years ago

Jordan-Hall commented 3 years ago

Summary

I'm submitting a:

When using the component inside reactive forms I've notice a bug. When you refresh the page the component has a valid token but, not showing it it ticked

DethAriel commented 3 years ago

Hi @Jordan-Hall, and thank you for reaching out!

When you say "refresh the page" -- what exactly do you mean, can you elaborate? How are you getting the "valid token" from the component?

Jordan-Hall commented 3 years ago

So i completed the form, captcha. I then refresh the website. The captcha apparently still contains a value but not ticked

DethAriel commented 3 years ago

@Jordan-Hall

The captcha apparently still contains a value but not ticked

What makes you think that captcha still contains a value? reCAPTCHA value is only ever stored in memory, and is not preserved upon page reloads

Jordan-Hall commented 3 years ago

@DethAriel because when i debugged the code it still had a value in the reactive form

DethAriel commented 3 years ago

@Jordan-Hall can you come up with a repro code sample and step-by-step instuctions? This behavior certainly sounds unexpected, but unfortunately I can't reproduce myself.

mokipedia commented 3 years ago

I am seeing the same issue with reactive form. I think what @Jordan-Hall wanted to say is not "reload" the page but destroy the recaptcha component. At least for me, having the component in an ngIf does exactly what @Jordan-Hall described - the captcha is not ticked anymore but the formcontrol is still valid and filled with the previous value. clearing the formcontrol by yourself also breaks the view. here is a stackblitz with a ngIf on a timer to show what is going on: https://stackblitz.com/edit/angular-ivy-htkcc9?devtoolsheight=33&file=src/app/app.component.ts

My workflow: 1) form gets filled, also captcha is valid 2) form gets submitted, spinner is shown instead of form 3) form submit ended (with or without error does not matter) 4) form says it's valid, captcha value still there, but check is gone

mokipedia commented 3 years ago

up

mokipedia commented 3 years ago

a workaround for this issue (at least mine, @Jordan-Hall if what i describe is not your issue, pls tell me and I will open up another issue) is to not use formValueAccessor and instead use the resolved event to manually patch the form. That way I can at least reset the formControl to be aligned with the recaptcha component

DethAriel commented 3 years ago

Thank you very much for the reproduction scenario @mokipedia! I can now reliably reproduce the behavior.

At this point I'm still on the fence whether to treat this as a bug, and if so - whether to fix it or not. The value provided as reCAPTCHA responses are supposed to be single-use only, and after grabbing it and passing it to the server, the reasonable expectation is that developers will reset the <re-captcha> component. However, the behavior demonstrated by your repro scenario does feel counter-intuitive, hence I'm undecided still. While I'm trying to figure out whether this warrants a fix, I want to call out that doing so would likely involve a component breaking change.

If you (or anyone else) are looking for a way to avoid this unfortunate situation, my recommendation would be to manually reset <re-captcha> component after using its value, something like this:

public onFromSubmit() {
  this.tryLoginUser(this.formGroup.value)
    .subscribe(() => {
      // if success, probably navigate away
      this.navigate();
    }, (error) => {
      // if error, and reCAPTCHA challenge needs to be presented again, reset its value
      this.formGroup.setValue({ captcha: null });
    });
}
mokipedia commented 3 years ago

@DethAriel have you tried that? I tried to patch or set value, in that case your formvalueaccessor screamed at me that this is not allowed. (Haven't tried since writing my workaround, so maybe it changed?)

DethAriel commented 3 years ago

@mokipedia yes, this does result in a console error from the value accessor (which is another avenue for a fix), however this error doesn't really affect the app in any detrimental way

mokipedia commented 3 years ago

@DethAriel I would agree not fixing this might be the way to go as long as you allow a reset without error (e.g. with control value accessor)

However doing a reset like you suggest actually breaks logic in a form I am using it, hence the workaround. Not sure what exactly happens but the form is unusable afterwards.

DethAriel commented 3 years ago

@mokipedia understood, thanks for working through this issue with me. I think I have an idea of how to fix this now, so I'll be working on a code update. What ng-recaptcha version are you on? I would like to better understand the extend of fix back-porting that I'll need to go through

mokipedia commented 3 years ago

7.0.1 at the time of first noticing the error due to a code refactor on our side.

DethAriel commented 3 years ago

The error that is generated when one is performing this.formGroup.setValue({ captcha: null }); has been fixed in Angular 11.1.0, it's this one:

forms: clean up connection between FormControl/FormGroup and corresponding directive instances (#39235) (a384961), closes #20007 #37431 #39590

While there is a lib-specific workaround I can implement, I'd rather not go that route. Especially considering that a fix for this issue that I'll back-port to 7.x.x branch will no longer require you to clear the form value manually.

DethAriel commented 3 years ago

This has now been fixed and released in v7.0.2 (for Angular 11) and v8.0.1 (for Angular 12). You can observe the new behavior here.