JPAKE client: include X-KeyExchange-Log header in /report call on manual abort

RESOLVED FIXED in mozilla6

Status

Cloud Services
Firefox Sync: Backend
P4
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: philikon, Assigned: rnewman)

Tracking

unspecified
mozilla6
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed in services][qa-])

Attachments

(1 attachment, 1 obsolete attachment)

If the easy setup transaction is manually aborted by the user right now, we make a call to the /report API on the server, but don't include a value for the X-KeyExchange-Log. We should introduce one, I propose jpake.error.userabort, and send it along with the /report call. This will also make the client compliant with the update API spec for the Key Exchange server.
See also bug 649535.
(Assignee)

Updated

7 years ago
Assignee: nobody → rnewman
(Assignee)

Comment 2

7 years ago
Created attachment 525771 [details] [diff] [review]
v1
Attachment #525771 - Flags: review?(philipp)
(Assignee)

Comment 3

7 years ago
Manually verifying a real J-PAKE setup now.
Comment on attachment 525771 [details] [diff] [review]
v1

With your approach, we should probably make JPAKEClient.abort() *require* the 'reason' parameter. We also need to fix up the Fennec UI (now lives in the mobile/ subdir), not just the Firefox UI.

At that point it just seems easier to transform a null reason to JPAKE_ERROR_USERABORT in abort() and save ourselves from fixing up all the places where abort() is called. Do you agree?
(Assignee)

Comment 5

7 years ago
(In reply to comment #4)

> We also need to fix up the Fennec UI (now lives in the
> mobile/ subdir), not just the Firefox UI.

Diff already touched mobile/chrome/content/sync.js...
 
> At that point it just seems easier to transform a null reason to
> JPAKE_ERROR_USERABORT in abort() and save ourselves from fixing up all the
> places where abort() is called. Do you agree?

Heh, that was my first approach, but I figured you "be explicit" Pythonistas would r- :D

Updated patch to follow.
(In reply to comment #5)
> Diff already touched mobile/chrome/content/sync.js...

Oops, I missed that! Sorry! (Also, very nice!)
(Assignee)

Comment 7

7 years ago
Created attachment 525783 [details] [diff] [review]
v2
Attachment #525771 - Attachment is obsolete: true
Attachment #525771 - Flags: review?(philipp)
Attachment #525783 - Flags: review?(philipp)
Comment on attachment 525783 [details] [diff] [review]
v2

r=me

Let's hold off on landing this until we have communicated this change to Tarek and infrasec. atoll already said he was ok with it (bug 649535 comment 15).

Once we land it we also need to update https://wiki.mozilla.org/Services/Sync/SyncKey/J-PAKE
Attachment #525783 - Flags: review?(philipp) → review+
(Assignee)

Updated

7 years ago
Priority: -- → P4
Whiteboard: [needs landing]
I spoke to mcoates this morning. He says he's fine with having user aborts show up in the logs, so long as they're distinguishable from actual failures. Given that we report them with a new log constant ("jpake.error.userabort"), this concern should be met already.

This should be ready to hit s-c then.
(Assignee)

Comment 10

7 years ago
(In reply to comment #9)

> This should be ready to hit s-c then.

Take that, Sauron!

https://hg.mozilla.org/services/services-central/rev/d563f8af5db1
Whiteboard: [needs landing] → [fixed in services]
Great. Please don't forget to update https://wiki.mozilla.org/Services/Sync/SyncKey/J-PAKE. Thanks!
(Assignee)

Comment 12

7 years ago
(In reply to comment #11)
> Great. Please don't forget to update
> https://wiki.mozilla.org/Services/Sync/SyncKey/J-PAKE. Thanks!

Done. I didn't take the liberty of bumping any API versions, as this is a trivial extension, but I did date-tag it.

I'll leave server behavior changes to Tarek to document.
(Reporter)

Updated

7 years ago
Whiteboard: [fixed in services] → [fixed in services][qa-]
http://hg.mozilla.org/mozilla-central/rev/d563f8af5db1
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
Is this and/or should this be in aurora?
(In reply to comment #14)
> Is this and/or should this be in aurora?

It's not in Aurora. We could ask for approval since the patch's risk is minimal, but then again, its benefit is also minimal, so I wasn't going to bother.
Depends on: 656349
You need to log in before you can comment on or make changes to this bug.