hoodiehq / hoodie-account-client

:dog: Account client API for the browser
https://hoodiehq.github.io/hoodie-account-client
Apache License 2.0
11 stars 24 forks source link

`signin` is not fired when logging in via token #144

Closed rmehner closed 7 years ago

rmehner commented 7 years ago

How to reproduce

  1. Create account for user via hoodie-account-server
  2. Add login token for user on server:
api.account({username: username}).tokens.add({
  id: 'MY_SECRET_TOKEN',
  type: 'login',
  recipient: username,
  timeout: 7200 // 2h
})
  1. On the client, register for events:
account.on('signin', () => console.log('sign in, woop!'))
  1. Sign in via hoodie-account-client
account.signIn({
  username: username,
  token: 'MY_SECRET_TOKEN'
})

Expected behaviour

  1. User is signed in.
  2. See sign in, woop! in the console.

Actual behaviour

  1. User is signed in.
  2. See nothing in the console.

Note: the event reauthenticate is fired after a sign in, which I currently use a workaround :)

gr2m commented 7 years ago

for anyone who wants to work on this. I’m very confident the problem is in this line: https://github.com/hoodiehq/hoodie-account-client/blob/69cd117f64435921619c8018e7fcd52a03e9a9f3/lib/sign-in.js#L45

It returns true because both get(state, 'account.username') and options.username are undefined in the case Robin describes above (signed out, signing in with token). We should add an additional check if get(state, 'account.username') returns undefined we should always emit signin, because it means the user is currently not signed in.

While at it, we shouldn’t compare to options.username but to data.account.username which is what the server returns. That will correctly trigger a reauthenticate event for a user that is already signed in and reauthenticates using a token, not username & password

gr2m commented 7 years ago

Actually, we should not compare usernames at all :) Instead, we should compare the user account id. So the if block would look something like this

if (state.account.id === data.account.id) {
  emitEvent = 'reauthenticate'
}

Because for one not all user accounts will have usernames and besides that a username could have changed, but we would still like a user to reauthenticate

sohini-roy commented 7 years ago

thank you for the description. it was very helpful :) i would love to work on this issue

gr2m commented 7 years ago

done via https://github.com/hoodiehq/hoodie-account-client/issues/157