Add account validation capability to the accounts plugin #1982

magopian · 2019-01-18T14:57:47Z

Fixes #1973

Try it here: https://kinto.github.io/kinto-account-demo/

This is a work in progress, may I have some feedback on the current work before pushing forward with the reset password functionality and the documentation?

Thanks!

tests/plugins/test_accounts.py

kinto/plugins/accounts/views.py

almet

Hi, thanks for the feature, it's great!

Here's some feedback:

Why not adding a flag to note if the account is validated or not, rather than relying on some other fields? I tend to think that explicit is better than implicit.
At first, I had trouble to understand what this activation-form-url was made for, but from what I understand, it's purpose is to add a link in the validation email that is sent to the user.

I have an alternative proposal for this one: what about this flow:

WebApp creates a new user. It specifies a validation_redirect_url
Kinto stores all the data and sens an email
The user clicks on the link in the email
Browser issues a GET request to the Kinto server with the activation key
Kinto server validates the account
Kinto server issues a redirect response to the redirection_url

kinto/plugins/accounts/views.py

magopian · 2019-01-21T14:47:35Z

At first, I had trouble to understand what this activation-form-url was made for, but from what I understand, it's purpose is to add a link in the validation email that is sent to the user.

@almet the idea (discussed in #1973) is that modifying the user record should not be done with a GET, but with a POST (or PATCH). So the activation-form-url is the link to the WebApp, which should display a proper form asking the user to confirm that they want to enable/activate/validate the account (probably a simple "call to action" button that will POST to the validate endpoint).

The WebApp should do this POST asynchronously, in the background (everything kinto related should be done this way in a WebApp I believe), so I'm not sure we need a redirection url.

I'm not arguing against the GET and redirection url though ;)

almet · 2019-01-21T17:28:23Z

At first, I had trouble to understand what this activation-form-url was made for, but from what I understand, it's purpose is to add a link in the validation email that is sent to the user.

@almet the idea (discussed in #1973) is that modifying the user record should not be done with a GET, but with a POST (or PATCH). So the activation-form-url is the link to the WebApp, which should display a proper form asking the user to confirm that they want to enable/activate/validate the account (probably a simple "call to action" button that will POST to the validate endpoint).

Why displaying a form if the user clicks on the link already? The Webapp could do the POST directly, saving an extra click, and staying RESTful. If we decide to go this way, we could have a default webapp shipped direclty with kinto-accounts, which will do the POST on behalf of the user, and letting the ability to the developer to specify a different webapp to use.

magopian · 2019-01-22T08:45:16Z

Why displaying a form if the user clicks on the link already? The Webapp could do the POST directly, saving an extra click, and staying RESTful. If we decide to go this way, we could have a default webapp shipped direclty with kinto-accounts, which will do the POST on behalf of the user, and letting the ability to the developer to specify a different webapp to use.

If we're comfortable with modifying a record without a user explicit action (auto-POSTing a form), I'd be ok with going all in and changing the validate endpoint to accept a simple GET.

If we do that, then there's no more need for an activation-form-url.

In any case, the redirect-url can already be set by modifying the link in the email_body_template setting. We could also accept it as a GET or POST parameter on the validate endpoint.

Maybe we should discuss this in another issue/PR?

magopian · 2019-01-22T10:52:30Z

Ok, the current work on the user creation and validation should be done now, and properly documented.

What's left to discuss before this can be merged:
1/ do we want to get rid of the activation-form-url in favor of validating a user using a simple GET? (see #1982 (comment))
2/ does it make sense to merge this PR as is, without the "reset password" functionality, which could be discussed in another issue, and implemented in another PR?

magopian · 2019-01-22T19:53:56Z

After an out-of-band discussion with @Natim we came to the conclusion that this PR could be slightly modified:

1/ don't require an activation-form-url field
2/ allow the user creator to provide some email context when POSTing on the /accounts/ endpoint (that will be used in addition to the user record and the activation-key to render the email template from the settings)
3/ to validate the user, still require a POST on the /accounts/<user id>/validate/<activation key> endpoint, through whatever means the user creator deems fit (it could be through a form, at a URL that they have provided in the email context in 2/). Here also allow the user creator to provide some email context (to render the "user successfully activated" email in 4/)
4/ when the user is validated, send another mail to confirm its activation

almet · 2019-01-22T21:21:44Z

Not sure what you mean with « email context ». You mean a template?

Also, In addition to what you've described, I think that if we go with a POST to validate the account, then we should provide a simple WebApp that does the POSTing automatically, in order to have a easy-to-setup account registration mechanism (e.g. as an admin deploying the stack, I want this to be simple enough)

almet · 2019-01-22T21:23:01Z

As for delaying reset password to another PR, I think it's un-necessary, unless you're in a hurry having this shipped (in which case, do as you prefer!)

Natim · 2019-01-23T07:27:04Z

Not sure what you mean with « email context ». You mean a template?

No the context to render the template aka:

{ "first_name": "Alexis", "email_subject": "blah"}

Natim · 2019-01-23T07:28:15Z

we should provide a simple WebApp that does the POSTing automatically, in order to have a easy-to-setup account registration mechanism

We could as an example but it is quite orthogonal to the service code.
I can't see someone using this feature not to wish to integrate it with their software UI.

almet · 2019-01-23T08:52:56Z

As for delaying reset password to another PR, I think it's un-necessary, unless you're in a hurry having this shipped (in which case, do as you prefer!)

we should provide a simple WebApp that does the POSTing automatically, in order to have a easy-to-setup account registration mechanism

We could as an example but it is quite orthogonal to the service code.
I can't see someone using this feature not to wish to integrate it with their software UI.

Some softwares have UI that are not browsers (e.g. a CLI script), which is why I think this is necessary to have by default.

Natim · 2019-01-23T08:55:03Z

Some softwares have UI that are not browsers

In that case they might not use links to validate the account.

almet · 2019-01-23T15:13:54Z

Some softwares have UI that are not browsers

In that case they might not use links to validate the account.

I think I missed something then. To me, if I register an account with the command line, the workflow is as follows:

A request is made from the CLI to the kinto server to create the account
An email is sent to the user, to confirm the email belongs to her / him
3. The user clicks on the link to validate
Account is validated.

Unfortunately, users use links in the emails to validate the accounts, and this happens through HTTP GET requests only.

What other scenario do you have in mind to avoid this webapp?

Natim · 2019-01-23T15:16:59Z

A request is made from the CLI to the kinto server to create the account

An email is sent to the user, to confirm the email belongs to her / him

The user clicks on the link to validate

Account is validated.

This is the case when working with a webapp, but if you are using a software you might want to use the following:

A request is made from the CLI to the kinto server to create the account
An email is sent to the user, to confirm the email belongs to her / him
The user copy the activation key and use the CLI to validate the account.
Account is validated.

almet · 2019-01-23T15:20:00Z

Ah yes! I haven't thought about this solution. I remember that's what Twitter used to do in the past :-)

That being said, I tend to prefer the « user clicks on the link » solution, because it's simpler. What's the value of having the user copy a verification code over clicking a link in an email, apart not having to implement an UI?

Natim · 2019-01-23T15:21:12Z

I think it is up to the developper to define the flow, I am not against building a little Elm demo webapp if it pleases you. We can even pair on it once this lands.

almet · 2019-01-23T16:15:12Z

I think it is up to the developper to define the flow, I am not against building a little Elm demo webapp if it pleases you. We can even pair on it once this lands.

I agree, we need to let the possibility for the developer to tweak the flow, if they want it to be different. One major selling point of kinto is « don't reinvent the wheel each time », and as such, I believe we should provide the golden path for everything. Something that makes it easy for everyone to use accounts, without having to implement something special.

Natim · 2019-01-23T16:18:15Z

I agree with you in the scope of Kinto which is an API. As soon as you have front-end facing experience, it is not the place of Kinto to solve it. However we could decide that the kinto-admin has got a built-in page to handle that.

almet · 2019-01-23T16:23:10Z

I agree, but I believe that if we start solving one problem (here having accounts validation), then we need to do it completely, and painlessly for the devs that want to use it.

As a side note, I think we're kinda going in circles with this discussion. I don't really mind disagreeing, but if anyone wants to weight in in order to move forward on this topic that would be beneficial :-) cc @leplatrem @glasserc

glasserc · 2019-01-23T17:08:20Z

I'm not sure you really want to hear my opinion, so I've been trying to stay out of it, but since you asked: I don't think Kinto is a good foundation for the kinds of thing you want to do. I don't like the current Kinto accounts plugin and I would prefer Kinto core not be cluttered with support for all the different possible things some developer might want an accounts system to do.

I think the PostgREST approach here is pretty solid: don't do any user stuff whatsoever -- delegate that to some other service -- but allow people to present JWTs to prove their identities. I recently saw https://github.com/netlify/gotrue which seems like a pretty nice product, but if you wanted to build something open source I would certainly support that too.

Natim · 2019-01-24T07:55:14Z

I would prefer Kinto core not be cluttered with support for all the different possible things some developer might want an accounts system to do.

I agree with you and I guess it is why we decided to put the account code in a plugin.

As of why plugins ships with Kinto itself, this is another story.

magopian · 2019-01-24T10:32:14Z

Wow, such long thread.

Here's a plan:
1/ I'll continue working on this PR to implement the modifications suggested in #1982 (comment)
2/ I'll implement the reset password feature

Once this is done, we can decide if it's good enough to land.

Regarding the "sample webapp that kinto should provide": let's discuss this in another issue, I'd like to not delay the landing of this functionality for too long. We can still improve in the future if we want (eg: also allowing a GET to validate an endpoint, providing redirect URL, providing a sample webapp, ...).

magopian · 2019-02-11T17:35:50Z

Ok folks, I hope I have addressed all your comments and reviews, but one thing: the heartbeat request. I've seen @natim PR and would love that they merge it. In any case, I'd love this PR to land as is, and maybe in the future add the heartbeat.

There's also this comment I'm not sure how to deal with it.

What do you think @Natim @leplatrem @almet? Anything else I'm missing?

Natim · 2019-02-12T07:44:38Z

kinto/plugins/accounts/__init__.py

+        # according to
+        # https://docs.pylonsproject.org/projects/pyramid_mailer/en/latest/#debugging
+        mailer = settings.get("mail.mailer", "")
+        config.include("pyramid_mailer" + ("." + mailer if mailer else ""))


Suggested change

config.include("pyramid_mailer" + ("." + mailer if mailer else ""))

config.include("pyramid_mailer" + (f".{mailer}" if mailer else ""))

Natim

Thank you for keeping at it! Great work 💃

I agree about the heartbeat. Nonetheless if you try to create an account you will see the issue quite quickly.

magopian · 2019-02-12T10:54:10Z

Quick note: it seems the + character (which can be present in an email address) is not allowed in the ID. Maybe we should fix that?

$ echo '{"data": {"id": "bob+test@example.com", "password": "321"}}' | http POST http://localhost:8888/v1/accounts
HTTP/1.1 400 Bad Request
Access-Control-Expose-Headers: Alert, Backoff, Content-Length, Retry-After
Content-Length: 167
Content-Type: application/json
Date: Tue, 12 Feb 2019 11:03:13 GMT
Server: waitress
X-Content-Type-Options: nosniff

{
    "code": 400,
    "details": [
        {
            "description": "Invalid object id",
            "location": "path",
            "name": null
        }
    ],
    "errno": 107,
    "error": "Invalid parameters",
    "message": "path: Invalid object id"
}

Natim · 2019-02-12T10:58:17Z

Quick note: it seems the + character (which can be present in an email address) is not allowed in the ID. Maybe we should fix that?

Yes we should probably fix it. Feel free to add a test for it if you want.

Natim · 2019-02-12T10:59:33Z

docs/configuration/settings.rst

+.. code-block:: ini
+
+    # Set the regular expression used to validate a proper email address.
+    kinto.account_validation.email_regexp = "^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+$"


Here you accept + character

that's an "additional restriction", it doesn't take over/replace what's declared in https://github.com/Kinto/kinto/blob/master/kinto/plugins/accounts/views.py#L33 (if that's the root cause of this issue, not sure yet, I'm investigating)

Maybe shall we use the same regex for both?

I don't think it's the same use case: in one case (no account validation) you want to allow a few set of characters, in the other case you might want to restrict emails to a given sub-domain.

I pushed a fix

I don't think it's the same use case: in one case (no account validation) you want to allow a few set of characters, in the other case you might want to restrict emails to a given sub-domain.

Yeah makes sense 👍

Natim

Let's merge this. @leplatrem final r+ ?

leplatrem

Almost r+ :)

I re-read everything :) This PR is huge but the feature is a valuable addition! Thanks a lot for your patience on this!

More seriously, my main concern is the organization of docs between settings and API. We should mention the context in the API docs, and leave settings as concise as possible.

And also some repeated code about cache manipulation that could be gathered in utils.

docs/api/1.x/accounts.rst

docs/api/index.rst

docs/configuration/settings.rst

kinto/plugins/accounts/views/__init__.py

kinto/plugins/accounts/views/validation.py

tests/test_config.py

tests/plugins/test_accounts.py

magopian · 2019-03-19T16:02:57Z

I believe I've addressed all your comments @leplatrem please let me know if I missed anything (in particular, are you happy with the way the docs are organized now?)

leplatrem

A lot better thanks!

🎉 🎉

Congrats for your patience on this!

n1k0 reviewed Jan 18, 2019

View reviewed changes

tests/plugins/test_accounts.py Outdated Show resolved Hide resolved

magopian force-pushed the 1973-account-validation branch from a8216cf to 0ff6728 Compare January 18, 2019 15:03

almet reviewed Jan 18, 2019

View reviewed changes

kinto/plugins/accounts/views.py Outdated Show resolved Hide resolved

almet reviewed Jan 18, 2019

View reviewed changes

kinto/plugins/accounts/views.py Outdated Show resolved Hide resolved

almet reviewed Jan 18, 2019

View reviewed changes

magopian commented Jan 19, 2019

View reviewed changes

kinto/plugins/accounts/views.py Outdated Show resolved Hide resolved

magopian force-pushed the 1973-account-validation branch from ba6102a to 9dd642e Compare January 21, 2019 14:38

magopian force-pushed the 1973-account-validation branch 2 times, most recently from 8268dc3 to 9313147 Compare January 22, 2019 14:47

magopian force-pushed the 1973-account-validation branch from 9181735 to 9f16d46 Compare February 8, 2019 14:42

magopian added 7 commits February 8, 2019 16:03

re-add the setup.py which was removed by mistake

d27de8c

Remove unused import

bc0af23

Some more code reorganization following review

cb8b480

Restrict the reset password to changing an account's password

ad96885

Move the code to send activation email to a listener

1940f8b

Move the code to send activation confirmation email to a listener

caec88d

Move all the mail sending related code to a separate module

80a7cae

magopian force-pushed the 1973-account-validation branch from c68665a to 80a7cae Compare February 11, 2019 16:36

Natim reviewed Feb 12, 2019

View reviewed changes

Natim approved these changes Feb 12, 2019

View reviewed changes

@Natim review

fd4fff3

Natim reviewed Feb 12, 2019

View reviewed changes

magopian and others added 4 commits February 12, 2019 12:09

Allow the '+' character in the account IDs

17911d1

Investigate py36-raw error.

768acb7

Remove a repetition in the docs

49c9e43

Use a patch when changing the password or the user is 'reset'

9309fb5

magopian force-pushed the 1973-account-validation branch from 13052e5 to 9309fb5 Compare February 21, 2019 18:20

Natim approved these changes Mar 8, 2019

View reviewed changes

leplatrem reviewed Mar 8, 2019

View reviewed changes

magopian force-pushed the 1973-account-validation branch from f28b95e to b3b6cc1 Compare March 19, 2019 15:28

Some more reviews from @leplatrem

bfde10d

magopian force-pushed the 1973-account-validation branch from b3b6cc1 to bfde10d Compare March 19, 2019 15:54

leplatrem approved these changes Mar 20, 2019

View reviewed changes

magopian merged commit 24f5d83 into Kinto:master Mar 20, 2019

glasserc mentioned this pull request May 17, 2019

pyramid-mailer dependency is missing from requirements #2149

Merged

	config.include("pyramid_mailer" + ("." + mailer if mailer else ""))
	config.include("pyramid_mailer" + (f".{mailer}" if mailer else ""))

Add account validation capability to the accounts plugin #1982

Add account validation capability to the accounts plugin #1982

Conversation

magopian commented Jan 18, 2019 • edited

almet left a comment

Choose a reason for hiding this comment

magopian commented Jan 21, 2019

almet commented Jan 21, 2019

magopian commented Jan 22, 2019

magopian commented Jan 22, 2019

magopian commented Jan 22, 2019

almet commented Jan 22, 2019

almet commented Jan 22, 2019

Natim commented Jan 23, 2019

Natim commented Jan 23, 2019

almet commented Jan 23, 2019

Natim commented Jan 23, 2019

almet commented Jan 23, 2019

Natim commented Jan 23, 2019

almet commented Jan 23, 2019

Natim commented Jan 23, 2019

almet commented Jan 23, 2019

Natim commented Jan 23, 2019 • edited

almet commented Jan 23, 2019

glasserc commented Jan 23, 2019

Natim commented Jan 24, 2019

magopian commented Jan 24, 2019

magopian commented Feb 11, 2019 • edited

Natim Feb 12, 2019

Choose a reason for hiding this comment

Natim left a comment • edited

Choose a reason for hiding this comment

magopian commented Feb 12, 2019 • edited

Natim commented Feb 12, 2019 • edited

Natim Feb 12, 2019

Choose a reason for hiding this comment

magopian Feb 12, 2019

Choose a reason for hiding this comment

Natim Feb 12, 2019

Choose a reason for hiding this comment

magopian Feb 12, 2019

Choose a reason for hiding this comment

magopian Feb 12, 2019

Choose a reason for hiding this comment

Natim Feb 12, 2019

Choose a reason for hiding this comment

Natim left a comment

Choose a reason for hiding this comment

leplatrem left a comment

Choose a reason for hiding this comment

magopian commented Mar 19, 2019

leplatrem left a comment • edited

Choose a reason for hiding this comment

magopian commented Jan 18, 2019 •

edited

Natim commented Jan 23, 2019 •

edited

magopian commented Feb 11, 2019 •

edited

Natim left a comment •

edited

magopian commented Feb 12, 2019 •

edited

Natim commented Feb 12, 2019 •

edited

leplatrem left a comment •

edited