Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
312503004:
Make enterprise.platformKeys documentation public. (Closed)
|
Created:
6 years, 6 months ago by pneubeck (no reviews) Modified:
6 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionMake enterprise.platformKeys documentation public.
Removes TODOs, adds some more important details and an example to the documentation.
Makes the documentation appear on the official developer.chrome.com page.
BUG=364435
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274591
Patch Set 1 : #
Total comments: 21
Patch Set 2 : Addressed comments. Mention "pre-install" requirement. #Patch Set 3 : Fix in the example. #
Total comments: 4
Patch Set 4 : Removing trailing whitespace. #Patch Set 5 : Addressed comments. #
Messages
Total messages: 12 (0 generated)
Generated documentation should be available here: https://chrome-apps-doc.appspot.com/_patch/312503004/extensions/enterprise_pl... @Ryan ptal. The documentation changes are nothing new compared to the design doc. The example is an extract from the existing api test. @Benjamin, please ensure the conformance to the extension framework. again.
On 2014/06/02 16:44:25, pneubeck wrote: > Generated documentation should be available here: > https://chrome-apps-doc.appspot.com/_patch/312503004/extensions/enterprise_pl... > > @Ryan ptal. The documentation changes are nothing new compared to the design > doc. The example is an extract from the existing api test. > > @Benjamin, please ensure the conformance to the extension framework. again. ah, and Benjamin, it seems that type definitions with single usage (here 'Token') are always inlined. Is there a way to prevent the inlining? I'd really like to have the Token type defined in the toplevel 'Types' section.
some high level comments, I got a bit lost in the bigger .html file with the technical stuff. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... File chrome/common/extensions/api/enterprise_platform_keys.idl (right): https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:11: [nocompile] dictionary Token { noinline_doc here should work, like [nocompile noinline_doc] https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:20: // interface. The crypto operations are hardware-backed. why is being hardware backed important? is it always true on all platforms? https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:21: // Only non-extractable RSASSA-PKCS1-V1_5 keys with moduloLength upto 2048 s/upto/up to https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:24: // with <code>window.crypto.subtle</code>. The opposite doesn't work either. I don't quite understand what these last 2 sentences mean... it might be because I'm not sure what the last sentence is saying. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:47: // SubtleCrypto interface. what restrictions? can you link to them? https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html (right): https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:2: <b>Note: </b> consider using <strong> not <b> https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:3: This API is only available on ChromeOS. mention what it will do on non-ChromeOS platforms
A few quick comments https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... File chrome/common/extensions/api/enterprise_platform_keys.idl (right): https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:20: // interface. The crypto operations are hardware-backed. On 2014/06/02 17:05:16, kalman wrote: > why is being hardware backed important? > > is it always true on all platforms? The cryptographic operations, including key generation, are hardware-backed. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:21: // Only non-extractable RSASSA-PKCS1-V1_5 keys with moduloLength upto 2048 On 2014/06/02 17:05:16, kalman wrote: > s/upto/up to modulo/modulus/ https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:24: // with <code>window.crypto.subtle</code>. The opposite doesn't work either. On 2014/06/02 17:05:16, kalman wrote: > I don't quite understand what these last 2 sentences mean... it might be because > I'm not sure what the last sentence is saying. Keys generated on a specific token cannot be used with any other Tokens, nor can they be used with <code>window.crypto.subtle</code>. Equally, Key objects created with <code>window.crypto.subtle</code> cannot be used with this interface.
https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... File chrome/common/extensions/api/enterprise_platform_keys.idl (right): https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:11: [nocompile] dictionary Token { On 2014/06/02 17:05:16, kalman wrote: > noinline_doc here should work, like [nocompile noinline_doc] Awesome! Thanks. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:21: // Only non-extractable RSASSA-PKCS1-V1_5 keys with moduloLength upto 2048 On 2014/06/03 00:02:42, Ryan Sleevi wrote: > On 2014/06/02 17:05:16, kalman wrote: > > s/upto/up to > > modulo/modulus/ Done. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:21: // Only non-extractable RSASSA-PKCS1-V1_5 keys with moduloLength upto 2048 On 2014/06/02 17:05:16, kalman wrote: > s/upto/up to Done. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:24: // with <code>window.crypto.subtle</code>. The opposite doesn't work either. On 2014/06/03 00:02:42, Ryan Sleevi wrote: > On 2014/06/02 17:05:16, kalman wrote: > > I don't quite understand what these last 2 sentences mean... it might be > because > > I'm not sure what the last sentence is saying. > > Keys generated on a specific token cannot be used with any other Tokens, nor can > they be used with <code>window.crypto.subtle</code>. Equally, Key objects > created with <code>window.crypto.subtle</code> cannot be used with this > interface. Done. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:24: // with <code>window.crypto.subtle</code>. The opposite doesn't work either. On 2014/06/02 17:05:16, kalman wrote: > I don't quite understand what these last 2 sentences mean... it might be because > I'm not sure what the last sentence is saying. Done. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:47: // SubtleCrypto interface. On 2014/06/02 17:05:16, kalman wrote: > what restrictions? can you link to them? Removed. That was rather redundant and misleading as SubtleCrypto wasn't mentioned before. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html (right): https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:2: <b>Note: </b> On 2014/06/02 17:05:16, kalman wrote: > consider using <strong> not <b> Done. https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:3: This API is only available on ChromeOS. On 2014/06/02 17:05:16, kalman wrote: > mention what it will do on non-ChromeOS platforms What do you mean? On other platforms it's not available and thus Chrome will not grant the required permission. This follows the example of docs/templates/intros/fileBrowserHandler.html and notifications.html
https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html (right): https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:3: This API is only available on ChromeOS. On 2014/06/03 09:22:21, pneubeck wrote: > On 2014/06/02 17:05:16, kalman wrote: > > mention what it will do on non-ChromeOS platforms > > What do you mean? On other platforms it's not available and thus Chrome will not > grant the required permission. > > This follows the example of > docs/templates/intros/fileBrowserHandler.html > and notifications.html Oops notifications is wrong. And good point on the fileBrowserHandler thing; we should probably have a standard template for chromeos-only actually. Anyhow, what I more meant was that that could mean that either chrome.enterprise.platformKeys doesn't exist on non-chromeos or that it throws errors; and I'm pretty sure that it's the latter of those. However I think you can hold off changes to this warning because you're going to need to mention the policy thing at some point once it's implemented.
https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html (right): https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:3: This API is only available on ChromeOS. On 2014/06/03 14:44:15, kalman wrote: > On 2014/06/03 09:22:21, pneubeck wrote: > > On 2014/06/02 17:05:16, kalman wrote: > > > mention what it will do on non-ChromeOS platforms > > > > What do you mean? On other platforms it's not available and thus Chrome will > not > > grant the required permission. > > > > This follows the example of > > docs/templates/intros/fileBrowserHandler.html > > and notifications.html > > Oops notifications is wrong. > > And good point on the fileBrowserHandler thing; we should probably have a > standard template for chromeos-only actually. Anyhow, what I more meant was that > that could mean that either chrome.enterprise.platformKeys doesn't exist on > non-chromeos or that it throws errors; and I'm pretty sure that it's the latter > of those. Shouldn't this behavior be the same as for all APIs for which the extension wasn't granted the permission? This behavior should also be determined by the '"platforms": ["chromeos"],' setting in _permission_features.json? > However I think you can hold off changes to this warning because you're going to > need to mention the policy thing at some point once it's implemented. The "policy thing" (requiring the extension to be pre-installed (aka force-installed) by policy) is already in place. I also updated the warning already to mention this requirement.
lgtm https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html (right): https://codereview.chromium.org/312503004/diff/30001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:3: This API is only available on ChromeOS. On 2014/06/03 14:48:58, pneubeck wrote: > On 2014/06/03 14:44:15, kalman wrote: > > On 2014/06/03 09:22:21, pneubeck wrote: > > > On 2014/06/02 17:05:16, kalman wrote: > > > > mention what it will do on non-ChromeOS platforms > > > > > > What do you mean? On other platforms it's not available and thus Chrome will > > not > > > grant the required permission. > > > > > > This follows the example of > > > docs/templates/intros/fileBrowserHandler.html > > > and notifications.html > > > > Oops notifications is wrong. > > > > And good point on the fileBrowserHandler thing; we should probably have a > > standard template for chromeos-only actually. Anyhow, what I more meant was > that > > that could mean that either chrome.enterprise.platformKeys doesn't exist on > > non-chromeos or that it throws errors; and I'm pretty sure that it's the > latter > > of those. > Shouldn't this behavior be the same as for all APIs for which the extension > wasn't granted the permission? > This behavior should also be determined by the '"platforms": ["chromeos"],' > setting in _permission_features.json? Yes! We can auto-generate it. But regarding my previous comment, I more mean that failure mechanisms include the API not existing or the API setting chrome.runtime.lastError. I think that non-chromeos platforms has the previous behaviour, and non-policy extensions the latter? A more precise way of phrasing this would be "This API doesn't exist on non-ChromeOS platforms and will file for non-policy-installed extensions" but that's a bit wordy. What you have is fine I suppose. > > > However I think you can hold off changes to this warning because you're going > to > > need to mention the policy thing at some point once it's implemented. > > The "policy thing" (requiring the extension to be pre-installed (aka > force-installed) by policy) is already in place. > I also updated the warning already to mention this requirement. Thanks. https://codereview.chromium.org/312503004/diff/90001/chrome/common/extensions... File chrome/common/extensions/api/enterprise_platform_keys.idl (right): https://codereview.chromium.org/312503004/diff/90001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:22: // hardware-backed. can you split this and the description for |id| into a couple of paragraphs each? the giant block of text looks a bit unwieldy. https://codereview.chromium.org/312503004/diff/90001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html (right): https://codereview.chromium.org/312503004/diff/90001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:31: <pre> put a data-filename="background.js" attribute on this.
https://codereview.chromium.org/312503004/diff/90001/chrome/common/extensions... File chrome/common/extensions/api/enterprise_platform_keys.idl (right): https://codereview.chromium.org/312503004/diff/90001/chrome/common/extensions... chrome/common/extensions/api/enterprise_platform_keys.idl:22: // hardware-backed. On 2014/06/03 15:12:21, kalman wrote: > can you split this and the description for |id| into a couple of paragraphs > each? the giant block of text looks a bit unwieldy. Done. https://codereview.chromium.org/312503004/diff/90001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html (right): https://codereview.chromium.org/312503004/diff/90001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/enterprise_platformKeys.html:31: <pre> On 2014/06/03 15:12:21, kalman wrote: > put a data-filename="background.js" attribute on this. Done.
The CQ bit was checked by pneubeck@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pneubeck@chromium.org/312503004/130001
Message was sent while issue was closed.
Change committed as 274591 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
