Closed Bug 1132343 Opened 5 years ago Closed 5 years ago

[bluetooth2] Support consent and displaypasskey pairing requests

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox40 fixed)

RESOLVED FIXED
2.2 S10 (17apr)
Tracking Status
firefox40 --- fixed

People

(Reporter: yrliou, Assigned: yrliou)

References

Details

(Whiteboard: [webbt-api])

Attachments

(2 files, 6 obsolete files)

31.62 KB, patch
Details | Diff | Splinter Review
2.49 KB, patch
Details | Diff | Splinter Review
In order to fully support consent and displaypasskey types in APIv2, we will need to

1) expose methods for gaia to confirm or cancel the incoming consent request from our users.
2) expose method for gaia to cancel the displaypasskey request.

To fulfill them, accept() and reject() are added into BluetoothPairingHandle[1].
This bug is for implementing these two methods to support these two pairing types from gecko side. Gaia work will be traced by another bug.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#BluetoothPairingHandle
Blocks: 1132345
Duplicate of this bug: 1131910
Assignee: nobody → joliu
Hi Shawn,

This patch is implementing accept/reject method as described in [1] to support consent and displaypasskey requests.
Please help to review my patch.

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#BluetoothPairingHandle

Thanks,
Jocelyn
Attachment #8585434 - Flags: review?(shuang)
patch summary:
  - Replace pre-defined literal string with BluetoothSspVariant HAL and daemon interface.
  - add accept/reject methods in PairingHandle
  - Use PinReplyInternal and SspReplyInternal for accepting or rejecting pairing requests.
Fix some typo...
(In reply to jocelyn [:jocelyn] from comment #3)
> patch summary:
>   - Replace pre-defined literal string with BluetoothSspVariant in HAL and
> daemon interface.
>   - add accept/reject methods in PairingHandle
>   - Use PinReplyInternal and SspReplyInternal for accepting or rejecting
> pairing requests.
diff from v1:
- remove setPairingConfrimation from BluetoothPairingHandle.cpp/h/webidl
- replace setPairingConfirmation call in marionette test
Attachment #8585434 - Attachment is obsolete: true
Attachment #8585434 - Flags: review?(shuang)
Attachment #8585931 - Flags: review?(shuang)
Comment on attachment 8585931 [details] [diff] [review]
Bug 1132343(v2) - Add accept/reject method in BluetoothPairingHandle interface for supporting consent and displaypasskey pairing requests.

Review of attachment 8585931 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for fixing this. I have some comments and let me know if you are ready to review again.

::: dom/bluetooth2/BluetoothService.h
@@ +247,5 @@
> +                   bool aAccept,
> +                   BluetoothReplyRunnable* aRunnable) = 0;
> +
> +  virtual void
> +  SetPinCodeInternal(const nsAString& aDeviceAddress,

PinReplyInternal and SetPinCodeInternal, (well and also SetPasskeyInternal) both are similar functions. If you have some strong reason to keep them both exist, please leave a comment to address this is related to legacy implementation and you prefer not to touch the old implementation.

::: dom/bluetooth2/bluedroid/BluetoothDaemonHelpers.cpp
@@ +583,4 @@
>    return NS_OK;
>  }
>  
>  nsresult

Why do you need to remove Convert function in BluetoothDaemonHelpers? This is a bit confusing for me.

::: dom/bluetooth2/bluedroid/BluetoothHALHelpers.h
@@ +129,5 @@
>  }
>  
>  inline nsresult
> +Convert(const BluetoothSspVariant& aIn, bt_ssp_variant_t& aOut)
> +{

This should add to BluetoothDaemonHelpers, right?

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
@@ +820,5 @@
> +BluetoothServiceBluedroid::SetPinCodeInternal(
> +  const nsAString& aDeviceAddress, const nsAString& aPinCode,
> +  BluetoothReplyRunnable* aRunnable)
> +{
> +}

Add comments to explain why we left it blank.

@@ +826,5 @@
> +void
> +BluetoothServiceBluedroid::SetPasskeyInternal(
> +  const nsAString& aDeviceAddress, uint32_t aPasskey,
> +  BluetoothReplyRunnable* aRunnable)
> +{

Add comments to explain why we left it blank.

@@ +833,5 @@
>  void
> +BluetoothServiceBluedroid::SetPairingConfirmationInternal(
> +  const nsAString& aDeviceAddress, bool aConfirm,
> +  BluetoothReplyRunnable* aRunnable)
> +{

Add comments to explain why we left it blank.

::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
@@ +4313,5 @@
> +void
> +BluetoothDBusService::PinReplyInternal(
> +  const nsAString& aDeviceAddress, bool aAccept,
> +  const nsAString& aPinCode, BluetoothReplyRunnable* aRunnable)
> +{

Add comments to explain why we left it blank.

@@ +4320,5 @@
> +void
> +BluetoothDBusService::SspReplyInternal(
> +  const nsAString& aDeviceAddress, BluetoothSspVariant aVariant,
> +  bool aAccept, BluetoothReplyRunnable* aRunnable)
> +{

Add comments to explain why we left it blank.

::: dom/bluetooth2/tests/marionette/head.js
@@ +752,2 @@
>      log("  - Received 'onpairingconfirmationreq' event with passkey: " + passkey);
>  

Can we split this marionette test patch into patch 2?

::: dom/webidl/BluetoothPairingHandle.webidl
@@ +14,5 @@
>    readonly attribute DOMString passkey;
>  
> +  /**
> +   * Reply pin code for enterpincodereq. The promise will be rejected if the
> +   * pairing request type is not enterpincodereq or operation fails.

nit: PAIRING_REQ_TYPE_ENTERPINCODE

@@ +21,5 @@
>    Promise<void> setPinCode(DOMString aPinCode);
> +
> +  /**
> +   * Accept pairing requests. The promise will be rejected if the pairing
> +   * request type is not pairingconfirmationreq or pairingconsentreq or

nit: PAIRING_REQ_TYPE_CONFIRMATION  or  PAIRING_REQ_TYPE_CONSENT.
Attachment #8585931 - Flags: review?(shuang)
Thanks for your time, Shawn.
Please see my replies below.
(In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> Comment on attachment 8585931 [details] [diff] [review]
> Bug 1132343(v2) - Add accept/reject method in BluetoothPairingHandle
> interface for supporting consent and displaypasskey pairing requests.
> 
> Review of attachment 8585931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for fixing this. I have some comments and let me know if you are
> ready to review again.
> 
> ::: dom/bluetooth2/BluetoothService.h
> @@ +247,5 @@
> > +                   bool aAccept,
> > +                   BluetoothReplyRunnable* aRunnable) = 0;
> > +
> > +  virtual void
> > +  SetPinCodeInternal(const nsAString& aDeviceAddress,
> 
> PinReplyInternal and SetPinCodeInternal, (well and also SetPasskeyInternal)
> both are similar functions. If you have some strong reason to keep them both
> exist, please leave a comment to address this is related to legacy
> implementation and you prefer not to touch the old implementation.
> 

Sure.

> ::: dom/bluetooth2/bluedroid/BluetoothDaemonHelpers.cpp
> @@ +583,4 @@
> >    return NS_OK;
> >  }
> >  
> >  nsresult
> 
> Why do you need to remove Convert function in BluetoothDaemonHelpers? This
> is a bit confusing for me.
> 

Because it's no longer needed since we pass BluetoothSspVariant directly when calling SspReply and SspReplyCmd.

> ::: dom/bluetooth2/bluedroid/BluetoothHALHelpers.h
> @@ +129,5 @@
> >  }
> >  
> >  inline nsresult
> > +Convert(const BluetoothSspVariant& aIn, bt_ssp_variant_t& aOut)
> > +{
> 
> This should add to BluetoothDaemonHelpers, right?
> 

As I mentioned above, I think there's no need to add it.

> ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
> @@ +820,5 @@
> > +BluetoothServiceBluedroid::SetPinCodeInternal(
> > +  const nsAString& aDeviceAddress, const nsAString& aPinCode,
> > +  BluetoothReplyRunnable* aRunnable)
> > +{
> > +}
> 
> Add comments to explain why we left it blank.
> 

Sure.

> @@ +826,5 @@
> > +void
> > +BluetoothServiceBluedroid::SetPasskeyInternal(
> > +  const nsAString& aDeviceAddress, uint32_t aPasskey,
> > +  BluetoothReplyRunnable* aRunnable)
> > +{
> 
> Add comments to explain why we left it blank.
> 
> @@ +833,5 @@
> >  void
> > +BluetoothServiceBluedroid::SetPairingConfirmationInternal(
> > +  const nsAString& aDeviceAddress, bool aConfirm,
> > +  BluetoothReplyRunnable* aRunnable)
> > +{
> 
> Add comments to explain why we left it blank.
> 
> ::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
> @@ +4313,5 @@
> > +void
> > +BluetoothDBusService::PinReplyInternal(
> > +  const nsAString& aDeviceAddress, bool aAccept,
> > +  const nsAString& aPinCode, BluetoothReplyRunnable* aRunnable)
> > +{
> 
> Add comments to explain why we left it blank.
> 

We left all new functions blank in DBusService, do we need to do this in DBusService?
If you think it's needed, we can open another followup bug to revise all of them.

> ::: dom/bluetooth2/tests/marionette/head.js
> @@ +752,2 @@
> >      log("  - Received 'onpairingconfirmationreq' event with passkey: " + passkey);
> >  
> 
> Can we split this marionette test patch into patch 2?
> 

No problem.

> ::: dom/webidl/BluetoothPairingHandle.webidl
> @@ +14,5 @@
> >    readonly attribute DOMString passkey;
> >  
> > +  /**
> > +   * Reply pin code for enterpincodereq. The promise will be rejected if the
> > +   * pairing request type is not enterpincodereq or operation fails.
> 
> nit: PAIRING_REQ_TYPE_ENTERPINCODE
> 
> @@ +21,5 @@
> >    Promise<void> setPinCode(DOMString aPinCode);
> > +
> > +  /**
> > +   * Accept pairing requests. The promise will be rejected if the pairing
> > +   * request type is not pairingconfirmationreq or pairingconsentreq or
> 
> nit: PAIRING_REQ_TYPE_CONFIRMATION  or  PAIRING_REQ_TYPE_CONSENT.

This value only defines in gecko, wouldn't it be better to use the actual string in webidl?
(In reply to jocelyn [:jocelyn] from comment #7)
> Thanks for your time, Shawn.
> Please see my replies below.
> (In reply to Shawn Huang [:shawnjohnjr] from comment #6)
> > Comment on attachment 8585931 [details] [diff] [review]
> > Bug 1132343(v2) - Add accept/reject method in BluetoothPairingHandle
> > interface for supporting consent and displaypasskey pairing requests.
> > 
> > Review of attachment 8585931 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks for fixing this. I have some comments and let me know if you are
> > ready to review again.
> > 
> > ::: dom/bluetooth2/BluetoothService.h
> > @@ +247,5 @@
> > > +                   bool aAccept,
> > > +                   BluetoothReplyRunnable* aRunnable) = 0;
> > > +
> > > +  virtual void
> > > +  SetPinCodeInternal(const nsAString& aDeviceAddress,
> > 
> > PinReplyInternal and SetPinCodeInternal, (well and also SetPasskeyInternal)
> > both are similar functions. If you have some strong reason to keep them both
> > exist, please leave a comment to address this is related to legacy
> > implementation and you prefer not to touch the old implementation.
> > 
> 
> Sure.
> 
> > ::: dom/bluetooth2/bluedroid/BluetoothDaemonHelpers.cpp
> > @@ +583,4 @@
> > >    return NS_OK;
> > >  }
> > >  
> > >  nsresult
> > 
> > Why do you need to remove Convert function in BluetoothDaemonHelpers? This
> > is a bit confusing for me.
> > 
> 
> Because it's no longer needed since we pass BluetoothSspVariant directly
> when calling SspReply and SspReplyCmd.
Agree. I also revisit this part and found this actually been handled.
> 
> > ::: dom/bluetooth2/bluedroid/BluetoothHALHelpers.h
> > @@ +129,5 @@
> > >  }
> > >  
> > >  inline nsresult
> > > +Convert(const BluetoothSspVariant& aIn, bt_ssp_variant_t& aOut)
> > > +{
> > 
> > This should add to BluetoothDaemonHelpers, right?
> > 
> 
> As I mentioned above, I think there's no need to add it.
Agree.
> 
> > ::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.cpp
> > @@ +820,5 @@
> > > +BluetoothServiceBluedroid::SetPinCodeInternal(
> > > +  const nsAString& aDeviceAddress, const nsAString& aPinCode,
> > > +  BluetoothReplyRunnable* aRunnable)
> > > +{
> > > +}
> > 
> > Add comments to explain why we left it blank.
> > 
> 
> Sure.
> 
> > @@ +826,5 @@
> > > +void
> > > +BluetoothServiceBluedroid::SetPasskeyInternal(
> > > +  const nsAString& aDeviceAddress, uint32_t aPasskey,
> > > +  BluetoothReplyRunnable* aRunnable)
> > > +{
> > 
> > Add comments to explain why we left it blank.
> > 
> > @@ +833,5 @@
> > >  void
> > > +BluetoothServiceBluedroid::SetPairingConfirmationInternal(
> > > +  const nsAString& aDeviceAddress, bool aConfirm,
> > > +  BluetoothReplyRunnable* aRunnable)
> > > +{
> > 
> > Add comments to explain why we left it blank.
> > 
> > ::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
> > @@ +4313,5 @@
> > > +void
> > > +BluetoothDBusService::PinReplyInternal(
> > > +  const nsAString& aDeviceAddress, bool aAccept,
> > > +  const nsAString& aPinCode, BluetoothReplyRunnable* aRunnable)
> > > +{
> > 
> > Add comments to explain why we left it blank.
> > 
> 
> We left all new functions blank in DBusService, do we need to do this in
> DBusService?
> If you think it's needed, we can open another followup bug to revise all of
> them.
> 
I prefer to do it by the follow up bug.
> > ::: dom/bluetooth2/tests/marionette/head.js
> > @@ +752,2 @@
> > >      log("  - Received 'onpairingconfirmationreq' event with passkey: " + passkey);
> > >  
> > 
> > Can we split this marionette test patch into patch 2?
> > 
> 
> No problem.
> 
> > ::: dom/webidl/BluetoothPairingHandle.webidl
> > @@ +14,5 @@
> > >    readonly attribute DOMString passkey;
> > >  
> > > +  /**
> > > +   * Reply pin code for enterpincodereq. The promise will be rejected if the
> > > +   * pairing request type is not enterpincodereq or operation fails.
> > 
> > nit: PAIRING_REQ_TYPE_ENTERPINCODE
> > 
> > @@ +21,5 @@
> > >    Promise<void> setPinCode(DOMString aPinCode);
> > > +
> > > +  /**
> > > +   * Accept pairing requests. The promise will be rejected if the pairing
> > > +   * request type is not pairingconfirmationreq or pairingconsentreq or
> > 
> > nit: PAIRING_REQ_TYPE_CONFIRMATION  or  PAIRING_REQ_TYPE_CONSENT.
> 
> This value only defines in gecko, wouldn't it be better to use the actual
> string in webidl?
hmm. I don't have strong intention to force you. It's ok.
- Add a comment in DBusService for those unimplemented methods
Attachment #8586085 - Attachment is obsolete: true
Attachment #8586085 - Flags: review?(shuang)
Attachment #8586098 - Flags: review?(shuang)
Comment on attachment 8586098 [details] [diff] [review]
Bug 1132343 - Patch1(v3): Add accept/reject method in BluetoothPairingHandle interface for supporting consent and displaypasskey pairing requests.

Review of attachment 8586098 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth2/BluetoothPairingHandle.cpp
@@ +115,4 @@
>    nsRefPtr<BluetoothReplyRunnable> result =
>      new BluetoothVoidReplyRunnable(nullptr /* DOMRequest */,
>                                     promise,
> +                                   NS_LITERAL_STRING("Accept"));

Do you think we shall add basic description/comments for the relationship regarding content table ("Pairing type v.s. BluetoothPairingHandle content")? Because |Accept| method only handles SspReplyInternal but |Reject| method handles both PinReplyInternal/SspReplyInternal. I guess most people might think it should be taking care both PinReply/SspReply.

::: dom/bluetooth2/bluez/BluetoothDBusService.cpp
@@ +4317,5 @@
> +void
> +BluetoothDBusService::PinReplyInternal(
> +  const nsAString& aDeviceAddress, bool aAccept,
> +  const nsAString& aPinCode, BluetoothReplyRunnable* aRunnable)
> +{

nit:Please add comments for blank implementation.

@@ +4324,5 @@
> +void
> +BluetoothDBusService::SspReplyInternal(
> +  const nsAString& aDeviceAddress, BluetoothSspVariant aVariant,
> +  bool aAccept, BluetoothReplyRunnable* aRunnable)
> +{

Ditto.
Attachment #8586098 - Flags: review?(shuang)
- add some comments in BluetoothPairingHandle.h
Attachment #8586098 - Attachment is obsolete: true
Attachment #8586592 - Flags: review?(shuang)
Comment on attachment 8586592 [details] [diff] [review]
Bug 1132343 - Patch1(v4): Add accept/reject method in BluetoothPairingHandle interface for supporting consent and displaypasskey pairing requests.

Review of attachment 8586592 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good.
Attachment #8586592 - Flags: review?(shuang) → review+
Comment on attachment 8586592 [details] [diff] [review]
Bug 1132343 - Patch1(v4): Add accept/reject method in BluetoothPairingHandle interface for supporting consent and displaypasskey pairing requests.

Hi Blake,

We have revised our BluetoothPairingHandle interface[1] which is responsible to reply pairing requests from remote devices.
There're four pairing request types, enterpincodereq, displaypasskeyreq, pairingconfirmationreq, and pairingconsentreq.

In our previous API design, there are no way to reject pairing requests except for pairingconfirmationreq. And we lack of an API for accepting pairingconsentreq.

In this bug, we add
  - accept(): accept pairingconfirmationreq or pairingconsentreq pairing requests
  - reject(): reject all types of pairing requests
Also remove setPairingConfirmation() since we use accept() and reject() now.

Could you help to review this API change in BluetoothPairingHandle.webidl/cpp/h?

Thanks,
Jocelyn

[1] https://wiki.mozilla.org/B2G/Bluetooth/WebBluetooth-v2/BluetoothPairingEvent#BluetoothPairingHandle
Attachment #8586592 - Flags: review?(mrbkap)
Attachment #8586592 - Flags: review?(mrbkap) → review+
No try server result since bluetooth2 won't be built.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/40f8ee7d5714
https://hg.mozilla.org/mozilla-central/rev/e8916b8e112b
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S10 (17apr)
Comment on attachment 8589435 [details] [diff] [review]
[Final] Bug 1132343 - Patch1: Add accept/reject method in BluetoothPairingHandle interface for supporting consent and displaypasskey pairing requests. r=shuang, r=mrbkap

Review of attachment 8589435 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth2/bluedroid/BluetoothServiceBluedroid.h
@@ +80,5 @@
>    RemoveDeviceInternal(const nsAString& aDeviceObjectPath,
>                         BluetoothReplyRunnable* aRunnable);
>  
>    virtual void
> +  PinReplyInternal(const nsAString& aDeviceAddress,

Doing this change was a mistake. It introduces new interfaces for no good reason, and it doesn't fix the BlueZ side.

In general, the mid-term goal for the backend code should be to get everything behind the same interfaces.
You need to log in before you can comment on or make changes to this bug.