-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Comments
@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. |
On initial look it seems account-js doesn't work well with next-js: this issue and this |
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.
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. |
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) 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 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) Hope it’s clear what I mean and I didn’t just add more confusion :D |
@janus-reith Thanks for the write-up. I got what you are trying to say and it makes sense :) |
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 :) |
@pradel Thanks for the offer. I was able to integrate the lib and have open PRs which should be merged soon. |
@pradel There is one thing that came up which is blocking me from implementing this. accounts-js has this security vulnerability: I have opened an issue for the same here: accounts-js/accounts#1140 |
v4.0.0 is already using accounts-js. closing |
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:
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.
The text was updated successfully, but these errors were encountered: