Closed Bug 928775 Opened 7 years ago Closed 3 years ago

[WAP Push] Support WAP Provisioning Bootstrap Authentication for USERNETWPIN

Categories

(Firefox OS Graveyard :: RIL, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:-, b2g-v1.3T affected, b2g-v2.0 affected, b2g-v2.1 affected)

RESOLVED WONTFIX
blocking-b2g -
Tracking Status
b2g-v1.3T --- affected
b2g-v2.0 --- affected
b2g-v2.1 --- affected

People

(Reporter: chucklee, Assigned: vchang)

References

Details

(Whiteboard: permafail)

Attachments

(2 files, 6 obsolete files)

4.49 KB, patch
Details | Diff | Splinter Review
8.71 KB, patch
Details | Diff | Splinter Review
Follow up bug for supporting WAP Provisioning Bootstrap Authentication for USERNETWPIN [1].

[1] http://technical.openmobilealliance.org/tech/affiliates/wap/wap-184-provboot-20010314-a.pdf, Clause 6.2.1
Should it be koi+ as well? Does it block 917312 for sure?
This will currently be out of scope for 1.2 since the platform is not able to support it yet.  My understanding is there are some security concerns with exposing what's needed for this authentication?
(In reply to Bruce Huang (:bhuang) from comment #2)
> This will currently be out of scope for 1.2 since the platform is not able
> to support it yet.  
Thanks Bruce. Removing the blocker dependency accordingly.
No longer blocks: 917312
I wonder if we could create a api to get the user input for this bug. Vincent, what do you think?
Flags: needinfo?(vchang)
(In reply to Ken Chang from comment #4)
> I wonder if we could create a api to get the user input for this bug.
> Vincent, what do you think?

FYI we were already discussing about the possible solutions in comments:

https://bugzilla.mozilla.org/show_bug.cgi?id=914685#c5
https://bugzilla.mozilla.org/show_bug.cgi?id=914685#c6
https://bugzilla.mozilla.org/show_bug.cgi?id=914685#c8
https://bugzilla.mozilla.org/show_bug.cgi?id=914685#c9
I have another possible solution: add a permission for getting imsi, and provide an API in telephony.
Maybe we should confirm with vyang ...
Flags: needinfo?(vchang) → needinfo?(vyang)
Yoshi, we also need your comments.
Flags: needinfo?(allstars.chh)
As jaoo raised in Bug 782603 before,
I don't see any reason we need to expose IMSI to Web.
Flags: needinfo?(allstars.chh)
Vicamo, is it fine to add a API in Mobile Connection for this bug?
Flags: needinfo?(vyang)
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Comment on attachment 8351316 [details] [diff] [review]
WIP - 0002. Provide CP authentication from object function.

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

I think all the lines in RadioInterfaceLayer.js can be easily moved into WapPushManager.js.

::: dom/wappush/src/WapPushAuth.manifest
@@ +1,3 @@
>  component {1664b58d-9466-4d15-8682-3efd60bee65f} WapPushMessage.js
>  contract @mozilla.org/dom/system-messages/wrapper/wappush-received;1 {1664b58d-9466-4d15-8682-3efd60bee65f}
> +

nit: redundant empty line
Attachment #8351316 - Flags: feedback?(vyang)
Attachment #8351315 - Attachment is obsolete: true
Attachment #8351315 - Flags: feedback?(gene.lian)
Attachment #8355997 - Flags: review?(gene.lian)
Comment on attachment 8355998 [details] [diff] [review]
0002. Provide CP authentication from object function.

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

r=me, thank you :)

::: dom/wappush/src/gonk/WapPushManager.js
@@ +46,3 @@
>  
> +const WAP_PUSH_CHECK_MSG              = "WapPushMessage:Check";
> +const NS_XPCOM_SHUTDOWN_OBSERVER_ID   = "xpcom-shutdown";

nit: empty line after 'const NS_XPCOM...'

@@ +72,5 @@
> +      if (DEBUG) {
> +        debug("WAP Push message " + msg.name +
> +              " from a content process with no 'wappush' privileges.");
> +      }
> +      return false;

nit: we don't have synced messages here.  Returning null or void is okay.

@@ +236,5 @@
> +   *        }
> +   }
> +   */
> +  check: function WapPushManager_check(aAuthInfo) {
> +    let result = false;

nit: `result` is never referenced.

@@ +237,5 @@
> +   }
> +   */
> +  check: function WapPushManager_check(aAuthInfo) {
> +    let result = false;
> +    let key = "";

nit: assigned a value which is never used.

@@ +238,5 @@
> +   */
> +  check: function WapPushManager_check(aAuthInfo) {
> +    let result = false;
> +    let key = "";
> +    let imsi = CP.Authenticator.formatImsi(gRIL.getRadioInterface(0).rilContext.imsi);

Please file a follow-up bug for WAP Push handling under DSDS and place a TODO comment above.
Attachment #8355998 - Flags: review?(vyang) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] (PTO Dec. 21 ~ Jan. 5) from comment #16)
> Comment on attachment 8355998 [details] [diff] [review]
> 0002. Provide CP authentication from object function.
> 
> Review of attachment 8355998 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +238,5 @@
> > +   */
> > +  check: function WapPushManager_check(aAuthInfo) {
> > +    let result = false;
> > +    let key = "";
> > +    let imsi = CP.Authenticator.formatImsi(gRIL.getRadioInterface(0).rilContext.imsi);
> 
> Please file a follow-up bug for WAP Push handling under DSDS and place a
> TODO comment above.

Thanks for reviewing~

WAP Push support for DSDS is in bug 951999, I'll update the patch based on bug 951999 after it's landed.
Support serviceId introduced by DSDS.
Attachment #8355997 - Attachment is obsolete: true
Attachment #8355997 - Flags: review?(gene.lian)
Attachment #8356461 - Flags: review?(gene.lian)
Support serviceId introduced by DSDS.
Attachment #8355998 - Attachment is obsolete: true
Attachment #8356462 - Flags: review?(vyang)
Comment on attachment 8356462 [details] [diff] [review]
0002. Provide CP authentication from object function. V2

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

::: dom/wappush/src/gonk/WapPushManager.js
@@ +73,5 @@
> +      if (DEBUG) {
> +        debug("WAP Push message " + msg.name +
> +              " from a content process with no 'wappush' privileges.");
> +      }
> +      return null;

Sorry that I didn't make myself clear.  Please see http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIMessageManager.idl#152 .  You only need a return value when it's called with a synced message type.  Otherwise, just `return` will do.  So all the return statement in this function don't really need a return value now.

@@ +86,5 @@
> +        requestID: authInfo.requestID,
> +        result: result
> +      });
> +
> +      return result;

ditto, just `return;`.

@@ +89,5 @@
> +
> +      return result;
> +    }
> +
> +    return false;

ditto, simply omit this return statement.
Attachment #8356462 - Flags: review?(vyang) → review+
Just for the records, we'd better avoid exposing an object which is able to call some internal functions of IPC helper. This would casue security issues.
Comment on attachment 8356461 [details] [diff] [review]
0001. Add object wrapper for WAP Push system message. V2

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

Clear the review? for now due to comment #22.

::: dom/wappush/src/WapPushAuth.manifest
@@ +1,2 @@
> +component {1664b58d-9466-4d15-8682-3efd60bee65f} WapPushMessage.js
> +contract @mozilla.org/dom/system-messages/wrapper/wappush-received;1 {1664b58d-9466-4d15-8682-3efd60bee65f}

Can we make the file name WapPushAuth.manifest so something more generic? So that we can reasonably keep adding stuff in this manifest in the future. I'd suggest WapPush.manifest.
Attachment #8356461 - Flags: review?(gene.lian)
1. Define interface for wap push message object, per comment 22.
2. Change filename per comment 23.
Attachment #8356461 - Attachment is obsolete: true
Attachment #8362386 - Flags: review?(gene.lian)
Put this bug into backlog.
blocking-b2g: --- → backlog
Whiteboard: 1.3tarakorun2 → 1.3tarakorun2, [2.0-flame-test-run-1]
Comment on attachment 8362386 [details] [diff] [review]
0001. Add object wrapper for WAP Push system message. V3

Don't review at this moment until the feature is really needed.
Attachment #8362386 - Flags: review?(gene.lian)
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
Whiteboard: 1.3tarakorun2, [2.0-flame-test-run-1] → permafail
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(dharris)
I will take over to this bug. Free feel to take it back if you are still working on it.
Assignee: chuckli0706 → vchang
blocking-b2g: backlog → -
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.