Closed Bug 917312 Opened 7 years ago Closed 6 years ago

[WAP push][Gaia] Support handling client provisioning messages

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.2 affected)

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
b2g-v1.2 --- affected

People

(Reporter: noemi, Assigned: jaoo)

References

Details

Attachments

(9 files, 21 obsolete files)

2.77 KB, text/plain
Details
2.53 KB, text/plain
Details
5.72 KB, patch
Details | Diff | Splinter Review
30.08 KB, patch
Details | Diff | Splinter Review
23.01 KB, patch
Details | Diff | Splinter Review
8.55 KB, patch
Details | Diff | Splinter Review
19.48 KB, patch
Details | Diff | Splinter Review
46 bytes, text/x-github-pull-request
Details | Review
12.10 KB, patch
Details | Diff | Splinter Review
As an operator I want to be able to send settings to my customers via OTA through a client provisioning document

Acceptance Criteria:
•APPIDs support (*): w2 (Browser) and w4 (Multimedia Messaging Service)
•security Mechanisms support (SEC):
   o“NETWPIN” (value=0)
   o“USERPIN” (value=1)
   o“USERNETWPIN” (value=2)
   o“USERPINMAC” (value=3)
   oWithout any security mechanism, the device must ignore the client provisioning document sent
•only browser settings are received as part of the client provisioning document
•only mms settings are received as part of the client provisioning document
•browser & mms settings are received as part of the client provisioning document
•user confirmation flow: one click for accepting/declining the data sent
•non supported settings need to be ignored
•a new “default” profile will be created (profile info can’t be modified by the users)

(*): Elements and params “MUST HAVE” to be supported in the first phase (v1.2):
•characteristic type="PXLOGICAL"
    oPROXY-ID
    oNAME
•characteristic type="PORT"
    oPORTNBR
    oSERVICE
•characteristic type="PXPHYSICAL"
    oPHYSICAL-PROXY-ID
    oPXADDR
    oPXADDRTYPE
    oTO-NAPID
•characteristic type="NAPDEF"
    oNAPID
    oBEARER
    oNAME
    oNAP-ADDRESS
    oNAP-ADDRTYPE
•characteristic type="NAPAUTHINFO"
    oAUTHTYPE
    oAUTHNAME
    oAUTHSECRET
•characteristic type="APPLICATION"
    oAPPID
    oTO-PROXY
    oNAME
    oADDR
blocking-b2g: --- → koi?
Blocks: b2g-oma-cp
Assignee: nobody → josea.olivera
No longer blocks: b2g-oma-cp
Chuck, AFAIK (see [1]) It seems gecko doesn't provide the security information transported as parameters to the media type `application/vnd.wap.connectivity-wbxml`. The parameters MAC and SEC are used to authenticate the sender of the provisioning document and ensure its integrity. We should file a bug and provide those parameters, shouldn't we? If so feel free to file please. Thanks!

[1] http://mxr.mozilla.org/mozilla-central/source/dom/wappush/src/gonk/WapPushManager.js#106
Flags: needinfo?(chulee)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #3)
> Chuck, AFAIK (see [1]) It seems gecko doesn't provide the security
> information transported as parameters to the media type
> `application/vnd.wap.connectivity-wbxml`. The parameters MAC and SEC are
> used to authenticate the sender of the provisioning document and ensure its
> integrity. We should file a bug and provide those parameters, shouldn't we?
> If so feel free to file please. Thanks!
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/wappush/src/gonk/
> WapPushManager.js#106

I have filed Bug 914685, the idea is to include SEC, MAC and raw WBXML data into the system message.
So Gaia app can do the MAC check based on these info.
Flags: needinfo?(chulee)
I didn't realize we also would need the IMSI (Subscriber's ID) to authenticate the sender when using either NETWPIN or USERNETWPIN security mechanism. Gecko doesn't expose that ID (AFAIK) so we would need to expose it as well through an API (e.g. mozIccManager) for certified apps. Is there any security risk on exposing the subscriber's ID (IMSI)?

Feedback from Jonas and Antonio would be appreciate.
Flags: needinfo?(jonas)
Flags: needinfo?(amac)
Depends on: 914685
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #5)
> it as well through an API (e.g. mozIccManager) for certified apps. Is there
> any security risk on exposing the subscriber's ID (IMSI)?
> 
> Feedback from Jonas and Antonio would be appreciate.

This question had been discussed in bug 782603.
Seems the question was answered already!
Removing needinfo requests.
Flags: needinfo?(jonas)
Flags: needinfo?(amac)
https://vimeo.com/jaoo/cpfxos-netpin this video shows how the application looks right now and the flow when using NETPIN authentication mechanism. The NETPIN-based mechanism is performed by gecko and at the moment it is fake (work being done in bug 914685).
Depends on: 922177
Attached patch WIP v1 (obsolete) — Splinter Review
Gabriele, as we talk on IRC this is the WIP patch I'd need some feedback. At [1] there is the WIP branch where the code lives. Feel free to leave the comments wherever you prefer. I'll try to split the patch ASAP. Thanks.

[1] https://github.com/jaoo/gaia/tree/917312
Attachment #814964 - Flags: feedback?(gsvelto)
We are not adding a new app for client provisioning messages at all so I change the title of the bug. IMHO the old title could be confusing.
Summary: OMA Client Provisioning gaia application → [WAP push][Gaia] Support handling client provisioning messages
Gabriele, I just wanted to let you know I've split the patch into several parts. I hope this helps you to review the work easier. See [1] please. Do you prefer to have all the parts attached in this bug? Thanks.

[1] https://github.com/jaoo/gaia/tree/917312
Flags: needinfo?(gsvelto)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #14)
> Gabriele, I just wanted to let you know I've split the patch into several
> parts. I hope this helps you to review the work easier.

Excellent!

> Do you prefer to have all the parts attached in this bug?

Yes, that would be best as I prefer to have the reviews on Bugzilla.
Flags: needinfo?(gsvelto)
Attachment #814964 - Attachment is obsolete: true
Attachment #814964 - Flags: feedback?(gsvelto)
Attachment #815545 - Flags: feedback?(gsvelto)
Attached patch Part 2: Update UI. v1 (obsolete) — Splinter Review
Attachment #815546 - Flags: feedback?(gsvelto)
Attachment #815547 - Flags: feedback?(gsvelto)
Attached patch Part 5: Authenticate server. v1 (obsolete) — Splinter Review
Attachment #815549 - Flags: feedback?(gsvelto)
Attached patch Part 6: Tests. v1 (obsolete) — Splinter Review
Attachment #815551 - Flags: feedback?(gsvelto)
(In reply to Gabriele Svelto [:gsvelto] from comment #15)
> > Do you prefer to have all the parts attached in this bug?
> 
> Yes, that would be best as I prefer to have the reviews on Bugzilla.

Patches attached. Please remember this is still WIP and needs some improvements and add more unit test. The part that is still unfinished in the one related to the authentication of the sender. For now I would need some feedback so don't waste to much time doing a deep review. We will iterate as soon as I complete and improve the work. Thanks for your time.
Comment on attachment 815545 [details] [diff] [review]
Part 1: Handle client provisioing messages. v1

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

This is looking good save for the two small changes I've highlighted below.

::: apps/wappush/js/provisioning.js
@@ +13,5 @@
> +    obj.authInfo.checked = true;
> +    obj.authInfo.pass = true;
> +    return obj;
> +  }
> +};

Could you please refactor this to match the style we ended up using for the ParsedMessages? You would need to turn this into something like this:

(function(exports) {
  'use strict';

  function Provisioning(obj) {
    // If obj is present copy all fields from it
  }

  Provisioning.fromMessage = function p_fromMessage(message) {
    var obj = new Provisioning();
    // The rest is the same ...
  };

  exports.Provisioning = Provisioning;
})(window);

::: apps/wappush/js/wappush.js
@@ +133,1 @@
>      return true;

This switch looks a bit like overkill to me unless you're planning to add more entries to it. You could rewrite it as:

if (message.type !== 'text/vnd.wap.connectivity-xml' &&
    !WhiteList.has(message.sender)) {
  /* The message comes from a non white-listed MSISDN and isn't a 
   * provisioning message, ignore it. */
  return false;
}

Besides, is it OK for us to ignore whitelisting for provisioning messages? Doesn't this pose a security threat? I imagined that a carrier might want to whitelist one of its MSISDNs and then use that one and only that one to sent provisioning messages.
Attachment #815545 - Flags: feedback?(gsvelto) → feedback+
Comment on attachment 815546 [details] [diff] [review]
Part 2: Update UI. v1

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

This patch will need some extra work before we can land it. The basics are sounds but I'd like you to re-architect it to reduce redundant elements and make the code more streamlined. As a general remark there's quite a bit of duplication here (I've put some notes below) and I would prefer if you could make all the bits that share the same behavior unified.

Also for singletons I would prefer if you could use this style:

var Foo = (function (){
  var a, b, ...
  function foo_doSomeStuff() ...
  function foo_doSomeOtherStuff() ...

  return {
    doSomeStuff: foo_doSomeStuff,
    doSomeOtherStuff: foo_doSomeOtherStuff
  };
})

Rather than:

var Foo = {
  _a: ...,
  _b: ...,
  doSomeStuff: function foo_doSomeStuff() ... 
  doSomeOtherStuff: function foo_doSomeOtherStuff() ... 
}

This provides better encapsulation and allows you to get rid of all the references to 'this'. I know I've used the latter style in wappush.js but I'm moving away from it and I'll refactor that file soon. See messagedb.js for a practical example of that style for singletons.

::: apps/wappush/index.html
@@ +11,5 @@
>      <!-- Shared styles -->
>      <link rel="stylesheet" type="text/css" href="shared/style/headers.css">
>      <link rel="stylesheet" type="text/css" href="shared/style/status.css">
> +    <link rel="stylesheet" type="text/css" href="shared/style/confirm.css" >
> +    <link rel="stylesheet" type="text/css" href="shared/style/input_areas.css" >

Please put these in alphabetical order.

@@ +27,5 @@
>      <script defer src="js/messagedb.js"></script>
>      <script defer src="js/utils.js"></script>
>      <script defer src="js/parsed_message.js"></script>
> +    <script defer src="js/si_sl_screen_helper.js"></script>
> +    <script defer src="js/cp_screen_helper.js"></script>

Same as above, and fixed parsed_message.js too which I've put in the wrong place :-).

@@ +33,5 @@
>      <script defer src="js/whitelist.js"></script>
>    </head>
>  
>    <body role="application">
> +    <article role="main">

I think it would be better if you used two separate <article> tags here instead of two <section>s. Also rather than prefixing every id with si-sl-* use classes and selectors in the CSS code.

@@ +38,5 @@
> +
> +      <!-- SI SL message screen -->
> +      <section id="si-sl-screen" role="region">
> +	<header>
> +	  <button id="si-sl-close"><span class="icon icon-close"> close </span></button>

You're duplicating the headers here and the close button as well. It would be better if you shared the header and made the accept button visible/invisible depending on the mode you are in. This also forces you to duplicate the relevant code in the helper classes, it's better if we keep this shared as long as the behavior is the same.

@@ +69,5 @@
>        </section>
>      </article>
> +
> +    <!-- Client provisioning quit app confirm dialog -->
> +    <form id="cp-quit-app-confirm" role="dialog" data-type="confirm">

This should go in a separate <article> tag and the other <form>s below as well if they're to be shown individually. Also it would be better to give an id to each article and then replace the ids of the sub-elements with classes and use selectors to find them / style them.

::: apps/wappush/js/cp_screen_helper.js
@@ +55,5 @@
> +    // Let's consider the message is completely processed once the settings are
> +    // stored into the database.
> +    this._processed = false;
> +
> +    // Retrieve the various page elements

If you use a single top-level id for each section and then selectors this will look much better, for example:

this._container = document.getElementById('cp-container');
this._closeButton = this._container.querySelector('.close');
this._cancelQuitButton = this._container.querySelector('.cancel');

And so on...

@@ +158,5 @@
> +   * from the client provisioning screen.
> +   */
> +  onClose: function cpsh_onClose(evt) {
> +    if (this._processed) {
> +      window.setTimeout(window.close);

Use WapPushManager.close() here and in other places when you need to close the application. Remember that whenever the interaction is over you should close the app; I think you're already doing it on all outgoing paths but since I'm not able to test this yet I cannot verify it.

::: apps/wappush/style/wappush.css
@@ +53,5 @@
> +  margin: 0 0 2rem;
> +  font-size: 1.5rem;
> +  line-height: 1.8rem;
> +}
> +

Please remove the classes, use the hidden attribute in the .html file for elements that should be hidden by default and then use xyz.style.visibility = 'visible' to display them.
Attachment #815546 - Flags: feedback?(gsvelto) → feedback-
Comment on attachment 815547 [details] [diff] [review]
Part 3: Parse WAP provisioning doc.

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

This patch will need some serious refactoring, on the bright side it can be made much smaller by using some helper functions. Please see my comments below.

::: apps/wappush/js/doc.js
@@ +2,5 @@
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +
> +'use strict';
> +
> +const CONTENT_TYPE = 'application/xml';

Please don't use const yet as we don't have proper ECMAScript 6 support and the current behavior is different than what the standard will require IIRC.

@@ +16,5 @@
> +
> +/**
> + * Provisioning document helper. Parses and takes out the APN(s) parameters.
> + */
> +var ProvisioningDoc = {

This is used as an object and not as a singleton if I'm understanding it correctly. It would be better if you could use the style I use for the ParsedMessage object with a specific prototype and factory methods for creating objects.

@@ +30,5 @@
> +  apnsReady: false,
> +  /** Names are ready to use (flag) */
> +  namesReady: false,
> +
> +  init: function sp_init(document) {

Your init function is very large, you should consider splitting it up into multiple logical parts to make it easier to read and understand. Also please provide a comment for all functions you define; for example it's hard to figure out what getNAPDEF() is for without knowing what a NAPDEF is :-). It also looks to me that this is more a factory function to create objects rather than the initialization of a singleton so the name is a bit misleading. Additionally please don't use 'document' as a parameter or variable name as it overlaps the global document variable. Use doc or provisioningDoc instead.

@@ +37,5 @@
> +    if (!this.document) {
> +      return;
> +    }
> +
> +    function getNAPDEF(TO_NAPID, NAPDEF) {

Please don't use all-caps parameter or function names, even when they're abbreviations as we're already using them for defining constants. I would change this to getNapDef(toNapId, napDef) for example.

@@ +38,5 @@
> +      return;
> +    }
> +
> +    function getNAPDEF(TO_NAPID, NAPDEF) {
> +      for (var i = 0; i < NAPDEF.length; i++) {

Looking at this function and the ones below I wonder if you're navigating over DOM nodes or not. If you are you could use querySelector/querySelectorAll instead to find the relevant nodes; it would require a fraction of the code. Also instead of navigating through attributes you should use element.hasAttribute() / element.getAttribute(). In any case the code would be cleaner and easier to understand if you have helper function for every type of node you need to look for. For example:

functiongetNapidFromNapdef(...) {
  // Do your lookup here, return null if nothing's found
}

NAPDEFTObject['NAPID'] = getNapidFromNapdef(...);

Is much more readable than:

if (NAPDEF.childNodes[i].attributes['name'] &&
  NAPDEF.childNodes[i].attributes['name'].value &&
  (NAPDEF.childNodes[i].attributes['name'].value === 'NAPID')) {
  NAPDEFTObject['NAPID'] = NAPDEF.childNodes[i].attributes['value'].value;
}
Attachment #815547 - Flags: feedback?(gsvelto) → feedback-
Comment on attachment 815548 [details] [diff] [review]
Part 4: Store new APNs into the settings  database. v1

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

This is looking good but requires some extra documentation to clarify what's going on in the callbacks you're invoking.

::: apps/wappush/js/store.js
@@ +4,5 @@
> +'use strict';
> +
> +const CP_APN_KEY = 'ril.data.cp.apns';
> +const MCC_KEY = 'operatorvariant.mcc';
> +const MNC_KEY = 'operatorvariant.mnc';

Please don't use const, see my comment in the previous patch.

@@ +9,5 @@
> +
> +var StoreProvisioning = {
> +  _mccMncCodes: { mcc: '000', mnc: '00' },
> +
> +  getMccMncCodes: function sp_getMccMncCodes(callback) {

Please document what this function does and what parameters the callback function accepts.

@@ +31,5 @@
> +      };
> +    };
> +  },
> +
> +  provision: function sp_provision(parameters, callback) {

As above please document the callback behavior since you're calling it for both success and error conditions.
Attachment #815548 - Flags: feedback?(gsvelto) → feedback+
Comment on attachment 815549 [details] [diff] [review]
Part 5: Authenticate server. v1

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

Looking good but please use the singleton style I've described above for the ProvisioningAuthentication object.
Attachment #815549 - Flags: feedback?(gsvelto) → feedback+
Comment on attachment 815551 [details] [diff] [review]
Part 6: Tests. v1

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

There's not much in the way of new tests here so I would suggest two things:

- Do not split the tests out but put them in the patch where they're relevant, this will help you in making the changes self-contained (otherwise you wouldn't be able to test them easily)
- Do not add trivial checks, only focus on functionality and correctness

See my notes below for a few more detailed comments.

::: apps/wappush/test/unit/auth_test.js
@@ +6,5 @@
> +const USER_PIN_AUTH_INFO = {};
> +
> +suite('auth >', function() {
> +  suite('Bootstrap process: Security mechanisms', function() {
> +    test('ProvisioningAuthentication.isDocumentValid is a function',

Unless you intend to dynamically change isDocumentValid this test is not necessary.

@@ +11,5 @@
> +      function() {
> +        assert.isFunction(ProvisioningAuthentication.isDocumentValid);
> +    });
> +
> +    test('Unknow security mechanism', function() {

You should add more tests covering the different forms of authentication.

::: apps/wappush/test/unit/doc_test.js
@@ +5,5 @@
> +suite('doc >', function() {
> +
> +  suite('Parser an empty provisioning document', function() {
> +    setup(function() {
> +      ProvisioningDoc.init('');

Testing the empty document is not very interesting, you should be testing some real document.

::: apps/wappush/test/unit/store_test.js
@@ +19,5 @@
> +  });
> +
> +  suite('StoreProvisioning', function() {
> +    test('StoreProvisioning.getMccMncCodes is a function', function() {
> +      assert.isFunction(StoreProvisioning.getMccMncCodes);

These tests are not very interesting, remove them and leave the functionality checks instead.
Attachment #815551 - Flags: feedback?(gsvelto) → feedback-
Andrew,

Please follow up.
Flags: needinfo?(overholt)
I am under the impression WAP Push has been committed for 1.2 so koi+.  Please re-nom if I'm incorrect.
blocking-b2g: koi? → koi+
Flags: needinfo?(overholt)
(In reply to Andrew Overholt [:overholt] from comment #30)
> I am under the impression WAP Push has been committed for 1.2 so koi+. 
> Please re-nom if I'm incorrect.

We should really find out first if this is specifically a must have user story as part of WAP Push.

Can product find out if this is a must have for 1.2?
Flags: needinfo?(ffos-product)
Flags: needinfo?(ffos-product) → needinfo?(bhuang)
(In reply to Gabriele Svelto [:gsvelto] from comment #23)
> Comment on attachment 815545 [details] [diff] [review]
> Part 1: Handle client provisioing messages. v1
> 
> Review of attachment 815545 [details] [diff] [review]:

Thanks for the feedback.

> Could you please refactor this to match the style we ended up using for the
> ParsedMessages? You would need to turn this into something like this:
> 
> (function(exports) {
>   'use strict';
> 
>   function Provisioning(obj) {
>     // If obj is present copy all fields from it
>   }
> 
>   Provisioning.fromMessage = function p_fromMessage(message) {
>     var obj = new Provisioning();
>     // The rest is the same ...
>   };
> 
>   exports.Provisioning = Provisioning;
> })(window);

Done.

> ::: apps/wappush/js/wappush.js
> @@ +133,1 @@
> >      return true;
> 
> This switch looks a bit like overkill to me unless you're planning to add
> more entries to it.

I kept the switch statement since I introduced an additional checking. Let me know if it makes sense to do the change you proposed.
Attachment #815545 - Attachment is obsolete: true
Attachment #818260 - Flags: feedback?(gsvelto)
Attached patch Part 2: Update UI. v2 (obsolete) — Splinter Review
(In reply to Gabriele Svelto [:gsvelto] from comment #24)
> Comment on attachment 815546 [details] [diff] [review]
> Part 2: Update UI. v1
> 
> Review of attachment 815546 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the feedback.

> Also for singletons I would prefer if you could use this style:
> 
> var Foo = (function (){
>   var a, b, ...
>   function foo_doSomeStuff() ...
>   function foo_doSomeOtherStuff() ...
> 
>   return {
>     doSomeStuff: foo_doSomeStuff,
>     doSomeOtherStuff: foo_doSomeOtherStuff
>   };
> })
> 
> Rather than:
> 
> var Foo = {
>   _a: ...,
>   _b: ...,
>   doSomeStuff: function foo_doSomeStuff() ... 
>   doSomeOtherStuff: function foo_doSomeOtherStuff() ... 
> }

Done.

> This should go in a separate <article> tag and the other <form>s below as
> well if they're to be shown individually.

I followed the approach seen in other different apps, the confirm dialogs don't go in <articles>s. May I know why you are suggesting this change? I kept what I had. Let me know if you still what the change please.
 
> Please remove the classes, use the hidden attribute in the .html file for
> elements that should be hidden by default and then use xyz.style.visibility
> = 'visible' to display them.

Done, more or less. Class are deleted and not used anymore. I ended up using `obj.hidden = false`. What you suggested didn't work.
Attachment #815546 - Attachment is obsolete: true
Attachment #818261 - Flags: feedback?(gsvelto)
Comment on attachment 815551 [details] [diff] [review]
Part 6: Tests. v1

(In reply to Gabriele Svelto [:gsvelto] from comment #28)
> Comment on attachment 815551 [details] [diff] [review]
> Part 6: Tests. v1
> 
> Review of attachment 815551 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the feedback.

> There's not much in the way of new tests here so I would suggest two things:
> 
> - Do not split the tests out but put them in the patch where they're
> relevant, this will help you in making the changes self-contained (otherwise
> you wouldn't be able to test them easily)

Will do.

> - Do not add trivial checks, only focus on functionality and correctness

The new test were trivial and didn't add much. There were only the beginning. I'll add more test focused on functionality and correctness.

I'll deleted this patch since tests will be added in the patch where they're relevant.
Attachment #815551 - Attachment is obsolete: true
Comment on attachment 818261 [details] [diff] [review]
Part 2: Update UI. v2

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

Looking much better overall! I've got just a few nits you can find below.

::: apps/wappush/index.html
@@ +39,5 @@
> +	<button id="close"><span class="icon icon-close"> close </span></button>
> +	<menu type="toolbar">
> +	  <button id="accept" data-l10n-id="accept" class="full recommend" hidden>Accept</button>
> +	</menu>
> +	<h1 id="title"> tittle </h1>

typo, tittle ===> title

::: apps/wappush/js/cp_screen_helper.js
@@ +112,5 @@
> +  /**
> +   * Makes the client provisioning screen visible and populate it.
> +   */
> +  function cpsh_populateScreen(message) {
> +    screen.classList.add('show');

Use the |visibility| attribute for this like you did for everything else.

::: apps/wappush/js/parsed_message.js
@@ +81,4 @@
>     *                  separate fields.
>     */
>    ParsedMessage.from = function pm_from(message, timestamp) {
> +    var _ = navigator.mozL10n.get;

It's not a good idea to have this here...

@@ +111,4 @@
>      } else if (message.contentType === 'text/vnd.wap.connectivity-xml') {
>        // Client provisioning (CP) message
>        obj.provisioning = Provisioning.fromMessage(message);
> +      obj.text = _('cp-message-received');

... I'd say it would be better to store 'cp-message-received' in the message object and then localize it only when you need to display it.

::: apps/wappush/js/si_sl_screen_helper.js
@@ +45,5 @@
> +    // flow differs. Let's add the listener once we know the message is a si sl
> +    // one.
> +    closeButton.addEventListener('click', sssh_onClose);
> +
> +    screen.classList.add('show');

Use the |visibility| attribute.

::: apps/wappush/style/wappush.css
@@ +16,5 @@
>    height: 100%;
>    background: white;
>    color: black;
>    z-index: 1;
> +  display: none;

This is not needed if all the main views are hidden by default in the HTML code.

@@ +21,5 @@
>  }
>  
> +[role=main].show {
> +  display: block;
> +}

Remove this once you'll have converted everything to use the |visibility| attribute.

@@ +36,5 @@
>    word-wrap: break-word;
>  }
> +
> +#cp-screen .container .message {
> +  color: #000;

Isn't this redundant?

::: apps/wappush/test/unit/wappush_test.js
@@ +7,5 @@
>  requireApp('wappush/shared/test/unit/mocks/mock_notification_helper.js');
>  requireApp('wappush/shared/test/unit/mocks/mock_navigator_moz_settings.js');
>  
> +requireApp('wappush/js/si_sl_screen_helper.js');
> +requireApp('wappush/js/cp_screen_helper.js');

Please create new tests for the .populateScreen() calls as replacements for the third test you removed (the first two weren't very interesting so you can leave them out).
Attachment #818261 - Flags: feedback?(gsvelto) → feedback+
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #33)
> I followed the approach seen in other different apps, the confirm dialogs
> don't go in <articles>s. May I know why you are suggesting this change? I
> kept what I had. Let me know if you still what the change please.

No, if other apps are doing the same then keep it as it is. I was just not aware of it.

> Done, more or less. Class are deleted and not used anymore. I ended up using
> `obj.hidden = false`. What you suggested didn't work.

Yes, that was a mistake on my part, `obj.hidden = false / true` is the way to go. I'll give you feedback on this ASAP.
Comment on attachment 818260 [details] [diff] [review]
Part 1: Accept client provisioing messages. v2

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

Looking good save for a small mistake I highlighted below. The other bit you'll need in this patch is to modify ParsedMessage.toJSON() to include the provisioning message if present so that it's stored correctly in the database after the message has been received.

::: apps/wappush/js/provisioning.js
@@ +11,5 @@
> +    }
> +  };
> +
> +  Provisioning.fromMessage = function p_fromMessage(message) {
> +    var obj = new Provisioning(message);

This should be:

var obj = new Provisioning();
Attachment #818260 - Flags: feedback?(gsvelto) → feedback+
(In reply to Gabriele Svelto [:gsvelto] from comment #25)
> Comment on attachment 815547 [details] [diff] [review]
> Part 3: Parse WAP provisioning doc.
> 
> Review of attachment 815547 [details] [diff] [review]:

Thanks for the previous review.

Since this part needed more work I've addressed the most important comments. Next is to add tests but I would like to receive some feedback on this patch first and add the test in the mean time.

> @@ +16,5 @@
> > +
> > +/**
> > + * Provisioning document helper. Parses and takes out the APN(s) parameters.
> > + */
> > +var ProvisioningDoc = {
> 
> This is used as an object and not as a singleton if I'm understanding it
> correctly. It would be better if you could use the style I use for the
> ParsedMessage object with a specific prototype and factory methods for
> creating objects.

You were right, new patch uses an object as you suggested.

> @@ +30,5 @@
> > +  apnsReady: false,
> > +  /** Names are ready to use (flag) */
> > +  namesReady: false,
> > +
> > +  init: function sp_init(document) {
> 
> Your init function is very large, you should consider splitting it up into
> multiple logical parts to make it easier to read and understand.

I've tried, please have a look.

> Also please
> provide a comment for all functions you define; for example it's hard to
> figure out what getNAPDEF() is for without knowing what a NAPDEF is :-).

Added more info along the code. It's difficult to explain everything as the terms come from OMA specs (http://technical.openmobilealliance.org/Technical/release_program/cp_v1_1.aspx).

> @@ +38,5 @@
> > +      return;
> > +    }
> > +
> > +    function getNAPDEF(TO_NAPID, NAPDEF) {
> > +      for (var i = 0; i < NAPDEF.length; i++) {
> 
> Looking at this function and the ones below I wonder if you're navigating
> over DOM nodes or not. If you are you could use
> querySelector/querySelectorAll instead to find the relevant nodes; it would
> require a fraction of the code. Also instead of navigating through
> attributes you should use element.hasAttribute() / element.getAttribute().

Done I was not aware how much powerful those functions are. Thanks!
Attachment #815547 - Attachment is obsolete: true
Comment on attachment 819081 [details] [diff] [review]
Part 3: Parse WAP provisioning doc. v2

Oops forgot to resquest feedback :(.
Attachment #819081 - Flags: feedback?(gsvelto)
Due to this being a late breaking feature, let's land this first on Mozilla Central and do a QA test pass to assess risk. Marking NO_UPLIFT to indicate that we should not uplift this until we do a risk assessment on the implementation to assess feature & regression risk.
Whiteboard: [NO_UPLIFT]
Depends on: 928775
(In reply to Gabriele Svelto [:gsvelto] from comment #37)
> Comment on attachment 818260 [details] [diff] [review]
> Part 1: Accept client provisioing messages. v2
> 
> Review of attachment 818260 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the previous feedback.

> Looking good save for a small mistake I highlighted below. The other bit
> you'll need in this patch is to modify ParsedMessage.toJSON() to include the
> provisioning message if present so that it's stored correctly in the
> database after the message has been received.
> 
> ::: apps/wappush/js/provisioning.js
> @@ +11,5 @@
> > +    }
> > +  };
> > +
> > +  Provisioning.fromMessage = function p_fromMessage(message) {
> > +    var obj = new Provisioning(message);
> 
> This should be:
> 
> var obj = new Provisioning();

Done.

Now is time for review, would you mind? ;) Thanks!
Attachment #818260 - Attachment is obsolete: true
Attachment #819827 - Flags: review?(gsvelto)
Attached patch Part 2: Update UI. v3 (obsolete) — Splinter Review
(In reply to Gabriele Svelto [:gsvelto] from comment #35)
> Comment on attachment 818261 [details] [diff] [review]
> Part 2: Update UI. v2
> 
> Review of attachment 818261 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the previous feedback.

> ::: apps/wappush/js/cp_screen_helper.js
> @@ +112,5 @@
> > +  /**
> > +   * Makes the client provisioning screen visible and populate it.
> > +   */
> > +  function cpsh_populateScreen(message) {
> > +    screen.classList.add('show');
> 
> Use the |visibility| attribute for this like you did for everything else.

This didn't worked for me. I kept the class.

> ::: apps/wappush/js/parsed_message.js
> @@ +81,4 @@
> >     *                  separate fields.
> >     */
> >    ParsedMessage.from = function pm_from(message, timestamp) {
> > +    var _ = navigator.mozL10n.get;
> 
> It's not a good idea to have this here...
> 
> @@ +111,4 @@
> >      } else if (message.contentType === 'text/vnd.wap.connectivity-xml') {
> >        // Client provisioning (CP) message
> >        obj.provisioning = Provisioning.fromMessage(message);
> > +      obj.text = _('cp-message-received');
> 
> ... I'd say it would be better to store 'cp-message-received' in the message
> object and then localize it only when you need to display it.

Done.

> ::: apps/wappush/test/unit/wappush_test.js
> @@ +7,5 @@
> >  requireApp('wappush/shared/test/unit/mocks/mock_notification_helper.js');
> >  requireApp('wappush/shared/test/unit/mocks/mock_navigator_moz_settings.js');
> >  
> > +requireApp('wappush/js/si_sl_screen_helper.js');
> > +requireApp('wappush/js/cp_screen_helper.js');
> 
> Please create new tests for the .populateScreen() calls as replacements for
> the third test you removed (the first two weren't very interesting so you
> can leave them out).

New test added for what you requested.

Now is time for the review, would you mind? Thanks!
Attachment #818261 - Attachment is obsolete: true
Attachment #819829 - Flags: review?(gsvelto)
(In reply to Gabriele Svelto [:gsvelto] from comment #26)
> Comment on attachment 815548 [details] [diff] [review]
> Part 4: Store new APNs into the settings  database. v1
> 
> Review of attachment 815548 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the previous feedback.

> This is looking good but requires some extra documentation to clarify what's
> going on in the callbacks you're invoking.

I've added some documentation on the functions. The callback function is just for unit testing purposes. 

Now it's time for review, would you mind please? Thanks!
Attachment #815548 - Attachment is obsolete: true
Attachment #819981 - Flags: review?(gsvelto)
Attached patch Part 5: Authenticate server. v2 (obsolete) — Splinter Review
(In reply to Gabriele Svelto [:gsvelto] from comment #27)
> Comment on attachment 815549 [details] [diff] [review]
> Part 5: Authenticate server. v1
> 
> Review of attachment 815549 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the previous feedback.

> Looking good but please use the singleton style I've described above for the
> ProvisioningAuthentication object.

Done. I've also added some new unit tests.

Now it's time for review, would you mind please? Thanks!
Attachment #815549 - Attachment is obsolete: true
Attachment #820224 - Flags: review?(gsvelto)
No longer depends on: 928775
Instead of requesting feedback at you on the v2 patch of this part I'll change to request review at you on the latest version I have.

Find bellow the discussion about this patch. Thanks!

> (In reply to Gabriele Svelto [:gsvelto] from comment #25)
> > Comment on attachment 815547 [details] [diff] [review]
> > Part 3: Parse WAP provisioning doc.
> > 
> > Review of attachment 815547 [details] [diff] [review]:
> 
> Thanks for the previous review.
> 
> Since this part needed more work I've addressed the most important comments.
> Next is to add tests but I would like to receive some feedback on this patch
> first and add the test in the mean time.
> 
> > @@ +16,5 @@
> > > +
> > > +/**
> > > + * Provisioning document helper. Parses and takes out the APN(s) parameters.
> > > + */
> > > +var ProvisioningDoc = {
> > 
> > This is used as an object and not as a singleton if I'm understanding it
> > correctly. It would be better if you could use the style I use for the
> > ParsedMessage object with a specific prototype and factory methods for
> > creating objects.
> 
> You were right, new patch uses an object as you suggested.
> 
> > @@ +30,5 @@
> > > +  apnsReady: false,
> > > +  /** Names are ready to use (flag) */
> > > +  namesReady: false,
> > > +
> > > +  init: function sp_init(document) {
> > 
> > Your init function is very large, you should consider splitting it up into
> > multiple logical parts to make it easier to read and understand.
> 
> I've tried, please have a look.
> 
> > Also please
> > provide a comment for all functions you define; for example it's hard to
> > figure out what getNAPDEF() is for without knowing what a NAPDEF is :-).
> 
> Added more info along the code. It's difficult to explain everything as the
> terms come from OMA specs
> (http://technical.openmobilealliance.org/Technical/release_program/cp_v1_1.
> aspx).
> 
> > @@ +38,5 @@
> > > +      return;
> > > +    }
> > > +
> > > +    function getNAPDEF(TO_NAPID, NAPDEF) {
> > > +      for (var i = 0; i < NAPDEF.length; i++) {
> > 
> > Looking at this function and the ones below I wonder if you're navigating
> > over DOM nodes or not. If you are you could use
> > querySelector/querySelectorAll instead to find the relevant nodes; it would
> > require a fraction of the code. Also instead of navigating through
> > attributes you should use element.hasAttribute() / element.getAttribute().
> 
> Done I was not aware how much powerful those functions are. Thanks!
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #38)
> Created attachment 819081 [details] [diff] [review]
> Part 3: Parse WAP provisioning doc. v2
> 
> (In reply to Gabriele Svelto [:gsvelto] from comment #25)
> > Comment on attachment 815547 [details] [diff] [review]
> > Part 3: Parse WAP provisioning doc.
> > 
> > Review of attachment 815547 [details] [diff] [review]:
> 
> Thanks for the previous review.
> 
> Since this part needed more work I've addressed the most important comments.
> Next is to add tests but I would like to receive some feedback on this patch
> first and add the test in the mean time.
> 
> > @@ +16,5 @@
> > > +
> > > +/**
> > > + * Provisioning document helper. Parses and takes out the APN(s) parameters.
> > > + */
> > > +var ProvisioningDoc = {
> > 
> > This is used as an object and not as a singleton if I'm understanding it
> > correctly. It would be better if you could use the style I use for the
> > ParsedMessage object with a specific prototype and factory methods for
> > creating objects.
> 
> You were right, new patch uses an object as you suggested.
> 
> > @@ +30,5 @@
> > > +  apnsReady: false,
> > > +  /** Names are ready to use (flag) */
> > > +  namesReady: false,
> > > +
> > > +  init: function sp_init(document) {
> > 
> > Your init function is very large, you should consider splitting it up into
> > multiple logical parts to make it easier to read and understand.
> 
> I've tried, please have a look.
> 
> > Also please
> > provide a comment for all functions you define; for example it's hard to
> > figure out what getNAPDEF() is for without knowing what a NAPDEF is :-).
> 
> Added more info along the code. It's difficult to explain everything as the
> terms come from OMA specs
> (http://technical.openmobilealliance.org/Technical/release_program/cp_v1_1.
> aspx).
> 
> > @@ +38,5 @@
> > > +      return;
> > > +    }
> > > +
> > > +    function getNAPDEF(TO_NAPID, NAPDEF) {
> > > +      for (var i = 0; i < NAPDEF.length; i++) {
> > 
> > Looking at this function and the ones below I wonder if you're navigating
> > over DOM nodes or not. If you are you could use
> > querySelector/querySelectorAll instead to find the relevant nodes; it would
> > require a fraction of the code. Also instead of navigating through
> > attributes you should use element.hasAttribute() / element.getAttribute().
> 
> Done I was not aware how much powerful those functions are. Thanks!
Attachment #819081 - Attachment is obsolete: true
Attachment #819081 - Flags: feedback?(gsvelto)
Attachment #821549 - Flags: review?(gsvelto)
Comment on attachment 819827 [details] [diff] [review]
Part 1: Accept client provisioing messages. v3

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

Looking good, r=me with the change I've described below.

::: apps/wappush/js/wappush.js
@@ +129,5 @@
> +        /* Security information is optional but the device must discard any
> +           message with no security information. */
> +        if (!message.provisioning.authInfo) {
> +          return false;
> +        }

Could you move this into ParsedMessage.from()? Basically if the message doesn't have the authInfo field consider the whole message invalid and return null instead of an object then we won't need this extra check here.

BTW have you verified if the MSISDN whitelist really doesn't apply to these mesages?
Attachment #819827 - Flags: review?(gsvelto) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #46)
> Comment on attachment 819827 [details] [diff] [review]
> Part 1: Accept client provisioing messages. v3
> 
> Review of attachment 819827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good, r=me with the change I've described below.

Thanks for the review.

> ::: apps/wappush/js/wappush.js
> @@ +129,5 @@
> > +        /* Security information is optional but the device must discard any
> > +           message with no security information. */
> > +        if (!message.provisioning.authInfo) {
> > +          return false;
> > +        }
> 
> Could you move this into ParsedMessage.from()? Basically if the message
> doesn't have the authInfo field consider the whole message invalid and
> return null instead of an object then we won't need this extra check here.

Done.

> BTW have you verified if the MSISDN whitelist really doesn't apply to these
> mesages?

Yes, It seems there is no such requeriment for CP. We figured out that after having a look at the certification documents we have.
Attachment #819827 - Attachment is obsolete: true
Comment on attachment 819829 [details] [diff] [review]
Part 2: Update UI. v3

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

This is looking very good; there's still a few things I'd like to see addressed and then I think we'll be good to go. The tests look good to me BTW.

::: apps/wappush/js/cp_screen_helper.js
@@ +112,5 @@
> +  /**
> +   * Makes the client provisioning screen visible and populate it.
> +   */
> +  function cpsh_populateScreen(message) {
> +    screen.classList.add('show');

You mentioned that using the visibility attribute didn't work but did you try with the hidden attribute?

<article id="cp-screen" role="main" hidden> and then using |screen.hidden = false| should work. The same applies for the SI/SL screen.

@@ +117,5 @@
> +
> +    // The close button in the header is shared between screens but sadly the
> +    // flow differs. Let's add the listener once we know the message is a client
> +    // provisioning one.
> +    closeButton.addEventListener('click', cpsh_onClose);

There's a problem here in that if we populate the screen multiple times we'll be piling up event listeners while this should be done only once. Since this button is shared with the SI/SL view you might want to add an event listener in wappush.js when the app is created which inokes a callback stored in the WapPushManager singleton. Whenever you enter a different mode (CP or SI/SL) set that callback to the appropriate behavior:

var WapPushManager = {
  onCloseCallback: null;

  onClose: function wpm_onClose() {
    if (onCloseCallback && (typeof onCloseCallback === function)) {
      onCloseCallback();
    }

    WapPushManager.close();
  }

  setOnCloseCallback: function wpm_setOnCloseCallback(callback) {
    this.onCloseCallback = callback;
  }
}

...

function cpsh_populateScreen(message) {
  WapPushManager.setOnCloseCallback(cpsh_onClose);
  ...
}

::: apps/wappush/js/si_sl_screen_helper.js
@@ +43,5 @@
> +  function sssh_populateScreen(message) {
> +    // The close button in the header is shared between screens but sadly the
> +    // flow differs. Let's add the listener once we know the message is a si sl
> +    // one.
> +    closeButton.addEventListener('click', sssh_onClose);

Same as above.

@@ +45,5 @@
> +    // flow differs. Let's add the listener once we know the message is a si sl
> +    // one.
> +    closeButton.addEventListener('click', sssh_onClose);
> +
> +    screen.classList.add('show');

Same as above with screen.hidden = false;

::: apps/wappush/js/wappush.js
@@ +135,5 @@
>            var iconURL = NotificationHelper.getIconURI(app) +
>                         '?timestamp=' + encodeURIComponent(message.timestamp);
> +
> +          message.text = (message.type == 'text/vnd.wap.connectivity-xml') ?
> +                         _(message.text) : message.text;

Move this into CpScreenHelper.populateScreen(), it makes more sense there.

::: apps/wappush/locales/wappush.en-US.properties
@@ +2,5 @@
> +quit             = Quit
> +cancel           = Cancel
> +store            = Store
> +try-again        = Try again
> +finish           = Finish

We're missing a generic description for the 'title' identifier in the SI/SL screen, this used to be:

wap-push-message = WAP Push message

It's OK if you turn it into

title = WAP Push message

::: apps/wappush/test/unit/wappush_test.js
@@ +192,5 @@
> +
> +  suite('receiving and displaying a CP message', function() {
> +    var message;
> +
> +     // UI elements

Nit, 4 spaces of indentation.
Attachment #819829 - Flags: review?(gsvelto)
Comment on attachment 821580 [details] [diff] [review]
Part 1: Accept client provisioing messages. r=gsvelto v4

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

::: apps/wappush/js/parsed_message.js
@@ +109,5 @@
>        obj.href = slNode.getAttribute('href');
> +    } else if (message.contentType === 'text/vnd.wap.connectivity-xml') {
> +      // Client provisioning (CP) message
> +      obj.provisioning = Provisioning.fromMessage(message);
> +      // Security information is optional. The device must discard any message

I imagine you meant that security information is mandatory, not optional. Otherwise it doesn't make sense to refuse messages that do not have it :-p
Comment on attachment 821549 [details] [diff] [review]
Part 3: Parse WAP provisioning doc. v3

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

Looking good!
Attachment #821549 - Flags: review?(gsvelto) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #49)
> Comment on attachment 821580 [details] [diff] [review]
> Part 1: Accept client provisioing messages. r=gsvelto v4
> 
> Review of attachment 821580 [details] [diff] [review]:
> -----------------------------------------------------------------

> I imagine you meant that security information is mandatory, not optional.
> Otherwise it doesn't make sense to refuse messages that do not have it :-p

Yes, you are right. I'll change the comment. Thanks.
Comment on attachment 819981 [details] [diff] [review]
Part 4: Store new APNs into the settings  database. v2

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

Also looking very good!
Attachment #819981 - Flags: review?(gsvelto) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #49)
> Comment on attachment 821580 [details] [diff] [review]
> Part 1: Accept client provisioing messages. r=gsvelto v4
> 
> Review of attachment 821580 [details] [diff] [review]:
> -----------------------------------------------------------------

> I imagine you meant that security information is mandatory, not optional.
> Otherwise it doesn't make sense to refuse messages that do not have it :-p

Comment updated. Thanks.
Attachment #821580 - Attachment is obsolete: true
Attached patch Part 2: Update UI. v4 (obsolete) — Splinter Review
(In reply to Gabriele Svelto [:gsvelto] from comment #48)
> Comment on attachment 819829 [details] [diff] [review]
> Part 2: Update UI. v3
> 
> Review of attachment 819829 [details] [diff] [review]:
> -----------------------------------------------------------------

> ::: apps/wappush/js/cp_screen_helper.js
> @@ +112,5 @@
> > +  /**
> > +   * Makes the client provisioning screen visible and populate it.
> > +   */
> > +  function cpsh_populateScreen(message) {
> > +    screen.classList.add('show');
> 
> You mentioned that using the visibility attribute didn't work but did you
> try with the hidden attribute?

No, I didn't. I've tried with the hidden attribute and It works. Changed.
 
> @@ +117,5 @@
> > +
> > +    // The close button in the header is shared between screens but sadly the
> > +    // flow differs. Let's add the listener once we know the message is a client
> > +    // provisioning one.
> > +    closeButton.addEventListener('click', cpsh_onClose);
> 
> There's a problem here in that if we populate the screen multiple times
> we'll be piling up event listeners while this should be done only once.
> Since this button is shared with the SI/SL view you might want to add an
> event listener in wappush.js when the app is created which inokes a callback
> stored in the WapPushManager singleton. Whenever you enter a different mode
> (CP or SI/SL) set that callback to the appropriate behavior:
> 
> var WapPushManager = {
>   onCloseCallback: null;
> 
>   onClose: function wpm_onClose() {
>     if (onCloseCallback && (typeof onCloseCallback === function)) {
>       onCloseCallback();
>     }
> 
>     WapPushManager.close();
>   }
> 
>   setOnCloseCallback: function wpm_setOnCloseCallback(callback) {
>     this.onCloseCallback = callback;
>   }
> }
> 
> ...
> 
> function cpsh_populateScreen(message) {
>   WapPushManager.setOnCloseCallback(cpsh_onClose);
>   ...
> }

Thank you very much for the suggestion. It works. Changed.

> ::: apps/wappush/js/wappush.js
> @@ +135,5 @@
> >            var iconURL = NotificationHelper.getIconURI(app) +
> >                         '?timestamp=' + encodeURIComponent(message.timestamp);
> > +
> > +          message.text = (message.type == 'text/vnd.wap.connectivity-xml') ?
> > +                         _(message.text) : message.text;
> 
> Move this into CpScreenHelper.populateScreen(), it makes more sense there.

This change needs to he here. This code is for building the notification.

> ::: apps/wappush/locales/wappush.en-US.properties
> @@ +2,5 @@
> > +quit             = Quit
> > +cancel           = Cancel
> > +store            = Store
> > +try-again        = Try again
> > +finish           = Finish
> 
> We're missing a generic description for the 'title' identifier in the SI/SL
> screen, this used to be:
> 
> wap-push-message = WAP Push message
> 
> It's OK if you turn it into
> 
> title = WAP Push message

Done.

Would you take a look again please? Thanks!
Attachment #819829 - Attachment is obsolete: true
Attachment #821774 - Flags: review?(gsvelto)
Attached patch Part 6: Testing. (obsolete) — Splinter Review
This patch is for testing. I've been developing this bug and testing the work with a real client provisioning server. I guess not many people have a real server to test with.

Use '0000' as PIN if you are prompted for a PIN.
Oops, forgot to mention that in order to see the new APNs you need a patch from bug 922177 also.
Comment on attachment 821774 [details] [diff] [review]
Part 2: Update UI. v4

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

Looks good!
Attachment #821774 - Flags: review?(gsvelto) → review+
Comment on attachment 820224 [details] [diff] [review]
Part 5: Authenticate server. v2

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

I'd like to give this an r+ as it looks good to me but there's an issue I have to figure out: I don't know if we can introduce GPLv3 code in Gaia or not. In any case please move the digest.js code in a subdirectory called ext (so apps/wappush/js/ext/digest.js); that's the common way we have to flag that the file has been imported from another project.
(In reply to Gabriele Svelto [:gsvelto] from comment #58)
> Comment on attachment 820224 [details] [diff] [review]
> Part 5: Authenticate server. v2
> 
> Review of attachment 820224 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for the review.

> In any case please move the digest.js code in a subdirectory called ext
> (so apps/wappush/js/ext/digest.js); that's the common way we have to flag
> that the file has been imported from another project.

It's already at apps/wappush/js/ext/ directory.
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #59)
> It's already at apps/wappush/js/ext/ directory.

Sorry, the review interface strips the paths so I hadn't noticed :(
Comment on attachment 820224 [details] [diff] [review]
Part 5: Authenticate server. v2

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

It seems that we have a problem here due to the anti-tivoization clause; this might be problematic for vendors and I fear we cannot accept GPLv3 code in Gaia so this is r- for now unless you can find a non-GPLv3 alternative. I'll ask for further information to :gerv as he knows more about this stuff and can give us a definitive answer.
Attachment #820224 - Flags: review?(gsvelto) → review-
(In reply to Gabriele Svelto [:gsvelto] from comment #61)
> Comment on attachment 820224 [details] [diff] [review]
> Part 5: Authenticate server. v2
> 
> Review of attachment 820224 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for figuring this out.

> It seems that we have a problem here due to the anti-tivoization clause;
> this might be problematic for vendors and I fear we cannot accept GPLv3 code
> in Gaia so this is r- for now unless you can find a non-GPLv3 alternative.

What other alternatives do I have? I mean, GPLv2 is fine, any other else?. I don't think implement the library myself is a alternative.
Hi Gervase, we've got a patch here that depends on a piece of GPLv3-licensed JavaScript for its functionality. I've r-'d it for now as I'm not sure if we can include GPLv3 code in Gaia or not. AFAIK the license deals with distribution of binaries and I have no idea how it applies to JavaScript were we compile and link at run-time and never ship a "real" binary to the user.
Oops, in-flight collision ate the needinfo flag, reposting:

Hi Gervase, we've got a patch here that depends on a piece of GPLv3-licensed JavaScript for its functionality. I've r-'d it for now as I'm not sure if we can include GPLv3 code in Gaia or not. AFAIK the license deals with distribution of binaries and I have no idea how it applies to JavaScript were we compile and link at run-time and never ship a "real" binary to the user.
Flags: needinfo?(gerv)
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #62)
> What other alternatives do I have? I mean, GPLv2 is fine, any other else?. I
> don't think implement the library myself is a alternative.

I can think of two things now:

- You can try to contact the digest.js author and ask him if he'd be willing to offer digest.js under a more permissive license that would be compatible with Gaia

- Alternatively you can look for an alternative. I've found a library providing digest authentication for node.js, it might be possible to re-purpose it for your patch but I've got no idea how complicated that would be:

https://github.com/gevorg/http-auth

I'm really sorry I hadn't spotted this earlier during review; it would have saved us some time :(.
Sorry, we can't ship GPLv3 code in Gaia. I suggest you cast around for an alternative and, if nothing obvious presents itself, let me know and I will talk to the author of the code you are using and ask him to consider relicensing. (Best if I do that; no disrespect, but when I have let other people do it in the past, they normally ask the wrong question or miss key points off :-)

Gerv
Flags: needinfo?(gerv)
(In reply to Gervase Markham [:gerv] from comment #66)

Thanks for the information.

> Sorry, we can't ship GPLv3 code in Gaia.

I'm curious, may I know what licenses are permitted please? I've seen BDS code already in Gaia.
Flags: needinfo?(gerv)
(In reply to Gervase Markham [:gerv] from comment #66)
> Sorry, we can't ship GPLv3 code in Gaia. I suggest you cast around for an
> alternative and, 

Alternatives are difficult to find. I mean other libraries, e.g. MPL ones. I'm working right now on having our on library but it will take more time and we are a little bit late (this is v1.2 feature).

> if nothing obvious presents itself, let me know and I will
> talk to the author of the code you are using and ask him to consider
> relicensing. (Best if I do that; no disrespect, but when I have let other
> people do it in the past, they normally ask the wrong question or miss key
> points off :-)

What about trying this alternative in parallel? I also think that it's best if you do that. Could you do it please? Thanks.

Note: digest.js repo at https://github.com/jcsirot/digest.js
(In reply to José Antonio Olivera Ortega [:jaoo] from comment #67)
> I'm curious, may I know what licenses are permitted please? I've seen BDS
> code already in Gaia.

You mean BSD? That's fine.
http://www.mozilla.org/MPL/license-policy.html has the full explanation.

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #68)
> What about trying this alternative in parallel? I also think that it's best
> if you do that. Could you do it please? Thanks.

OK; here's what I need to know:

* What exactly do we need it for? Is "using HMAC-SHA1 to authenticate configuration messages as part of the WAP Push feature" the right way to explain it?

* How much of the code are you using? It looks like the digest.js file is his entire project, so if you need it all that's basically a project-wide licence change for him. It may be more acceptable to him if we only need a part of the code.

* If this is going to be part of a Gaia app, I assume that we would need the code under Apache 2. Is that correct?

Also, for my information: our codebase is packed with crypto stuff, and we even have an implementation of an entire crypto API in JS. Why is none of our existing code suitable? I find it hard to believe we don't have code capable of doing an HMAC using SHA1 anywhere in our current platform...

Gerv
Flags: needinfo?(gerv)
Attached patch Part 5: Authenticate server. v3 (obsolete) — Splinter Review
(In reply to Gabriele Svelto [:gsvelto] from comment #61)
> Comment on attachment 820224 [details] [diff] [review]
> Part 5: Authenticate server. v2
> 
> Review of attachment 820224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems that we have a problem here due to the anti-tivoization clause;
> this might be problematic for vendors and I fear we cannot accept GPLv3 code
> in Gaia so this is r- for now unless you can find a non-GPLv3 alternative.

Now using a BSD licensed library. Tested on the device against a real server. Tess are also passing.

Would you mind to take a look please? Thanks!
Attachment #820224 - Attachment is obsolete: true
Attachment #823924 - Flags: review?(gsvelto)
(In reply to Gervase Markham [:gerv] from comment #69)
> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #67)

Thanks for the information.

> > I'm curious, may I know what licenses are permitted please? I've seen BDS
> > code already in Gaia.
> 
> You mean BSD? That's fine.
> http://www.mozilla.org/MPL/license-policy.html has the full explanation.

Latest part 5 patch uses a BSD licensed library.

> (In reply to José Antonio Olivera Ortega [:jaoo] from comment #68)
> > What about trying this alternative in parallel? I also think that it's best
> > if you do that. Could you do it please? Thanks.

This wouldn't be necessary. Thanks for willing to help us and sorry for the noise.
Comment on attachment 823924 [details] [diff] [review]
Part 5: Authenticate server. v3

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

This is OK though we'll need to do another run at all the patches as I've been landing changes in the meantime. I suggest to wait for bug 911934 to land and rebase on top of that. I did it myself to test your patches here and the changes are fairly trivial, if you run into problems regarding the rebase please contact me. Once everything is in good shape and travis is green we can land this.
Attachment #823924 - Flags: review?(gsvelto) → review+
(In reply to Gabriele Svelto [:gsvelto] from comment #72)
> Comment on attachment 823924 [details] [diff] [review]
> Part 5: Authenticate server. v3
> 
> Review of attachment 823924 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks!

> This is OK though we'll need to do another run at all the patches as I've
> been landing changes in the meantime.

I'm aware of it. I've been rebasing the parches. Most up ti date parches are already in github.

> I suggest to wait for bug 911934 to
> land and rebase on top of that.

Ok, once you finish that bug I'll rebase again. I'll attach them later.

> I did it myself to test your patches here
> and the changes are fairly trivial, if you run into problems regarding the
> rebase please contact me. Once everything is in good shape and travis is
> green we can land this.

Most up to date parches works correctly ante trata pass.
Attachment #821549 - Attachment is obsolete: true
Attachment #821774 - Attachment is obsolete: true
Attached patch Part 6: Testing.Splinter Review
Attachment #822386 - Attachment is obsolete: true
gaia/master: https://github.com/mozilla-b2g/gaia/commit/c220e3711b7bd5e21cf04587e10b06f91e85b80e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
After landing this I noticed that the patch-set introduced some small visual regressions which I'm fixing in bug 932859. So this is a reminder that if we decide to uplift this to v1.2 we'll need to uplift bug 932859 too.
No longer blocks: 932859
Depends on: 932859
> cp-accept-help = Configuration message received, click on 'Accept' to continue.

Is "click" the right choice of words on a touch device?
(In reply to Francesco Lodolo [:flod] from comment #83)
> Is "click" the right choice of words on a touch device?

Definitely not the best choice, though we use it in other places too: http://is.gd/63I9s2

'Press/tap "Accept" to continue' might be better, shall we file a follow-up?
(In reply to Gabriele Svelto [:gsvelto] from comment #84)
> 'Press/tap "Accept" to continue' might be better, shall we file a follow-up?

Maybe a generic bug to review this kind of strings would be useful(cc :matej and :pike, not sure what the policy is to change strings on master right now, I think we need new IDs anyway).
String reviews should be done by Matej, really.

I'm chiming in because this landing comes with string changes, and possible follow-ups, and we're having the hard string freeze for 1.2 on Monday, November 11.

Do we have all necessary data to decide on the uplift by then?
Keywords: late-l10n
(In reply to Axel Hecht [:Pike] from comment #86)
> String reviews should be done by Matej, really.
> 
> I'm chiming in because this landing comes with string changes, and possible
> follow-ups, and we're having the hard string freeze for 1.2 on Monday,
> November 11.
> 
> Do we have all necessary data to decide on the uplift by then?

I'll add you to the email thread w/product that's discussing this right now.
The timing on this is too late to enter 1.2. We should work on clearing up the remaining bugs (however minor) and target 1.3.
Flags: needinfo?(bhuang)
Moving to 1.3+ per comment 88
blocking-b2g: koi+ → 1.3+
Whiteboard: [NO_UPLIFT]
Removing late-l10n as this only ever landed on master.
Keywords: late-l10n
You need to log in before you can comment on or make changes to this bug.