Compare and merge with brettle:accounts-add-service

[zimme]

Compare and keep the best solution and work together?

/cc @brettle

[splendido]

Lets consider also mondora:connect-with

[zimme]

Lets consider that connect with package in the context of #2

[brettle]

I spent a while this morning looking at your code, and here's my take on a comparison:

1. When a logged in user tries to log in using an OAuth service/id for which there isn't yet an app account, both packages add the service to the logged in user's account and result in the user continuing to be logged in to the same account. The one-account package treats this as a normal successful login. The brettle:accounts-add-service package cancels the login by throwing a LoginCancelled error.
2. When a (logged in or logged out) user logs in using an OAuth service where he has a verified email address for which there is already an app account, only one-account adds the service to the existing account and logs the user into that account. Per issue Force users to login to both services before merging #3, I view this as a security issue.
3. When a (logged in or logged out) user logs in using an OAuth service, only one-account adds the email address to the user's emails property, and (unless configured not to) sends a verification email if necessary. This seems primarily aimed at facilitating case 2 above, but it might be useful in its own right. If we drop support for case 2, perhaps this could go in a separate package?
4. When a logged in user tries to log in using a non-OAuth service (e.g. accounts-password) for which there isn't an yet an app account, only brettle:accounts-add-service adds the service to the logged in user's account and cancels the login attempt.

Another difference is that one-account monkey patches the core, but brettle:accounts-add-service does not (as of Meteor 1.2).

Apologies if I've missed anything...

Based on the above, my (obviously biased) recommendation would be to just use brettle:accounts-add-service.

I continue to be happy to move any/all of brettle:accounts-* packages under the useraccounts umbrella.

[zimme]

Because of 3, 4 isn't necessary in one-account. i.e. any service account becomes a password account because the email is added to the emails list. Just without a password set.

edit: because of this, any account can always use reset password.
edit2: and you always have an easy way to find other accounts which have the same email, without using that registerEmails array @splendido used in accounts-meld.

[zimme]

When this discussion is over, it would be nice if we have settled on some "sane" and secure defaults for our account merging solution.

[brettle]

I agree that 3 provides some support for accounts-password (I hadn't thought of that), but if the user wants to be able to login to his account with a username or a different email address, the client can't just call Accounts.createUser(). Moreover, accounts-password was just an example. There are other non-OAuth services and they might not necessarily use updateOrCreateUserFromExternalService().

[zimme]

I haven't thought about other non-oauth providers. That's a good point.

[zimme]

I know we have taken a bit of a different approaches to solving pretty much the same problem.

I know i "monkey patch" the core and wrapping functions to do my stuff. I did see you using a lot of work around packages which pretty much monkey patches the core too.

Maybe a "new" accounts system forked from the core accounts system is a better approach, this way we would have full control over what happens where and we don't have to monkey patch or work around anything?

[splendido]

What about sharing these concerns with MDG and try to see whether there's room for them to accept changed to the current Accounts system?

[zimme]

Ofc, thats the preferred option. Im just throwing Ideas out there. an overhaul of the accounts system is needed. And mdg is fine woth that even better. Right now Im working on separating email stuff from accounts-password for example

[splendido]

I'm sorry my post was incomplete.
Sharing these concerns together with a proposal with the changes we'd need to put things safer is what I meant to write.

The overhaul you're doing is great!

[brettle]

@zimme, I hope you didn't take offense at my "monkey patching" comment. None was intended. In fact, I don't see any way to achieve the "add service to the current logged in user and successfully log in as that same user (instead of throwing an error)" behavior without monkey patching.

As you mention, I did plenty of monkey patching in the workaround packages you mention, so I realize that sometimes it is needed. But just to be clear, the workaround packages that brettle:accounts-add-service depends on are no-ops under Meteor 1.2. Those packages workaround bugs that I submitted PRs for, and those PRs are included in Meteor 1.2. The only reason I continue to have those dependencies is to support Meteor versions < 1.2.

[zimme]

@brettle, I never get offended online ;) and I know you were only pointing out differences in approaches.

The more we talk this through the more I feel like maybe we should try and put together a PR for this which is opt-in behaviour to keep backwards compatibility, or do we want to do this as a separate package to have support for previous versions of meteor?

[zimme]

I've started this https://github.com/meteor-useraccounts/one-account/wiki/Architecture

Where I think we should outline what we expect from the behaviours of this solution. Once we have in black and white the behaviour we expect, we move on to see how we should implement this in the "best" way.

[brettle]

I've added my perspective to the wiki page. In terms of defining the problem, it might be more useful to frame it in terms of "user stories" from the perspective of the various "users" involved, namely the end user, the app developer, developers of login handler packages, and possibly Meteor core developers. I'll take a shot at that when I get a moment.

But first, I had a flash of insight concerning a solution (I think). Assuming the one-account security issue is addressed by requiring that a new service can only be added to the current logged in user, the primary (only?) advantage of one-account over accounts-add-service is that it doesn't treat adding a service as an error. I think I could address that by monkey patching Accounts._loginUser (and/or getting an equivalent PR accepted into the core). When that function is called, the (potentially new) account that the user is trying to logged in to has been identified, and any validateLoginAttempt handlers have passed. The new _loginUser would detect the case where (1) the user is already logged in (Meteor.userId() !== null) and (2) the user to login to is new (i.e. doesn't have any "resume" login tokens). Under those circumstances, the new user's service data would be merged into the current user, the new user would be deleted, and the original _loginUser would be called with userId = Meteor.userId(). The login would be considered successfuly and the user would remain logged in to the same account. Thoughts?

P.S. I somewhat misspoke about whether accounts-add-service requires any monkey patching to work with Meteor 1.2. While the functionality of accounts-add-service itself doesn't require any monkey patching, accounts-ui-unstyled does need to be monkey patched (by brettle:workaround-issue-5110) to hide the LoginCancelledError thrown when adding the password service. This is not an issue when using useraccounts instead of derivatives of accounts-ui-unstyled. It also wouldn't be required with the solution described above.

[zimme]

I don't think we can assume that an account without resume tokens are a "new" account. It might just be that an admin of a app have force logged out all users because of some reason.

The user stories thing I like, but we need to specify how the api is expected to work in each case too.

[brettle]

Good point on lack of resume tokens not necessarily meaning an account is new. I'm currently using that approach in brettle:accounts-add-service, so I just added issue brettle/meteor-accounts-add-service#1 to document it as well as a propose a fix.

However, I think I might have another better idea. Instead of monkey patching updateOrCreateUserFromExternalService, why not monkey patch Accounts.insertUserDoc (which is called by both updateOrCreateUserFromExternalService and Accounts.createUser). The service-adding code would be triggered when a login attempt is in progress and Meteor.user() !== null and it would just add the service to the current user's account and return the current user's ID. It would not run the onCreateUser handler or the onValidateNewUser handlers (because a new user was not created). The app developer should probably have some way to control under what conditions a new login service can be added to an existing account. Maybe we provide onValidateAddService?

Regarding the user stories and api, my thinking was that the user stories where the "users" are developers would specify how the api would work.

[splendido]

What if we start from the 'end-user'? Just to put clear the various flows we'd like to support...
Then we'll see whether they overlap or not and decide whether to implment them with different packages or simply by means of a configuration parameters.

As a side note, UA used to set forbidClientAccountCreation to true so to force the use of a custom server-side method to create new users while validating additional profile fields.
If we'll keep this unchanged, we don't have to care about client-side calls to Accounts.createUser.

[zimme]

For this package I think the client-side calls to createUser is irrelevant because we only work on server-side when a user is created and don't take into account from where the call came.

I like the idea of maybe working on inserUserDoc so we can intercept both createUser and updateOrCreateUserFromExternalService. Giving the developers the option to override a default merge handler is a nice option I think.

[zimme]

The end-user stories to get all the scenarios out there in text is a good idea, then we have developerstories on how they would use the package/api.