Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Reporter
	Description • 11 years ago

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

	Kai-Chih Hu [:khu]
	Comment 1 • 11 years ago

Should it be koi+ as well? Does it block 917312 for sure?

	Bruce Huang [:bhuang] (inactive)
	Comment 2 • 11 years ago

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?

	Beatriz Rodríguez [:brg]
	Comment 3 • 11 years ago

(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.

	Ken Chang[:kenkai]
	Comment 4 • 11 years ago

I wonder if we could create a api to get the user input for this bug. Vincent, what do you think?

	José Antonio Olivera Ortega [:jaoo]
	Comment 5 • 11 years ago

(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

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Reporter
	Comment 6 • 11 years ago

I have another possible solution: add a permission for getting imsi, and provide an API in telephony.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 7 • 11 years ago

Maybe we should confirm with vyang ...

	Ken Chang[:kenkai]
	Comment 8 • 11 years ago

Yoshi, we also need your comments.

	Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]
	Comment 9 • 11 years ago

As jaoo raised in Bug 782603 before,
I don't see any reason we need to expose IMSI to Web.

	Ken Chang[:kenkai]
	Comment 10 • 11 years ago

Vicamo, is it fine to add a API in Mobile Connection for this bug?

	Vicamo Yang [:vicamo][:vyang]
	Comment 13 • 11 years ago

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

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Reporter
	Comment 15 • 11 years ago

Address comment 13.

	Vicamo Yang [:vicamo][:vyang]
	Comment 16 • 11 years ago

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.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Reporter
	Comment 17 • 11 years ago

(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.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Reporter
	Comment 18 • 11 years ago

Support serviceId introduced by DSDS.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Reporter
	Comment 19 • 11 years ago

Support serviceId introduced by DSDS.

	Vicamo Yang [:vicamo][:vyang]
	Comment 20 • 11 years ago

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.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Reporter
	Comment 21 • 11 years ago

Address comment 20.

	Gene Lian (I already quit Mozilla)
	Comment 22 • 11 years ago

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.

	Gene Lian (I already quit Mozilla)
	Comment 23 • 11 years ago

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.

	Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests] Reporter
	Comment 24 • 11 years ago

1. Define interface for wap push message object, per comment 22.
2. Change filename per comment 23.

	Ken Chang[:kenkai]
	Comment 25 • 11 years ago

Put this bug into backlog.

	Gene Lian (I already quit Mozilla)
	Comment 26 • 10 years ago

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.

	Vincent Chang[:vchang][changyihsin] Assignee
	Comment 27 • 10 years ago

I will take over to this bug. Free feel to take it back if you are still working on it.

	BMO Automation
	Comment 28 • 7 years ago

Firefox OS is not being worked on