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

RFC: Replace Meteor IDP with Accounts-JS #6308

Closed
janus-reith opened this issue Nov 16, 2020 · 10 comments
Closed

RFC: Replace Meteor IDP with Accounts-JS #6308

janus-reith opened this issue Nov 16, 2020 · 10 comments
Assignees
Labels
discussion For issues in which discussion is expected but no further action is needed enhancement For issues that describe a feature that needs to be added, changed, or removed, but is not a bug

Comments

@janus-reith
Copy link
Collaborator

janus-reith commented Nov 16, 2020

I experimented with accounts-js, which provides authentication and accounts management similar to the builtin Meteor accounts.
The main advantage with this is, that I could maintain compatibility with existing stored Credentials in the users collection.
The server side can be plugged in via REST or GraphQL and works great with Apollo.
The client side library part of this also is really convenient, and e.g. does the jwt handling in localStorage, adds it as Authorization header on the Apollo Link, and so on.

My approach for now was to just add an API plugin for that, and use the Client-side parts of this right in the storefront and admin apps, removing the need for Hydra, the Meteor IDP, and the separate Postgres DB.
I believe that this approach could be way more suitable for the average user - As we only authenticate first partys using that Flow, I see no added value from a security perspective by using Hydra, especially none that would outweight the pain of setting RC up the first time without deeper knowledge of the stack, or how OAuth works.
Enterprises that actually are interested to integrate RC into their login solution most likely also won't use the IDP that RC brings, but plug in what they already have.

I would however like to support both approaches with this, and would like some input on how to wrap up things best.
So within the scope of this would be at least these two separate paths:

A. Add an accounts-js auth plugin to the api, provide guidance on how to add it to the Storefront and Admin, debate wether this should be some sort of default stack / wether we could support multiple "flavors" of RC usage.
The Architecture would be way more compact, you would have Admin, Storefont, API and MongoDB, and much less environment vars to set up.

B. Without additional changes to the base architecture of services, replace the Meteor IDP which, although bare and without many bundled plugins, still loads up really slow. We could use nextjs or CRA or some custom React App, I would prefer something that serves static html from the server and does not need to load up when arrived on the client, I really like nextjs but I'm open to other suggestions.
For this we would need to debate wether:

  • Option 1: This should just be a frontend app and the accounts-js related logic remains on the API
  • Option 2: We replicate the existing structure, meaning the IDP would spin up its own Apollo Server and have the Accounts JS logic in there, while accessing the user collection in the same mongo db which the current API uses.

I would currently favor Option 1 and have it in the API, and rather focus on making the api plugins depend less on each other so at some point they, including that new Accounts plugin, could just be used individually.
Everyone interested in wiring up their own auth solution could still just not import that plugin then with a single line of code.

@janus-reith janus-reith added core work For issues that track feature development work being done by core Reaction developers enhancement For issues that describe a feature that needs to be added, changed, or removed, but is not a bug discussion For issues in which discussion is expected but no further action is needed and removed core work For issues that track feature development work being done by core Reaction developers labels Nov 16, 2020
@Akarshit Akarshit self-assigned this Mar 23, 2021
@Akarshit
Copy link
Contributor

@janus-reith Hey! I am reasearching on this and from what I have found account-js seems to be a much simpler alternative to what we currently have. I will keep posting my notes here for reference.

@Akarshit
Copy link
Contributor

On initial look it seems account-js doesn't work well with next-js: this issue and this

@janus-reith
Copy link
Collaborator Author

janus-reith commented Mar 23, 2021

@janus-reith Hey! I am reasearching on this and from what I have found account-js seems to be a much simpler alternative to what we currently have. I will keep posting my notes here for reference.

Great! Yes it could be rather simple to integrate, some of the further graphql related auth functionality which that package provides might not even be relevant to us for now, but just for the basic signuo/login logic it could be a rather simple plug-in instead of Meteor Accounts.

We could replace the IDP with it rather quickly, or as discussed even go a step further and not rely on the Oauth flow with the IDP. at all, and just have Accounts-JS back right in the api and use that directly in the storefront and admin.

On initial look it seems account-js doesn't work well with next-js: this issue and this

I had success with a rough test implementation I did a few months ago which covers both storefront and api, so it should work with nextjs atleast somehow, I might need to take another look what I did.

@Akarshit
Copy link
Contributor

To implement this I was debating between adding a new auth-plugin or extending the current accounts plugin. If anyone has any views, I would love to hear those...

@janus-reith
Copy link
Collaborator Author

To implement this I was debating between adding a new auth-plugin or extending the current accounts plugin. If anyone has any views, I would love to hear those...

I would assume the Accounts plug-in to still be relevant, if we keep that split of internal generic Account Collection + Source X (currently meteor users collection, or could be some Oauth Flow)
So far that worked rather well for many of my usecases and also with multiple providers, as they would all be generalized into an Accounts document.(Although technically, it wouldn’t be required to store the account in the api at all for external users and just have them on the request context)

Rather the current authentication plugin that parsed the Hydra Auth Header could be deprecated but kept under some separate name api-plugin-authentication-hydra https://github.com/reactioncommerce/api-plugin-authentication
and a new one could be added, which also includes a middleware which parses an Auth Header or JWT (coming from accounts-js) just like the other plugin did, but additionally exposes the accounts-js graphql schemas and resolvers to add signup functionality right to the api.

In the other case where we would only add Accounts-JS to the IDP to not rely on Meteor and keep the existing Oauth Flow the same otherwise, even the current hydra-based api-plugin-authentication could be kept the same and the api wouldn’t require further changes. Although I would like to consider switching that to JWT anyways to not require that roundtrip to Hydra again, as the api currently calls Hydra again to inspect the token.

...which again led me to another consideration, which would be, what if we have some generic solution for determining if incoming requests are authenticated, that would support arbitrary JWTs and read from that, so that could be either coming from accounts-js, from hydra using the Oauth flow, or any other third party Oauth flow(used next-auth in the storefront often and generate the jwts there on the Server-parsed callback url, similar to our current custom api routes for the hydra flow)
The api would then just need a list of allowed JWT providers and matching public keys

Hope it’s clear what I mean and I didn’t just add more confusion :D

@Akarshit
Copy link
Contributor

@janus-reith Thanks for the write-up. I got what you are trying to say and it makes sense :)
Regarding the last para, the plan it to get JWT working for now and try to do it a way that people can connect their OAuth implementation, but the first goal is to just get this working.
Thanks for the comment, helps clear up the things a lot.

@pradel
Copy link

pradel commented May 6, 2021

Hey I just found out about this issue, I am the maintainer of accounts-js, let me know if you face any issue during the integration :)

@Akarshit
Copy link
Contributor

Akarshit commented May 6, 2021

@pradel Thanks for the offer. I was able to integrate the lib and have open PRs which should be merged soon.
Thanks for the awesome lib :)

@Akarshit
Copy link
Contributor

Akarshit commented May 11, 2021

@pradel There is one thing that came up which is blocking me from implementing this. accounts-js has this security vulnerability:
Screen Shot 2021-05-11 at 2 27 45 PM
Would it be possible to migrate to graphql-modules v1 to fix this?

I have opened an issue for the same here: accounts-js/accounts#1140

@delagroove
Copy link
Contributor

v4.0.0 is already using accounts-js. closing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion For issues in which discussion is expected but no further action is needed enhancement For issues that describe a feature that needs to be added, changed, or removed, but is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants