Skip to content
This repository has been archived by the owner. It is now read-only.

proxy for account/reset #249

Merged
merged 5 commits into from Nov 14, 2013
Merged

proxy for account/reset #249

merged 5 commits into from Nov 14, 2013

Conversation

@dannycoates
Copy link
Member

@dannycoates dannycoates commented Nov 13, 2013

Analogous to account and session create, we would need a proxy to do account resets. /raw_password/account/reset ?

@zaach
Copy link
Contributor Author

@zaach zaach commented Oct 24, 2013

Another option is to host the entire flow on the web.

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Oct 24, 2013

This is important, but is this actually a blocker for you guys right now?

@zaach
Copy link
Contributor Author

@zaach zaach commented Oct 25, 2013

I think people were jazzed about hosting the whole flow from the "jelly". So, we'd put a link in the Settings on FxOS that points to the browser. In that case, the jelly takes the fall. We could still provide a proxy endpoint for the web, but the major gain is slimming the client library we ship on the phone. Without password reset, we could get away with leaving out sjcl, ect.

@dannycoates
Copy link
Member

@dannycoates dannycoates commented Oct 25, 2013

I can throw this together tonight if it helps

@zaach
Copy link
Contributor Author

@zaach zaach commented Oct 25, 2013

It's not vital -- we can still redirect users to the jelly and have them wait several seconds for the key-stretching to complete. The major gain is doing all the crypto remotely, which we get with a redirect or a proxy. Let's go with redirect, since jgruen et al were enthusiastic about it. The jelly can already handle a redirect to the password reset flow.

@zaach zaach closed this Oct 25, 2013
@ckarlof ckarlof reopened this Oct 25, 2013
@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Oct 25, 2013

I think we need to discuss this further, as well as our near to medium term plans for the proxy endpoints.

@zaach
Copy link
Contributor Author

@zaach zaach commented Oct 25, 2013

Yeah, the proxy endpoints could be useful for demoing in the short term. Ideally, we'll be able to land the native crypto in time for FxOS 1.3. I don't think we want to support these proxy endpoints forever, which tends to happen when we ship stuff on the phone.

@ghost ghost assigned dannycoates Nov 12, 2013
@dannycoates
Copy link
Member

@dannycoates dannycoates commented Nov 12, 2013

/account/reset serves two cases, change password and change wrapKb.

// /raw_password/account/reset
// request body
{
  "email": "me@example.com",
  "old_password": "foo",
  "new_password": "bar",
  "wrapKb": "ABBA" // do we want this?
}

Do we want to allow changing wrapKb with this endpoint?

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Nov 13, 2013

Do we want to allow changing wrapKb with this endpoint?

The alternative is that the server would generate a new random one. /account/reset could also function this way. @warner should weigh in.

@dannycoates
Copy link
Member

@dannycoates dannycoates commented Nov 13, 2013

The server currently generates a new random wrapKb if the unbundled value of the submitted wrapKb is 32bytes of 0s. If I recall correctly the intent of sending wrapKb is to maintain the existing value. For a raw_password equivalent I think a property specifying if wrapKb should be reset would be sufficient.

{
  "email": "me@example.com",
  "old_password": "foo",
  "new_password": "bar",
  "reset_wrap_kb": true // false or excluded to keep the same
}

???

@dannycoates
Copy link
Member

@dannycoates dannycoates commented Nov 13, 2013

I implemented /raw_password/account/reset with the optional parameter resetWrapKb. I left it undocumented in api.md. If we want to keep it I'll add it to the docs and if we don't I'll delete the parameter.

f? @ckarlof @zaach @warner
r? @rfk

@@ -79,95 +79,6 @@ TestServer.start(config.public_url)
)

test(
'(reduced security) Login with email and password',

This comment has been minimized.

@rfk

rfk Nov 13, 2013
Member

I really like having these moved into a separate file

This comment has been minimized.

@ckarlof

ckarlof Nov 13, 2013
Contributor

Too dirty to be with the main code? ;)

This comment has been minimized.

@rfk

rfk Nov 13, 2013
Member

Yes, I like to imagine being able to rip it out one day, even if I know that day may never come ;-)

@rfk
Copy link
Member

@rfk rfk commented Nov 13, 2013

It's not entirely clear from the code, is resetWrapKb a boolean flag or is it the old wrapKb? It seems to be the later. Otherwise code looks fine.

Something about futzing with wrapKb in this flow is deeply unsettling to me. IIUC it's not possible to obtain a keyFetchToken via the raw_password API, is that accurate? If so then I argue that it doesn't make sense to expose key-handling functionality via other raw_password endpoints either.

I'm going to wait for @warner and @zaach feedback before signing off on this.

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Nov 13, 2013

@dannycoates and I discussed. This endpoint should be something like /raw_password/password/change and reset_wrap_kb should be omitted (the server will manage that). In addition we'll need (proposed) /raw_password/password/reset that takes:

{
  "accountResetToken": "99ce20e3f5391e134596c2ac55c0520f2edfb026761443da0ab27b1fa18c98912af6291714e9600aa349917519979b93a45e6d0da34c7509c5632ac35b2865d3",
  "newPassword": "bar"
}

the accountResetToken is gotten from a call to /password/forgot/verify_code. Our convention is camelCase here not snake_case, correct?

@rfk
Copy link
Member

@rfk rfk commented Nov 13, 2013

Our convention is camelCase here not snake_case, correct?

nod

…nge for changing password and /raw_password/password/reset for forgot password
@dannycoates
Copy link
Member

@dannycoates dannycoates commented Nov 13, 2013

Its actually easier (for me) to HAWK the reset request with the accountResetToken instead of passing it in the body. Unless its a problem for the client I'd rather do that.

@dannycoates
Copy link
Member

@dannycoates dannycoates commented Nov 13, 2013

@rfk test for /raw_password/password/reset is still coming...

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Nov 14, 2013

Its actually easier (for me) to HAWK the reset request with the accountResetToken instead of passing it in the body. Unless its a problem for the client I'd rather do that.

We can of course be flexible here. Just so I understand, you're proposing:

/raw_password/password/reset takes:

{
  "newPassword": "bar"
}

and is Hawk-ed with the accountResetToken?

@dannycoates
Copy link
Member

@dannycoates dannycoates commented Nov 14, 2013

@ckarlof yes

@rfk ready for r? (ps - I wrote this in a hurry)


:lock: HAWK-authenticated with accountResetToken

This resets the account password and resets the encrypted "wrap(kB)" value.

This comment has been minimized.

@rfk

rfk Nov 14, 2013
Member

worth adding something like "...to a new randomly-generated value" to make more explicit what happens to wrapKb?

This comment has been minimized.

@dannycoates

dannycoates Nov 14, 2013
Author Member

ok

* status code 413, errno 113: request body too large


## POST /v1/raw_password/password/change

This comment has been minimized.

@rfk

rfk Nov 14, 2013
Member

What does it to to wrapKb?

This comment has been minimized.

@dannycoates

dannycoates Nov 14, 2013
Author Member

wrapKb is unchanged

@rfk
Copy link
Member

@rfk rfk commented Nov 14, 2013

Awesome, I'm feeling much better about this with the new endpoints like that. r+ with doc nits as noted above.

@ckarlof
Copy link
Contributor

@ckarlof ckarlof commented Nov 14, 2013

Awesome, I'm feeling much better about this with the new endpoints like that. r+ with doc nits as noted above.

Feeling better with more /raw_password endpoints? Welcome to the dark side, dude.

@dannycoates
Copy link
Member

@dannycoates dannycoates commented Nov 14, 2013

@rfk updated api.md. :shipit:

rfk added a commit that referenced this pull request Nov 14, 2013
@rfk rfk merged commit 258f350 into mozilla:master Nov 14, 2013
1 check passed
1 check passed
@rik
default The Travis CI build passed
Details
@dannycoates dannycoates deleted the dannycoates:i249 branch Apr 30, 2015
vladikoff added a commit that referenced this pull request Feb 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants