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

Move accounts plugin to Node app and better split accounts from users/IDP #5693

Merged
merged 15 commits into from
Oct 8, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Oct 7, 2019

Resolves #5588
Resolves #5213
Impact: minor
Type: refactor

Changes

  • Move the "account" plugin to the Node app core-services folder
  • Move createUser into the account plugin. It's still relying on Meteor, but that's OK until we split the apps.
  • Reaction Admin no longer creates automatic "anonymous" users in the system, and a migration deletes all anonymous users and accounts. These have not been used since 2.0.0, but the code creating them was never removed from Reaction Admin.
  • Move all auth logic and middleware into the account plugin (see below)

New Middleware Pattern

Plugins can register Express middleware using registerPlugin:

    expressMiddleware: [
      {
        route: "graphql",
        stage: "authenticate",
        fn: tokenMiddleware
      }
    ],

For now, only "graphql" route is supported, and the following stages are supported in this order:

  • first
  • before-authenticate
  • authenticate
  • before-response

An authenticate middleware function should do something like look up the user by the Authorization header, and either set request.user or send a 401 response if the token is invalid. It should not require a token. This is what the built-in account service now does.

The first middleware stage can be used for loggers or anything else that needs to be first in the middleware list. before-response middleware will have the user available if there is one, and is called before the Apollo GraphQL middleware.

A middleware function is passed context and must return the Express middleware handler function, which must call next() or send a response.

Auth functions

Plugins can also define auth-related functions when calling registerPlugin. The built-in accounts service does this.

    auth: {
      accountByUserId,
      getHasPermissionFunctionForUser,
      getShopsUserHasPermissionForFunctionForUser
    },

These are all functions that are called on every request, when adding request-specific properties to context.

  • accountByUserId(context, userId): Must return an account for the user ID, if one can be found
  • getHasPermissionFunctionForUser(context): Must return a function that matches the context.userHasPermission signature and returns true or false. This function can assume context.user/context.account is the user/account to check.
  • getShopsUserHasPermissionForFunctionForUser(context): Must return a function that matches the context.shopsUserHasPermissionFor signature and returns an array of shop IDs. This function can assume context.user/context.account is the user/account to check.

Testing

  • Verify Reaction Admin works, especially logging in / out.
  • Verify that authenticated GraphQL requests continue to work. Clicking around in Reaction Admin, such as product and order tables, is enough to verify this.

@aldeed aldeed self-assigned this Oct 7, 2019
@aldeed aldeed requested a review from kieckhafer October 8, 2019 16:39
@aldeed aldeed marked this pull request as ready for review October 8, 2019 16:40
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

Code looks mostly good, just a few import updates.

Testing functionality now.

Signed-off-by: Eric Dobbertin <[email protected]>

Co-Authored-By: Erik Kieckhafer <[email protected]>
@aldeed
Copy link
Contributor Author

aldeed commented Oct 8, 2019

Made suggested changes

Signed-off-by: Eric Dobbertin <[email protected]>
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

👍 Works as expected, good to merge once CI passes.

@kieckhafer kieckhafer merged commit 82209ff into develop Oct 8, 2019
@kieckhafer kieckhafer deleted the feat-aldeed-demeteor-accounts-plugin branch October 8, 2019 19:47
@kieckhafer kieckhafer restored the feat-aldeed-demeteor-accounts-plugin branch October 8, 2019 19:47
@willopez willopez mentioned this pull request Oct 9, 2019
@kieckhafer kieckhafer deleted the feat-aldeed-demeteor-accounts-plugin branch October 28, 2019 01:08
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.

2 participants