Plots2: Signup page captcha validation bug

[cesswairimu]

So there is a bug, the signup page if you do not check the captcha or check the animals checkbox it throws the spam detection error but actually the account is already created. That is why the test is failing. I think something broke
Looping in @shreyaa-s too

_Originally posted by @cesswairimu in https://github.com/publiclab/plots2/issues/8460#issuecomment-704449628_

[jywarren]

Strange, I don't see any changes to server-side validation in https://github.com/publiclab/plots2/pull/7768/files, and I'm wondering if that was actually the issue that caused this? Let me walk through the validation steps on the server side for failing captcha, one step at a time.

[jywarren]

My orig response was:

Ah i see. Yes, ok, let's revert #7768 and resolve related bugs and circle back to re-examine. Thank you @cesswairimu and ccing @shreyaa-sharmaa (shreya's new handle) in relation -- no worries, this is good to have caught! Note the tests we changed in #8461 as a result, we'd want to revert those back too...

However maybe this error existed earlier somehow?

[cypherean]

The ID creating even when spam test was not passed? This existed before too. I remember having a conversation about this with @VladimirMikulic. I'll try finding that Github thread for reference.

[cypherean]

https://github.com/publiclab/plots2/issues/7618#issuecomment-602606103 here's the comment that I remembered.
So sorry for the trouble that PR caused. It worked fine when I added it. I'll see what I missed.

[jywarren]

Thanks @shreyaa-sharmaa and no worries -- even if it did start with that PR i don't think you're at fault, there's something on the server side validations happening and I don't think you touched that! We're also coordinating in the chatroom too just FYI - https://gitter.im/publiclab/publiclab. THANK YOU for looking in here and don't stress. Also isn't it way late there? Get some sleep, no worries!

[jywarren]

Also noting that /signup shows all validations red on initial load:

Which is also not ideal but probably unrelated... i'll spin that out into a new issue just after this!

[jywarren]

Just noting that indeed #7768 was the first instance of the system test failure, /after/ merging, so it may be related to auto-merging of the files -- https://travis-ci.com/github/publiclab/plots2/jobs/394444026#L2486

[jywarren]

Just one change to /app/views/users/_create_form.html.erb on June 1st -- https://github.com/publiclab/plots2/commits/main/app/views/users/_create_form.html.erb

[cypherean]

Also noting that /signup shows all validations red on initial load:

Which is also not ideal but probably unrelated... i'll spin that out into a new issue just after this!

It is related. Can be traced to this one line https://github.com/publiclab/plots2/blob/0c3dc208cf5017bad581c356fbcf00f482a5a75e/app/assets/javascripts/validation.js#L49 . Adding a condition for validating form on page reload only if all fields are filled should do the trick.

[jywarren]

ah ok thank you @shreyaa-sharmaa -- will spin that out as its own issue. Confirming what @cesswairimu noted too though wiht more docs, we should write a system test for this:

the spam validation doesn't stop the user from being created:

[cypherean]

@jywarren @cesswairimu how can I help?
The system tests passed locally and I was hoping they would give some idea where to start looking for the error.

[jywarren]

Hi Shreyaa - could you try getting a version of the system test running on the current code, and checking against that error Cess found - that submitting the signup form but failing the spam validation still creates a user?

https://github.com/publiclab/plots2/blob/8c9c39da0152d5a965e7eb77697c056ca3d45111/test/system/signup_form_test.rb has your orig system test code. i'm tsting this manually now in gitpod too.

But again, it's late there so no stress, we can do this slower tomorrow!

[jywarren]

i'm checking if we have a corresponding integration test
aha, we actually only test the "leave it empty" box, not the spam detection itself... https://github.com/publiclab/plots2/blob/main/test/integration/signup_flow_test.rb

[jywarren]

https://github.com/publiclab/plots2/blob/cf434b421a8d40c4cc9376a26ced7c83f5e89381/app/controllers/users_controller.rb#L18-L19 is the issue, i think.

User creation does not fail if you fail spamaway -- it's not part of user validation, but is instead part of controller logic. Yes!!!

This issue predates. so we need to:

1. patch this in the controller
2. test against it with functional tests like in https://github.com/publiclab/plots2/pull/8467

[cypherean]

I'm not sure if it was a mistake on my end or the url changed, but entering a taken email in the signup modal redirects to /register and I had mentioned /signup. It's probably the former.

[cypherean]

Hi Shreyaa - could you try getting a version of the system test running on the current code, and checking against that error Cess found - that submitting the signup form but failing the spam validation still creates a user?

https://github.com/publiclab/plots2/blob/8c9c39da0152d5a965e7eb77697c056ca3d45111/test/system/signup_form_test.rb has your orig system test code. i'm tsting this manually now in gitpod too.

But again, it's late there so no stress, we can do this slower tomorrow!

Yeah sure. Should I define another system test?

[cypherean]

I'm not sure if it was a mistake on my end or the url changed, but entering a taken email in the signup modal redirects to /register and I had mentioned /signup. It's probably the former.

Looks like I'm a little slow on catching up. Is there anything else I'm missing out on?

[jywarren]

I believe it's because the results of a validation failure are on a different action which lives under the /register route, even though it actually uses the same template as /signup -- so hopefully unrelated!

Yeah that would be great -- maybe a system test asserting no change in count of users... see how I did that in the integration test I wrote?

https://github.com/publiclab/plots2/pull/8467/files#diff-63fe62e807d12c93fd1b80ff646e6b7aR85

[jywarren]

I don't think so, thanks @shreyaa-sharmaa !!!

Then, we can re-instantiate your original client-side validation code, and ensure that the new tests aren't affected. It's still not clear to me exactly what broke, to be honest... but as long as we test existing functionality thoroughly and don't change those tests, re-adding your validation changes should be perfectly fine!

[jywarren]

What's interesting about the test @cesswairimu changed is that it really should be /register, right? Is this a timing issue, and possibly even unrelated to the spam validation issue we found?

https://github.com/publiclab/plots2/commit/367ef5002ae9da0143dd03945b2a08bcfba4b06b#diff-08ffcd32abee9382e25ec89ed0109c5eR19

Here's an idea - in system tests, if you add a CSS selector assertion, it will wait until page load to complete it. I'm not sure if other assertions do that. So, let's do a CSS selector assertion before checking the path -- say, look for the .alert-error first. Then it'll wait for the page to finish loading before doing that, and we can be sure the page has refreshed before it continues with checking the path.

[cesswairimu]

Yeah it's redirecting to /register but it's a post URL so maybe we need to also do a proper redirection after failure

[cypherean]

I believe it's because the results of a validation failure are on a different action which lives under the /register route, even though it actually uses the same template as /signup -- so hopefully unrelated!

Yeah that would be great -- maybe a system test asserting no change in count of users... see how I did that in the integration test I wrote?

https://github.com/publiclab/plots2/pull/8467/files#diff-63fe62e807d12c93fd1b80ff646e6b7aR85

So if I'm understanding it right, I register a user, leave the spam blank and check if the user count is created?

[jywarren]

That's right, @shreyaa-sharmaa !

Also, awesome, this integration test just caught that: https://github.com/publiclab/plots2/pull/8467/files

Filed the fix too. So any test added to this should not trigger it.

@shreyaa-sharmaa i wonder if you just want to re-add your own test with:

1. the User.count assertion
2. nothing that depends on the new code you'd added in https://github.com/publiclab/plots2/pull/7768/

Then if we can merge that cleanly, we can follow up with your original improved client-side validation code in its own PR, with the additional fix that it doesn't show validation errors on the initial page load. How does this sound? I think it's non-urgent as well, so please don't feel you have to prioritize it tonight unless you really want to!

[jywarren]

https://github.com/publiclab/plots2/blob/8c9c39da0152d5a965e7eb77697c056ca3d45111/test/system/signup_form_test.rb#L8-L29

[jywarren]

OK, will merge https://github.com/publiclab/plots2/pull/8467 once it passes and then leave this page open with the follow-ups of re-adding @shreyaa-sharmaa's code!

[jywarren]

Thanks, all!

[cypherean]

That's right, @shreyaa-sharmaa !

Also, awesome, this integration test just caught that: https://github.com/publiclab/plots2/pull/8467/files

Filed the fix too. So any test added to this should not trigger it.

@shreyaa-sharmaa i wonder if you just want to re-add your own test with:

1. the `User.count` assertion

2. nothing that depends on the new code you'd added in #7768

Then if we can merge that cleanly, we can follow up with your original improved client-side validation code in its own PR, with the additional fix that it doesn't show validation errors on the initial page load. How does this sound? I think it's non-urgent as well, so please don't feel you have to prioritize it tonight unless you really want to!

I'll add this test before heading off and follow back with the fixes tomorrow.

[cypherean]

Linking both here #8469 #8468

[jywarren]

Both completed! Now, just to re-add the original JS validation code and ensure it does not show an error on initial pageload.

[cesswairimu]

awesome :tada: thanks @jywarren . Sorry @shreyaa-s I reverted your changes while it was an existing bug. Thanks to your tests we caught it :+1:

[cesswairimu]

Should we create a follow-up for removing the errors on initial pageload?