Fix event listener race condition on login-form

[SystemKeeper]

Summary

We received reports about the error "State token does not match" when trying to login with talk-ios (see nextcloud/talk-ios#1017). I have seen this error myself from time to time, but wasn't able to pinpoint it to when exactly it happens. I took a closer look at the requests / responses received and noticed, that usually we should see something like this

GET /index.php/login/flow/grant?stateToken=<TOKEN>&clientIdentifier=&user=&direct=0

but in some cases it was just like this

GET /index.php/login/flow/grant?

so without any query parameters. The login process itself (username, password) does work fine, but fails after granting access with the error mentioned above.
The page itself is rendered correctly, so the forms actions include the parameters as seen in the first request above. As this won't work natively (at least not on iOS), we need a small javascript snippet (authpicker.js) to to a redirect, instead of a the native browser action. Problem is that the listener for this to happen is added when the document is ready, but the user is actually able to press "Log in" before the listener is added, therefore doing a native browser action with the query parameters removed.

How to test:

• Comment out this code

  server/core/js/login/authpicker.js

  Lines 9 to 12 in 791a182

  	document.getElementById('login-form').addEventListener('submit', function (e) {
  	e.preventDefault();
  	document.location.href = e.target.attributes.action.value
  	})

• Try to login using flow v1
• See that you receive "State token does not match" at the end

Because this code is still using jQuery and therefore considered legacy, I went for disabling the button until the corresponding jQuery code ran.

• Resolves: State token does not match talk-ios#1017
• Looks like this was introduced in 25, so backport to stable-25 ?

CC: @Ivansss

Checklist

• Code is properly formatted
• Sign-off message is added to all commits
• Tests (unit, integration, api and/or acceptance) are included
• Screenshots before/after for front-end changes
• Documentation (manuals or wiki) has been updated or is not required
• Backports requested where applicable (ex: critical bugfixes)

[artonge]

Makes sense :)

[kesselb]

Thanks for your pull request 👍

I think that could explain this error: #32387

But the error shown to the user is a bit different.

[SystemKeeper]

I think that could explain this error: #32387

But this points to V2?! But maybe a similar error?

[SystemKeeper]

Ah, LoginV2 uses the same script, sorry. In this case we should add „disabled“ to the V2 template as well?

[kesselb]

But this points to V2?!

Well spotted ;)
I don't know to be honest.

[SystemKeeper]

But this points to V2?!

Well spotted ;) I don't know to be honest.

😉

I guess it makes sense, looking at the V2 form we have the same situation here:

server/core/templates/loginflowv2/authpicker.php

Line 49 in ecad09b

<form id="login-form" action="<?php p($urlGenerator->linkToRouteAbsolute('core.ClientFlowLoginV2.grantPage', ['stateToken' => $_['stateToken'], 'user' => $_['user']])) ?>" method="get">

so that could very well lead to the same problem. I’ll take a look at this.

[SystemKeeper]

I think that could explain this error: #32387

I tested this with the steps outlined in the first post and the error in #32387 is then reproducible. I added the same disabled property to the V2 form now.

[ChristophWurst]

🤯

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[kesselb]

/backport to stable25

[kesselb]

/backport to stable24

[ChristophWurst]

/backport to stable25

[ChristophWurst]

/backport to stable24

[backportbot-nextcloud]

The backport to stable25 failed. Please do this backport manually.

[backportbot-nextcloud]

The backport to stable24 failed. Please do this backport manually.

[solracsf]

/backport to stable25

[solracsf]

/backport to stable24

[backportbot-nextcloud]

The backport to stable24 failed. Please do this backport manually.

[SystemKeeper]

Not sure we need a backport to stable24, because there it is a link, instead of a form:

server/core/templates/loginflow/authpicker.php

Lines 49 to 51 in 7003a40

	<a href="<?php p($urlGenerator->linkToRoute('core.ClientFlowLogin.grantPage', ['stateToken' => $_['stateToken'], 'clientIdentifier' => $_['clientIdentifier'], 'oauthState' => $_['oauthState'], 'user' => $_['user'], 'direct' => $_['direct']])) ?>">
	<input type="submit" class="login primary icon-confirm-white" value="<?php p($l->t('Log in')) ?>">
	</a>

And there's no event handler:

https://github.com/nextcloud/server/blob/stable24/core/js/login/authpicker.js

But I'm not sure what the reason for the change was in the first place.

[kesselb]

I guess 25 is fine then.