Allow users to click account renewal links multiple times without hitting an 'Invalid Token' page #74

anoadragon453 · 2020-12-17T12:31:45Z

This PR makes some modifications to the account renewal endpoint to enable idempotency. For context, when the account validity feature is in use, users are sent an email with a renewal link a little while before their account expires.

Users may end up clicking this link twice accidentally, or their spam software will click it for them before they have a chance to do so manually. In each case, every click after the first would return an Invalid Token error page, as we delete renewal tokens once used.

This PR changes things up a bit. We no longer delete the token upon use. However, we also ensure that tokens cannot be re-used by (ab)using the email_sent boolean. This boolean is set to True when an email with the token is sent out, and set to False when the account is renewed using that token. With this, and with the token still intact, adding a new token_used_ts_ms column to the account_validity table. With this, we can check if the token is valid, yet has already been used. In this case, return a new template which simply informs the user of when their account will expire, but not that it was just renewed.

Part of this PR was also adding information about when a user's account would expire in the actual renewal template. Mostly for nicer UX purposes. Doing so meant converting the account validity config code to using the new read_templates function, which simplifies it massively!

Reviewable commit-by-commit.

Also some typing and docstring fixups.

We're doing this as we're planning to inject account expiry dates into the templates (so that they can show "Your account is valid until XX/YY/ZZ"). Since this is only information we'll have during server operation, we need to switch the config variables to be templates instead. Also nice as we no longer need to hardcoded the template content inline. The public_baseurl change was due to self.public_baseurl raising an AssertionError if we try to call it before it exists, which happens to be the case for the account validity config code. Note that we override __getattr__ above.

For when users accidentally click the renewal link twice.

We check if the user is re-using a token by (ab)using the 'email_sent' column associated with each user's account validity information. When a token is first created and emailed to a user, 'email_sent' is set to True. When a user comes back to us with this token, we renew the account and set 'email_sent' to False - but critically we keep the token around. If the user comes back again with the same token, we'll see that although the token is valid, 'email_sent' is False. From this we can learn that the token was already used to renew the account. We call this case a stale token, and return a new template to the user. The new template simply informs the user when their account will expire. This is much nicer UX than the 'invalid token!' users would get when they tried a token twice.

Note that the variable for this test include an account expiring after 1 week of it last being renewed (or created), and sending the user a renewal token 2 days before the account is set to expire. The change here is that after we successfully renew the account, move forward and try to renew it again. We should get a success, and the new template returned, but the account's expiration date should not have been extended.

clokep · 2020-12-21T12:59:18Z

In this case, return a new template which simply informs the user of when their account with expire, but not that it was just renewed.

Curious why this was done? Why not just return the same template always?

giomfo · 2020-12-21T18:27:03Z

@clokep: the plan is to be able to handle separately the 2 cases:

the request renews actually the user's account
the request do nothing (because the provided token has already been used)

clokep · 2020-12-21T18:29:15Z

Right, but it seems like a generic message could be used in both cases "Your account renewal succeeded and is valid until xxx yy, zzzz."

I haven't thought through all the scenarios, but I'm wondering if there's an attack by showing different information (similar to how you limit the information during password resets). I doubt there is, but wanted to mention it!

giomfo · 2020-12-21T19:13:06Z

I understand what you mean, the aim here is to keep the opportunity to customize the 2 cases.
The second case may happen at any time (whereas the account has been already renewed several days ago), we want to be sure the user understands correctly the message.

@anoadragon453 I would say if there is no technical problem to handle separately these 2 cases (And I think this is already done), I would prefer handle them separately. If you see any reason to merge these 2 cases, feel free to merge them

clokep

The code seems like it would work ok.

We no longer delete the token upon use. However, we also ensure that tokens cannot be re-used by (ab)using the email_sent boolean. This boolean is set to True when an email with the token is sent out, and set to False when the account is renewed using that token.

I'm not a big fan of this approach -- multiple pieces of state ending up in a boolean isn't great. Can we add an additional boolean column of "token_used" or something? (Or maybe a timestamp of when it was first clicked.)

synapse/res/templates/account_renewed.html

synapse/storage/databases/main/registration.py

synapse/handlers/account_validity.py

tests/rest/client/v2_alpha/test_register.py

synapse/config/_base.py

Calling `self.read_templates` from AccountValidityConfig does not work as expected as it does not have state such as self.public_baseurl properly set up. This is due to AccountValidityConfig not being initialised like other config classes, but instead simply in the body of RegistrationConfig.read_config. I think there's an argument for initialising it properly, instead of as a part of registration config. Perhaps even putting it in its own file. But that should be left to a separate PR.

Co-authored-by: Patrick Cloke <[email protected]>

…ail_sent

anoadragon453 · 2020-12-29T18:11:12Z

Can we add an additional boolean column of "token_used" or something? (Or maybe a timestamp of when it was first clicked.)

The original reason for using email_sent was to minimise the necessary changes for this feature. But given the current size of this PR I don't really have any objections with adding a migration.

I've added a column token_used_ts_ms to account_validity, which we now use to check the stale status of a token.

…oa/account_renewal_idempotency

clokep

Overall this seems OK. I think I want to discuss the config bits a bit more though.

synapse/handlers/account_validity.py

synapse/storage/databases/main/registration.py

clokep · 2020-12-29T20:47:49Z

synapse/config/registration.py

+        # We do this here instead of in AccountValidityConfig as read_templates
+        # relies on state that hasn't been initialised in AccountValidityConfig


This is due to the AccountValidityConfig not being part of the huge config object that normally gets created? I wonder if there's a good reason for that.

Indeed, and my thought is that this class was just created to clean up RegistrationConfig a bit, rather than making AccountValidityConfig a proper config class.

But I'm not sure if there's any justification against not doing so. May have to wait for people to come back to ask.

We talked a bit about this and the conclusion was to attempt to make it a proper config class, but if it turns out to be too hard then to punt it.

We're not using this to calculate whether a user has used this token previously anymore.

synapse/storage/databases/main/registration.py

Since we're pulling config options out of the config during class __init__ functions now, instead of during runtime, we can't just change the homeserver config and expect runtime to have the new value. We need to change the vars that get set in those __init__ functions directly. In this case, RegistrationStore.__init__.

anoadragon453

Updated to make AccountValidityConfig its own actual config class.

Let me know if you want this to be in a separate PR instead 🙂

anoadragon453 · 2020-12-30T15:51:15Z

synapse/config/account_validity.py

+from synapse.config._base import Config, ConfigError
+
+
+class AccountValidityConfig(Config):


This diff doesn't make it obvious what changed in the move. Essentially it was just a cut n' paste, but setting up read_config (instead of __init__) and generate_config_section methods for AccountValidityConfig.

I also modified self.account_validity.xxx instances to self.account_validity_xxx. Stuff that pulled from config (which used to be synapse_config["account_validity"], now pulls from new var account_validity_config.

anoadragon453 · 2020-12-30T15:53:01Z

synapse/config/account_validity.py

+        # after that the validity period changes and Synapse is restarted, the users'
+        # expiration dates won't be updated unless their account is manually renewed. This
+        # date will be randomly selected within a range [now + period - d ; now + period],
+        # where d is equal to 10% of the validity period.


I needed to switch instances of %% to % in this config block. This is because we were running % locals() on this text before in RegistrationConfig, but we do not need to here. All of the placeholder variables were related to registration options.

The %(app)s bits are meant to be left in the config, and will get parsed by other code during runtime.

clokep

I think this looks fine.

clokep · 2020-12-30T16:04:32Z

tests/rest/client/v2_alpha/test_register.py

@@ -863,7 +863,7 @@ def make_homeserver(self, reactor, clock):
        config["account_validity"] = {"enabled": False}

        self.hs = self.setup_test_homeserver(config=config)
-        self.hs.config.account_validity_period = self.validity_period
+        self.hs.get_datastore()._account_validity_period = self.validity_period


Would a better way to do this be to define default_config?

hs.config.account_validity_period still won't be set even if account_validity.period if defined in the homeserver config though, right?

We talked a bit in #synapse-dev about this. The reason this test was working before is because we were reading period and startup_job_max_delta from the homeserver config file during test execution.

Now we're reading account validity config values during homeserver initialisation, which will happen during the self.setup_test_homeserver call. Before setting those homeserver config options.

I've added a comment to explain this, but this test probably still needs a separate look. (For instance, why does the docstring of test_background_job say it does the same thing as test_background_job??).

synapse/handlers/account_validity.py

Co-authored-by: Patrick Cloke <[email protected]>

Also localise a variable that didn't need to be class-level.

anoadragon453 · 2021-04-16T16:50:20Z

This PR was never ported to mainline unfortunately. After a bit of cleanup, a port has been put up: matrix-org/synapse#9832

…ut hitting an 'Invalid Token' page #74" from synapse-dinsic (#9832) This attempts to be a direct port of matrix-org/synapse-dinsic#74 to mainline. There was some fiddling required to deal with the changes that have been made to mainline since (mainly dealing with the split of `RegistrationWorkerStore` from `RegistrationStore`, and the changes made to `self.make_request` in test code).

anoadragon453 added 5 commits December 16, 2020 17:36

Don't clear a user's renewal token after they've validated their account

39dd511

Also some typing and docstring fixups.

Add template for trying to renew an account with a previously used token

57ffeb8

For when users accidentally click the renewal link twice.

anoadragon453 added the sponsored label Dec 17, 2020

anoadragon453 requested a review from a team December 17, 2020 12:31

clokep reviewed Dec 22, 2020

View reviewed changes

anoadragon453 and others added 7 commits December 29, 2020 17:18

Apply suggestions from code review

a9d4ca2

Co-authored-by: Patrick Cloke <[email protected]>

Change log line to be info, also log attempted token

6d5d4a4

Simplify Content-Type checking test code

d950e33

Add token_used_ts_ms column to account_validity table

f8addd1

Switch to a new token_used_ts_ms db column, instead of overloading em…

9e4d541

…ail_sent

mypy

b06d392

anoadragon453 requested a review from clokep December 29, 2020 18:11

Merge branch 'dinsic' of github.com:matrix-org/synapse-dinsic into an…

0f3299a

…oa/account_renewal_idempotency

clokep reviewed Dec 29, 2020

View reviewed changes

Remove email_sent from get_user_from_renewal_token return values

62b5c30

We're not using this to calculate whether a user has used this token previously anymore.

clokep reviewed Dec 30, 2020

View reviewed changes

synapse/storage/databases/main/registration.py Outdated Show resolved Hide resolved

Remove unnecessary email_sent column from query

749ea53

anoadragon453 requested a review from clokep December 30, 2020 13:38

anoadragon453 added 3 commits December 30, 2020 16:00

Move AccountValidityConfig to its own config file

5fc82f9

Change account validity config option instances

c239f78

anoadragon453 force-pushed the anoa/account_renewal_idempotency branch from 0ae4dcf to 150e7e9 Compare December 30, 2020 16:01

anoadragon453 commented Dec 30, 2020

View reviewed changes

clokep approved these changes Dec 30, 2020

View reviewed changes

anoadragon453 and others added 4 commits December 30, 2020 16:37

Update synapse/handlers/account_validity.py

b3a88b0

Co-authored-by: Patrick Cloke <[email protected]>

Allow account_validity to be None in the config

4e4b619

Define AV variables as None initially, instead of not setting them

ddc1a66

Also localise a variable that didn't need to be class-level.

Add some explanation to directly setting account_validity-related config

cb82e7f

anoadragon453 merged commit 63e8ab5 into dinsic Dec 30, 2020

anoadragon453 deleted the anoa/account_renewal_idempotency branch December 30, 2020 17:43

anoadragon453 mentioned this pull request Jan 18, 2021

Ensure we read account validity templates from custom template dir if provided #77

Merged

anoadragon453 mentioned this pull request Apr 16, 2021

Port "Allow users to click account renewal links multiple times without hitting an 'Invalid Token' page #74" from synapse-dinsic matrix-org/synapse#9832

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to click account renewal links multiple times without hitting an 'Invalid Token' page #74

Allow users to click account renewal links multiple times without hitting an 'Invalid Token' page #74

anoadragon453 commented Dec 17, 2020 •

edited

Loading

clokep commented Dec 21, 2020

giomfo commented Dec 21, 2020

clokep commented Dec 21, 2020

giomfo commented Dec 21, 2020

clokep left a comment

anoadragon453 commented Dec 29, 2020

clokep left a comment

clokep Dec 29, 2020

anoadragon453 Dec 30, 2020

clokep Dec 30, 2020

anoadragon453 left a comment

anoadragon453 Dec 30, 2020

anoadragon453 Dec 30, 2020

clokep left a comment

clokep Dec 30, 2020

anoadragon453 Dec 30, 2020

anoadragon453 Dec 30, 2020

anoadragon453 commented Apr 16, 2021

		# We do this here instead of in AccountValidityConfig as read_templates
		# relies on state that hasn't been initialised in AccountValidityConfig

		from synapse.config._base import Config, ConfigError


		class AccountValidityConfig(Config):

Allow users to click account renewal links multiple times without hitting an 'Invalid Token' page #74

Allow users to click account renewal links multiple times without hitting an 'Invalid Token' page #74

Conversation

anoadragon453 commented Dec 17, 2020 • edited Loading

clokep commented Dec 21, 2020

giomfo commented Dec 21, 2020

clokep commented Dec 21, 2020

giomfo commented Dec 21, 2020

clokep left a comment

Choose a reason for hiding this comment

anoadragon453 commented Dec 29, 2020

clokep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoadragon453 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anoadragon453 commented Apr 16, 2021

anoadragon453 commented Dec 17, 2020 •

edited

Loading