Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: magic-link; password-free login #1150

Merged
merged 14 commits into from
Jul 25, 2021

Conversation

larsivi
Copy link
Contributor

@larsivi larsivi commented Jun 14, 2021

This is an addon service to password and adds the ability to authenticate via a token. The token can be sent to you via email using the requestLoginToken endpoint, or by other means server side by generating a token and adding it using addLoginToken. Users can now be found with this token, using findUserByLoginToken.
Default expiry is 15 minutes.

Three new packages are added, magic-link and mongo-magic-link and client-magic-link.

There is a logical dependency on package that handles users (typically password which in any case is a good companion module), but it is not hard if you have a different one providing the same.

Example of use (serverside, accompanied by password):

// There is nothing specific in Database setup, except the need for the mongo-magic-link package

const accountsPassword = new AccountsPassword({});
const accountsMagicLink = new AccountsMagicLink({});
const accountsServer = new AccountsServer(
       {
            db: accountsDb,
            tokenSecret: "terrible secret"
        },
        {
            password: accountsPassword,
            magicLink: accountsMagicLink,
        }
);

Example of use clientside (using GraphQL):

const apolloClient = new ApolloClient({ ...apolloOptions });
const accountsGraphQL = new GraphQLClient({graphQLClient: apolloClient});
accountsClient = new AccountsClient({}, accountsGraphQL);
const accountsMagicLink = new AccountsClientMagicLink(accountsClient);

 // Requesting email
await accountsMagicLink.requestMagicLinkEmail(email);

// Then, in the login handler
const loginResult = await accountsMagicLink.login({ token });

@changeset-bot
Copy link

changeset-bot bot commented Jun 14, 2021

🦋 Changeset detected

Latest commit: 62997f7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 22 packages
Name Type
@accounts/client Minor
@accounts/client-magic-link Minor
@accounts/database-manager Minor
@accounts/mongo Minor
@accounts/mongo-magic-link Minor
@accounts/typeorm Minor
@accounts/graphql-api Major
@accounts/graphql-client Minor
@accounts/magic-link Major
@accounts/rest-client Major
@accounts/rest-express Minor
@accounts/server Minor
@accounts/types Minor
@accounts/client-password Patch
@accounts/boost Patch
@accounts/express-session Patch
@accounts/oauth Patch
@accounts/mongo-password Patch
@accounts/mongo-sessions Patch
@accounts/redis Patch
@accounts/database-tests Patch
@accounts/two-factor Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@larsivi larsivi changed the title token: password-free login feat: token; password-free login Jun 18, 2021
Copy link
Member

@pradel pradel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for the pr, this will be a great addition that a lot of people requested!

As a general note, what do you think about renaming @accounts/token to @accounts/magic-link? The name is pretty standard for these kinds of authentication services, so it will be clear what this package is doing.

Regarding the logic, the implementation sounds good, I didn't do a deep review for this one but left a few small comments for now.
I was not able to compile the packages locally, graphql-client is throwing an error.

Security questions:

  • when requesting 2 emails, should both be active or just the last one sent? So when I ask for a new email it invalidates all the previous tokens, only the last email link is working.
  • after a successful login all the tokens should be invalidated

packages/client-token/README.md Outdated Show resolved Hide resolved
packages/client-token/src/client-token.ts Outdated Show resolved Hide resolved
website/docs/strategies/password.md Outdated Show resolved Hide resolved
packages/token/src/accounts-token.ts Outdated Show resolved Hide resolved
@pradel
Copy link
Member

pradel commented Jun 18, 2021

More comments: documentation and examples are still missing for this, is this something you would be up to do?

@larsivi
Copy link
Contributor Author

larsivi commented Jun 18, 2021

More comments: documentation and examples are still missing for this, is this something you would be up to do?

Yes, but if possible in a later PR.

@larsivi
Copy link
Contributor Author

larsivi commented Jun 18, 2021

As a general note, what do you think about renaming @accounts/token to @accounts/magic-link? The name is pretty standard for these kinds of authentication services, so it will be clear what this package is doing.

Sounds good!

Security questions:

  • when requesting 2 emails, should both be active or just the last one sent? So when I ask for a new email it invalidates all the previous tokens, only the last email link is working.
  • after a successful login all the tokens should be invalidated

Both of these makes sense, but maybe they could/should be covered by options? Or tied to the reason, such that login tokens explicitly added through addLoginToken could be managed by other means.

@codecov
Copy link

codecov bot commented Jun 18, 2021

Codecov Report

Merging #1150 (d72e62a) into master (14ce077) will decrease coverage by 0.10%.
The diff coverage is 94.27%.

❗ Current head d72e62a differs from pull request most recent head 62997f7. Consider uploading reports for the commit 62997f7 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1150      +/-   ##
==========================================
- Coverage   95.49%   95.39%   -0.11%     
==========================================
  Files          93      106      +13     
  Lines        2152     2344     +192     
  Branches      402      458      +56     
==========================================
+ Hits         2055     2236     +181     
- Misses         95      105      +10     
- Partials        2        3       +1     
Impacted Files Coverage Δ
...s/graphql-api/src/modules/accounts/schema/types.ts 100.00% <ø> (ø)
packages/server/src/utils/email.ts 70.00% <0.00%> (-12.36%) ⬇️
packages/graphql-api/src/modules/accounts/index.ts 76.92% <66.66%> (-1.34%) ⬇️
packages/magic-link/src/utils/validation.ts 75.00% <75.00%> (ø)
...aphql-api/src/modules/accounts-magic-link/index.ts 78.57% <78.57%> (ø)
packages/magic-link/src/accounts-magic-link.ts 93.02% <93.02%> (ø)
...ackages/client-magic-link/src/client-magic-link.ts 100.00% <100.00%> (ø)
packages/database-manager/src/database-manager.ts 100.00% <100.00%> (ø)
packages/database-mongo-magic-link/src/index.ts 100.00% <100.00%> (ø)
.../database-mongo-magic-link/src/mongo-magic-link.ts 100.00% <100.00%> (ø)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b24d75c...62997f7. Read the comment docs.

Fix issues from PR
@larsivi larsivi changed the title feat: token; password-free login feat: magic-link; password-free login Jun 20, 2021
Copy link
Member

@pradel pradel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • can you provide an example of the user creation? My understanding is that a password would be required in the current state? I might be missing something obvious tho :D

  • I guess you deleted the yarn.lock file and it was regenerated from scratch, this is creating some issues, could you please revert the change you made and update the one from master instead of recreating it?

packages/client-magic-link/__tests__/client-magic-link.ts Outdated Show resolved Hide resolved
packages/client-magic-link/package.json Outdated Show resolved Hide resolved
packages/client-magic-link/src/client-magic-link.ts Outdated Show resolved Hide resolved
packages/database-manager/__tests__/database-manager.ts Outdated Show resolved Hide resolved
packages/database-mongo-magic-link/src/mongo-magic-link.ts Outdated Show resolved Hide resolved
website/docs/transports/graphql.md Outdated Show resolved Hide resolved
larsivi and others added 6 commits June 22, 2021 21:47
@pradel
Copy link
Member

pradel commented Jul 3, 2021

@larsivi

// There is nothing specific in Database setup, except the need for the mongo-magic-link package

const accountsPassword = new AccountsPassword({});
const accountsMagicLink = new AccountsMagicLink({});
const accountsServer = new AccountsServer(
       {
            db: accountsDb,
            tokenSecret: "terrible secret"
        },
        {
            password: accountsPassword,
            magicLink: accountsMagicLink,
        }
);

regarding this, it's a bit weird that you have to add the password package too, this means that all the mutations (or routes via REST) will be exposed and user creation via password will be allowed.
Would it be possible to just install the magic link one?

@larsivi
Copy link
Contributor Author

larsivi commented Jul 3, 2021

regarding this, it's a bit weird that you have to add the password package too, this means that all the mutations (or routes via REST) will be exposed and user creation via password will be allowed.
Would it be possible to just install the magic link one?

Well, with the last cleanup I think that should work. The example was mostly like that since I do use the password module for user creation (and also for alternative login method). I haven't tried without though.

@pradel
Copy link
Member

pradel commented Jul 3, 2021

Okay, would be great to have the example so we are sure it's working that way. Will make it easier for users to get started with magic-link!

@acomito
Copy link

acomito commented Jul 7, 2021

you guys are gods

@pradel pradel merged commit 2205664 into accounts-js:master Jul 25, 2021
@pradel
Copy link
Member

pradel commented Jul 25, 2021

Amazing work @larsivi thank you so much for this!

@pradel
Copy link
Member

pradel commented Jul 25, 2021

Everything is published on npm, would be cool to get some documentation for other users!

@larsivi
Copy link
Contributor Author

larsivi commented Jul 25, 2021

Thank you! I'll look at writing some docs over the next few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants