Do not expire invitations on GET requests

[richardTowers]

At the moment, when the user visits:

/invitations/accept?code=some-code

the invitation code from their email is immediately expired and replaced
with a newly generated code which is put in a hidden input in the HTML
form. Each time the user submits the form, the code is expired and (if
necessary - e.g. if there's a validation issue) replaced with a new one.

This is fine so long as the user fills the form in immediately, but
there are a number of edge cases where this approach causes usability
problems:

1. If the user refreshes the page it will tell them their invitation has
   expired.
2. If the user closes the tab without submitting the form, and then
   follows the invitation link from their email later it will show as
   expired.
3. If the user's email client or web browser pre-fetches the link for
   any reason (e.g. virus scanning / spam detection / performance
   optimisation) then the link will not work when they follow it for
   real.

The third issue is the most serious.

We (GOV.UK PaaS) have had some users working in places that
pre-fetch links in emails (for some reason or other), and this means
they're completely unable to accept invitations. Judging from the irate
support tickets we've had from these users the experience is pretty
frustrating.

This commit changes the GET request to /invitations/accept so that it
does not expire the token (unless the invitation is being auto-accepted).

The POST handler is unchanged, so if the user actually submits the form
then the token will change (as it did before), even if there's a
validation issue that prevents the invitation being accepted.

This change fixes the usability issues, and makes the behaviour more
consistent with HTTP's semantics (in the sense that GET requests should
be "safe" - should not modify the state of the server).

I haven't tested this in a production-like setting yet, but I should have time to tomorrow.

This fixes #587 (which I don't think should be closed).

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/169322795

The labels on this github issue will be updated when the story is started.

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ Richard Towers (2f8600e)

[richardTowers]

I haven't tested this in a production-like setting yet, but I should have time to tomorrow.

Just to say I've tested this in a production-like environment now and it appears to work as intended:

• The first time the user visits the accept invitation form they're asked to choose and confirm their password
• If the user refreshes the page they do not see a message saying their invitation has expired
• If the user closes the page and follows the link a second time they're still asked to choose and confirm their password
• If the user submits the form their invitation code changes and they can no longer use the code from the email, regardless of whether the form submission was valid
• If the user submits an invalid form they're still able to correct their mistakes and proceed using the new invitation code which is generated for them

GOV.UK PaaS will be updating our UAA fork to run this code until it's merged into upstream.

[joshuatcasey]

Hi @richardTowers - we haven't gotten to this yet, but it's on our list. Hopefully we can review early next week!

[dbeneke]

Hello @richardTowers - The UAA team still plans to review this PR for merge. We're currently focused on some Spring Boot and k8s related changes that have consumed our available time/resource. We appreciate the submission, and haven't forgotten our need to review it.

[tlwr]

Hi, I've rebased this onto the develop branch

We've been running this in production since @richardTowers raised this PR, on our fork, and it is working perfectly. Our users no longer report that their invitations have expired due to email client shenanigans, we haven't had a support ticket about this in months.

[thausler786]

Moving our conversation from slack. We plan to create a story for us to look at this--we can't right now because of some competing priorities.

One security qualm that came up in our discussion--without spending a lot of time to validate--is that the PR expands the amount of time that the invite link is valid. If y’all have already thought through these scenarios we’d appreciate some input on their security implications..

We also discussed solving the problem as described in this StackExchange post, where the email has a GET link to a page where you confirm acceptance with a POST button, which gets around the lack of expiration. This seemed like a good alternative solution to us.

Thanks for your engagement on the issue and we hope to get back to you soon.

[richardTowers]

Hi @thausler786!

I think the solution you describe ("the email has a GET link to a page where you confirm acceptance with a POST button") is pretty much how things work at the moment, and is how things would work following this PR. The user already has to fill in a form and click a button to accept the invitation.

The only problem at the moment is that the initial GET request rotates the invitation code (so the old one is expired, and a new one is placed in the form). If this GET request is done by a non-human user agent (like an email client pre-fetching the link) then the whole system doesn't work.

From a security perspective, this only extends the time that the invitation code is valid in the situation where a user or process follows the link, but doesn't complete the form. Otherwise, the invitation codes still retain their standard expiry. I can't think of a scenario where that would give a malicious user an advantage (but perhaps someone with a more sneaky mind could think of something).

[AP-Hunt]

A brief look at the code change suggests to me that the PR has no impact on the maximum lifetime of an invite (disclaimer: I wasn't involved in writing it, and I'm not very familiar with the code base). The invite codes still have an expiry time, the only difference is that the invitation code isn't automatically expired in the situations described above.

We've been running this code in two production Cloud Foundry deployments since the PR was raised (23rd October 2019) and we haven't felt there was a concern around invite codes in that time.

I think when it comes down to it, we're not really fussed by which approach is taken to solve the problem we set out to solve. I think what we're most interested in is making sure invitation links don't expire at the wrong time, and that we're eventually able to swap back to running from the upstream releases.

[richardTowers]

I've rebased this against develop so it's a bit easier to review.

[thausler786]

Thanks! We've prioritized a story for us to look at this here

[LeePorte]

@thausler786 Any news on getting this reviewed and merged? This is causing delays in our ability to respond to CVEs as we are having to maintain our own fork.

[AP-Hunt]

Hi @thausler786 I can't access the story you linked. What's the state of play? Is there a timeline to this making it in to a release?

[adarshasp87]

From the end-users usability point of view, the invite code should not expire after the first hit from the browser. It can happen that the user closes the browser or tab without submitting the password.

Request you to prioritize this feature and merge the PR ASAP

[strehle]

LGTM

[strehle]

@peterhaochen47 OK with you to accept it, since I see no technical reason