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

Possible to allow melding of unverified password account? #21

Open
steph643 opened this issue Jul 12, 2015 · 9 comments
Open

Possible to allow melding of unverified password account? #21

steph643 opened this issue Jul 12, 2015 · 9 comments

Comments

@steph643
Copy link
Contributor

From the doc:

At the moment it is not possible to do the contrary: a call to Meteor.loginWithPassword will log out the current user and login the one associated with the password service. After this, only in case the email used with the password service is already verified, the two account will be elected for melding

Why this limitation? Any technical difficulty?

@zacharynevin
Copy link

+1

@steph643
Copy link
Contributor Author

I guess the reason is that you intercept updateOrCreateUserFromExternalService(), which is not called when logging-in with a password...

A solution to this would be either:

  • to have access to the current userId from Accounts.validateLoginAttempt, or
  • to have access to the former userId from Accounts.onLogin.

What do you think? Maybe we should fill a feature request?

@brettle
Copy link

brettle commented Aug 23, 2015

brettle:accounts-multiple takes the approach @steph643 is describing. @splendido, would you consider a PR that modifies accounts-meld to use brettle:accounts-multiple instead of intercepting updateOrCreateUserFromExternalService()?

@splendido
Copy link
Owner

I had no chance to check brettle:accounts:multiple so far and, to be hoest, it's been quite a bit of time I'm not trying to update this package.
This is due to the little spare time and the work I'm doing on autopublish.meteor.com and the useraccounts packages.

For sure I'll try to revise all the logic behind this package when I'll be converting it to a plugin package for [email protected]

@splendido
Copy link
Owner

@steph643 is right, the call to Accounts.loginWithPassword is not intercepted at the moment, hence the normal flow is followed.

If there's something to deal with this without thouching the updateOrCreateUserFromExternalService part, I'll try to consider a PR (provided that comprehensive tests will be included as well...)

@steph643
Copy link
Contributor Author

@brettle, from my tests, in a Accounts.validateLoginAttempt() callback:

  • Meteor.userId() triggers an exception
  • this.userId returns undefined

So I don't understand how you would implement the PR. I am also surprised this line works :-)
There is obviously something I am missing. Can you please explain?

@brettle
Copy link

brettle commented Aug 24, 2015

brettle:accounts-multiple uses brettle:workaround-issue-4862 to workaround the Meteor bug that causes the problems you described.

FYI, MDG has merged a PR I submitted to fix the bug, so the workaround won't be necessary in Meteor 1.2+.

@steph643
Copy link
Contributor Author

Great, thanks.

brettle added a commit to brettle/meteor-accounts-meld that referenced this issue Sep 1, 2015
…ple}

Fix the "add new password account" case by just doing
`api.use('brettle:accounts-add-service')`.

Fix the "meld existing password account" case by copying logic from
`updateOrCreateUserFromExternalService()` into a `validateSwitch`
callback registered with `brettle:accounts-multiple`.

Add tests for both cases.
@brettle
Copy link

brettle commented Sep 1, 2015

PR #23 addresses this issue. FYI, the non-melding case (i.e. just associate a new email/password with the current account) can be handled by just doing meteor add brettle:accounts-add-service to existing apps. The PR adds brettle:accounts-add-service as a dependency to achieve the same effect. The melding case (i.e. melding an existing password account with the current account) required some minor additions to the code so it could use brettle:accounts-multiple.

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

No branches or pull requests

4 participants