3meters / proxibase

Patchr web service
1 stars 0 forks source link

Password reset: use branch links to pass reset token to mobile clients #409

Closed Jaymassena closed 7 years ago

Jaymassena commented 8 years ago

I think the service will need to generate a branch link for each password reset call. They have a very small javascript sdk that is available via npm.

https://www.npmjs.com/package/branch-sdk

Let's use this issue to coordinate the branch part of the feature. I think that the custom data we add to the link should include at least these metadata parameters:

resetToken: String (I'm assuming this probably has everything you need to identify the user account) resetUserName: String resetUserPhoto: String (I think we can get away with just passing the photo.prefix value if one)

The user photo and name are displayed in the client so they recognize the account being reset.

Let me know what credentials you need to make the call to branch.

Branch supports a special parameter that will override the redirect url if the user clicks on the reset email on a non-mobile device. We will want to use it to send them to a page that nicely asks them to click the link from mail on their mobile device.

Jaymassena commented 8 years ago

More on the branch sdk: https://github.com/BranchMetrics/Smart-App-Banner-Deep-Linking-Web-SDK

Jaymassena commented 8 years ago

Here is the branch key: key_live_aabKfhL9u6piQ93E2lzVslklzraKnSc3

Jaymassena commented 8 years ago

Here is the special param I mentioned: $desktop_url

I think we should redirect them to patchr.com/reset

georgesnelling commented 8 years ago

Ok I have an implementation that calls branch and creates a deep link. Whether or not it works for you we will have to test from the client, since hitting the url from the browser only returns us to the public patcher page for now.

To request a password reset by email:

method: post, path: '/user/pw/reqreset', body: {email: user.email},

This send an email to the user with a branch url in it. Clicking the branch url from a patchr-enabled device should open patch and pass back in a data object containing:

token: token, userName: user.name, userPhoto: user.photo.prefix,

Assuming this works (this is the part I haven't figured out how to test yet), reset the password using

path: /user/pw/reset body: {token: token, password: password}

Please let me know if this works. I'm looking into some automated test strategies for the branch link stuff in the mean time.

georgesnelling commented 8 years ago

Finally figured out how to confirm that branch has the data that we intend. I believe this works now and is ready for the client implementation. Let me know if you see any problems.

Jaymassena commented 8 years ago

I'm getting a 404 from the service but I think my endpoint is per spec.

"https://api.aircandi.com/v1/user/pw/reset"

Call is a post and body includes token and new password.

georgesnelling commented 8 years ago

Per discussion, if the email is not found in /usr/pw/reqreset the service now returns error status 401 with error code 401.4, email not found.

If the token is not found or is expired in /usr/pw/reset, the service now returns 401 with error code 401.1, bad authorization, the same error is returned for an invalid password.

Tests, including failure cases are in basic/02_passwords.js#resetPasswordByEmail

Also as per discussion, the expiration window for password reset is now 1 hour.

Jaymassena commented 8 years ago

What happens if a token that has already been used to reset a password is used again in a call to /user/pw/reset?

georgesnelling commented 8 years ago

Once the token is spent resetting a password it is deleted, so you should get back a 401.1 on subsequent calls with the same token. Is that not what you expect?

Jaymassena commented 8 years ago

Yes, just wanted to confirm that the tokens are one-use only.

Jaymassena commented 8 years ago

I'm seeing something extra at the bottom of the reset emails, after the "-The Patchr team" signature. It doesn't happen for the first reset request but shows on the next one. See below

Hi Jay Massena,

We got a request to reset your Patchr password.

Please click this link from your phone to reset it:

https://bnc.lt/cdud/sdDQnxDpcs

If you ignore this message, your password won't be changed.

Thanks!

-The Patchr team

Jay Massena Patchr https://bnc.lt/cdud/E0ffeY9pcs Patchr Jay Massena Patchr https://bnc.lt/cdud/slOZLmEqcs Patchr
Jaymassena commented 8 years ago

I'm also getting a 401.1 back on reset calls and the token was just generated. Seems like something is hanging around in the system so a fresh reset request is influenced by a previous request.

georgesnelling commented 8 years ago

Looking through the logs it looks like the client posted this request 5 times:

post /v1/user/pw/reset { token: 'c75c636b52d0a3f618f3d8d744e05037e9fb034b', password: '**' }

Can you confirm that the branch.io urls in the various password reset emails you received are different from one another? I can add some logging to record those transactions, but we don't keep it around now.

Jaymassena commented 8 years ago

I'll check but I have noticed multiple calls to branch can return the same link signature if they deem the payload to be identical.

Jaymassena commented 8 years ago

The three request emails I have from today all have the same branch link: https://bnc.lt/cdud/sdDQnxDpcs

Yesterday, I have two request emails with the same link (not the same as todays) and one that is different.

Jaymassena commented 8 years ago

Because of their focus on deep linking, we might need to add something additional to the branch call to make them see it as unique. I'm not complete certain of how they treat custom data included in the payload with regards to uniqueness.

georgesnelling commented 8 years ago

That was certainly my intention, but clearly I did not succeed. I think I just push a fix to the string bug, but it's a pain to test. Would you mind trying that again to confirm it?

georgesnelling commented 8 years ago

As discussed, this was caused by the string bug. It was corrupting the emails themselves.

Just pushed version 3.8.4 which signs the user in as the final step of a successful password reset.

Jaymassena commented 8 years ago

I'm seeing this when I try to request a reset.

2016-03-31 14:39:52:188 Patchr[2860:2977796] Network Error Summary [;g255,127,0;2016-03-31 14:39:52:189 Patchr[2860:2977796] Forbidden: You have tried to reset this password too many times. Contact support. [;g255,127,0;2016-03-31 14:39:52:189 Patchr[2860:2977796] 403.0 [;g255,127,0;2016-03-31 14:39:52:189 Patchr[2860:2977796] Request failed: forbidden (403)

How long do I have to wait between tests?

georgesnelling commented 8 years ago

It only lets five unused tokens per user accumulate before locking you out. I just cleaned out all the tokens. As long as you succeed most times (starting over now) it should be ok. Can come up with something fancier if need be.

Jaymassena commented 8 years ago

It is kind of easy to imagine accumulating five because of the testing/debugging focus right now especially if there isn't a good way to ever clear it out (other than manually). Does a successful reset clear out the unused tokens?

georgesnelling commented 8 years ago

yes

Jaymassena commented 8 years ago

Everything is looking good. I think we can call this implemented.

Jaymassena commented 7 years ago

Branch had to make some changes to expand their namespace capacity to support universal links and app links. We have a new address to use:

Old: bnc.lt/cdud New: bvvb.app.link

georgesnelling commented 7 years ago

We call them to generate the deep links, and as far as I can tell they are already generating the new form with no change needed on our side. The string 'bnc.lt' is not in our source code, and when I run our password tests and debug the links they return us, they look like this:

https://bvvb.test-app.link/sbrHWyEeDv

I don't think we need to change anything on our side. I'm going to close this one. If you see things differently please reopen.