Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174423002:
IDL implementation of requestPin API in certificateProvider. (Closed)
DescriptionIDL implementation of requestPin API in certificateProvider.
BUG=612884
Patch Set 1 #Patch Set 2 : Small fixes in comments #
Total comments: 22
Patch Set 3 : Fixed review comments #
Total comments: 8
Patch Set 4 : Added details in comments #Patch Set 5 : Fixed review comments #
Total comments: 2
Patch Set 6 : Fixed review comments #
Total comments: 1
Messages
Total messages: 15 (7 generated)
Description was changed from ========== Implementation after review BUG= ========== to ========== Implementation of requestPin API in certificateProvider. BUG=612884 ==========
stevenjb@chromium.org changed reviewers: + stevenjb@chromium.org
https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:41: long signRequestId; Nice solution, thanks. This needs to be optional though, right? https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:61: // The error message to display for user. Default - no error. The error request would be set if a previous failed, correct? We should document that. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:64: // The number of attempts left. We should expand this comment, e.g.: // The number of attempts left. This is provided so that any UI can present this information to the user. Chrome is not expected to enforce this, instead StopPinRequestDetails will be called with errorType = MAX_ATTEMPTS_EXCEEDED when the number of pin requests is exceeded. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:68: dictionary StopPinRequestDetails { We should probably pass signRequestId here. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:73: DOMString? userInput; This should also provide an error value, e.g. 'REQUEST_ID_NOT_FOUND', 'USER_CANCELED', 'UNAVAILABLE' (in case we have scenarios where we can't actually show a dialog, even though we currently don't expect that to be the case), 'BUSY' (in case the UI is already showing a different system modal dialog). https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:83: callback StopPinRequestCallback = void (); Do we actually need this?
igorcov@google.com changed reviewers: + igorcov@google.com
https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:41: long signRequestId; On 2016/07/25 18:57:57, stevenjb wrote: > Nice solution, thanks. This needs to be optional though, right? I think Chrome should always include this parameter as it doesn't know at this point if PIN will be required or not. Don't see any scenario when this could be skipped. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:61: // The error message to display for user. Default - no error. On 2016/07/25 18:57:57, stevenjb wrote: > The error request would be set if a previous failed, correct? We should document > that. Done. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:64: // The number of attempts left. On 2016/07/25 18:57:57, stevenjb wrote: > We should expand this comment, e.g.: > // The number of attempts left. This is provided so that any UI can present this > information to the user. Chrome is not expected to enforce this, instead > StopPinRequestDetails will be called with errorType = MAX_ATTEMPTS_EXCEEDED when > the number of pin requests is exceeded. > Updated, thanks. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:68: dictionary StopPinRequestDetails { On 2016/07/25 18:57:57, stevenjb wrote: > We should probably pass signRequestId here. At this point we can use the active extension_id (the one that initiated the flow) to check that it matches the extension that asked to stop the request. This allows us to validate the request without the ID. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:73: DOMString? userInput; On 2016/07/25 18:57:57, stevenjb wrote: > This should also provide an error value, e.g. 'REQUEST_ID_NOT_FOUND', > 'USER_CANCELED', 'UNAVAILABLE' (in case we have scenarios where we can't > actually show a dialog, even though we currently don't expect that to be the > case), 'BUSY' (in case the UI is already showing a different system modal > dialog). Good point, but this is intended to be achieved using chrome.runtime.lastError variable. As that seems to be the standard way to notify about the errors in Chrome API. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:83: callback StopPinRequestCallback = void (); On 2016/07/25 18:57:57, stevenjb wrote: > Do we actually need this? It might be useful in case the user closes the dialog while the extension is processing previous input. Using this callback and chrome.runtime.lastError the extension will be notified that user stopped the flow manually. Other case is when extension asks to stop the flow that wasn't initiated at all. It would provide a way to have more information and better debug abilities when writing the extension code.
https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:41: long signRequestId; On 2016/07/26 10:21:11, igorcov1 wrote: > On 2016/07/25 18:57:57, stevenjb wrote: > > Nice solution, thanks. This needs to be optional though, right? > > I think Chrome should always include this parameter as it doesn't know at this > point if PIN will be required or not. Don't see any scenario when this could be > skipped. Making this required would break any existing usage. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:68: dictionary StopPinRequestDetails { On 2016/07/26 10:21:11, igorcov1 wrote: > On 2016/07/25 18:57:57, stevenjb wrote: > > We should probably pass signRequestId here. > > At this point we can use the active extension_id (the one that initiated the > flow) to check that it matches the extension that asked to stop the request. > This allows us to validate the request without the ID. Conceivably couldn't the extension issue more than one request? (Chrome would presumably handle one at a time). If we intend to disallow that we should document that and add an error code. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:73: DOMString? userInput; On 2016/07/26 10:21:11, igorcov1 wrote: > On 2016/07/25 18:57:57, stevenjb wrote: > > This should also provide an error value, e.g. 'REQUEST_ID_NOT_FOUND', > > 'USER_CANCELED', 'UNAVAILABLE' (in case we have scenarios where we can't > > actually show a dialog, even though we currently don't expect that to be the > > case), 'BUSY' (in case the UI is already showing a different system modal > > dialog). > > Good point, but this is intended to be achieved using chrome.runtime.lastError > variable. As that seems to be the standard way to notify about the errors in > Chrome API. Fair point. We should document that, and define what the response looks like on failure (empty?). https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:83: callback StopPinRequestCallback = void (); On 2016/07/26 10:21:11, igorcov1 wrote: > On 2016/07/25 18:57:57, stevenjb wrote: > > Do we actually need this? > > It might be useful in case the user closes the dialog while the > extension is processing previous input. Using this callback and > chrome.runtime.lastError the extension will be notified that user stopped the > flow manually. > > Other case is when extension asks to stop the flow that wasn't initiated at all. > It would provide a way to have more information and better debug abilities when > writing the extension code. Acknowledged.
emaxx@chromium.org changed reviewers: + emaxx@chromium.org
https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:68: dictionary StopPinRequestDetails { On 2016/07/26 16:10:15, stevenjb wrote: > On 2016/07/26 10:21:11, igorcov1 wrote: > > On 2016/07/25 18:57:57, stevenjb wrote: > > > We should probably pass signRequestId here. > > > > At this point we can use the active extension_id (the one that initiated the > > flow) to check that it matches the extension that asked to stop the request. > > This allows us to validate the request without the ID. > > Conceivably couldn't the extension issue more than one request? (Chrome would > presumably handle one at a time). If we intend to disallow that we should > document that and add an error code. +1 for passing request id here. Even though our current plan is to disallow simultaneous requests, requiring to pass the id here leaves us with more choices for the future changes. Also this would make the API a bit more symmetric. https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:16: enum RequestType { Maybe better to rename it to "PinRequestType", so that it may not clash with any other "request" that could be added into the API in the future? https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:21: enum ErrorType { And here probably too: "PinRequestErrorType"? As the current name looks like it's a generic error type for the whole certificateProvider API. https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:40: // The unique ID to be used if extension will require a PIN. I think the comment in this structure shouldn't talk about PIN. It's just a unique identifier, which potentially may be used for other purposes too. https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:86: // The callback to be used by Chrome to send to middleware application the nit: s/middleware application/the extension/ (the word "middleware" is not used anywhere in these docs)
Description was changed from ========== Implementation of requestPin API in certificateProvider. BUG=612884 ========== to ========== IDL implementation of requestPin API in certificateProvider. BUG=612884 ==========
https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:41: long signRequestId; On 2016/07/26 16:10:15, stevenjb wrote: > On 2016/07/26 10:21:11, igorcov1 wrote: > > On 2016/07/25 18:57:57, stevenjb wrote: > > > Nice solution, thanks. This needs to be optional though, right? > > > > I think Chrome should always include this parameter as it doesn't know at this > > point if PIN will be required or not. Don't see any scenario when this could > be > > skipped. > > Making this required would break any existing usage. I don't see how it would break anything existent. It's an event, so the extension just receives additional parameter in request. The old implementations should ignore it. There are often changes in Chrome that add mandatory parameters in events, for example this CL: http://crrev.com/1857033002 https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:68: dictionary StopPinRequestDetails { On 2016/07/26 16:10:15, stevenjb wrote: > On 2016/07/26 10:21:11, igorcov1 wrote: > > On 2016/07/25 18:57:57, stevenjb wrote: > > > We should probably pass signRequestId here. > > > > At this point we can use the active extension_id (the one that initiated the > > flow) to check that it matches the extension that asked to stop the request. > > This allows us to validate the request without the ID. > > Conceivably couldn't the extension issue more than one request? (Chrome would > presumably handle one at a time). If we intend to disallow that we should > document that and add an error code. Yes, we don't plan to keep a queue of requests. If active request is on going, all the new requests are rejected immediately. Added this in comments to PinResponseDetails and to requestPin to clarify this. https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:73: DOMString? userInput; On 2016/07/26 16:10:15, stevenjb wrote: > On 2016/07/26 10:21:11, igorcov1 wrote: > > On 2016/07/25 18:57:57, stevenjb wrote: > > > This should also provide an error value, e.g. 'REQUEST_ID_NOT_FOUND', > > > 'USER_CANCELED', 'UNAVAILABLE' (in case we have scenarios where we can't > > > actually show a dialog, even though we currently don't expect that to be the > > > case), 'BUSY' (in case the UI is already showing a different system modal > > > dialog). > > > > Good point, but this is intended to be achieved using chrome.runtime.lastError > > variable. As that seems to be the standard way to notify about the errors in > > Chrome API. > > Fair point. We should document that, and define what the response looks like on > failure (empty?). > Done.
https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:68: dictionary StopPinRequestDetails { On 2016/07/26 17:36:21, emaxx wrote: > On 2016/07/26 16:10:15, stevenjb wrote: > > On 2016/07/26 10:21:11, igorcov1 wrote: > > > On 2016/07/25 18:57:57, stevenjb wrote: > > > > We should probably pass signRequestId here. > > > > > > At this point we can use the active extension_id (the one that initiated the > > > flow) to check that it matches the extension that asked to stop the request. > > > This allows us to validate the request without the ID. > > > > Conceivably couldn't the extension issue more than one request? (Chrome would > > presumably handle one at a time). If we intend to disallow that we should > > document that and add an error code. > > +1 for passing request id here. > > Even though our current plan is to disallow simultaneous requests, requiring to > pass the id here leaves us with more choices for the future changes. Also this > would make the API a bit more symmetric. Added the ID here and made the details parameter in stopPinRequest mandatory. https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:16: enum RequestType { On 2016/07/26 17:36:21, emaxx wrote: > Maybe better to rename it to "PinRequestType", so that it may not clash with any > other "request" that could be added into the API in the future? Done. https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:21: enum ErrorType { On 2016/07/26 17:36:21, emaxx wrote: > And here probably too: "PinRequestErrorType"? > As the current name looks like it's a generic error type for the whole > certificateProvider API. Done. https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:40: // The unique ID to be used if extension will require a PIN. On 2016/07/26 17:36:21, emaxx wrote: > I think the comment in this structure shouldn't talk about PIN. > It's just a unique identifier, which potentially may be used for other purposes > too. Done. https://codereview.chromium.org/2174423002/diff/40001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:86: // The callback to be used by Chrome to send to middleware application the On 2016/07/26 17:36:21, emaxx wrote: > nit: s/middleware application/the extension/ > (the word "middleware" is not used anywhere in these docs) Done.
OK, this LGTM. Thanks for your patience! https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/20001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:41: long signRequestId; On 2016/07/26 17:45:29, igorcov1 wrote: > On 2016/07/26 16:10:15, stevenjb wrote: > > On 2016/07/26 10:21:11, igorcov1 wrote: > > > On 2016/07/25 18:57:57, stevenjb wrote: > > > > Nice solution, thanks. This needs to be optional though, right? > > > > > > I think Chrome should always include this parameter as it doesn't know at > this > > > point if PIN will be required or not. Don't see any scenario when this could > > be > > > skipped. > > > > Making this required would break any existing usage. > > I don't see how it would break anything existent. It's an event, so the > extension just receives additional parameter in request. The old implementations > should ignore it. There are often changes in Chrome that add mandatory > parameters in events, for example this CL: http://crrev.com/1857033002 I guess that JS being JS it won't break the code. It might break Closure compilation, but probably mostly not. https://codereview.chromium.org/2174423002/diff/80001/chrome/common/extension... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/80001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:63: // request failed, to notify about the reason of fail. s/notify about the reason of fail/notify the user of the failure reason/ https://codereview.chromium.org/2174423002/diff/80001/chrome/common/extension... chrome/common/extensions/api/certificate_provider.idl:88: // OTHER_FLOW_IN_PROGRESS - A request PIN flow in ongoing already. s/in/is/
Patchset #6 (id:100001) has been deleted
lgtm https://codereview.chromium.org/2174423002/diff/120001/chrome/common/extensio... File chrome/common/extensions/api/certificate_provider.idl (right): https://codereview.chromium.org/2174423002/diff/120001/chrome/common/extensio... chrome/common/extensions/api/certificate_provider.idl:88: // OTHER_FLOW_IN_PROGRESS - A request PIN flow is ongoing already. You have to check with the other existing APIs, but I thing chrome.runtime.lastError.message usually contains a human-readable error. So it's better not to list here any exact string values.
Message was sent while issue was closed.
igorcov@chromium.org changed reviewers: - igorcov@google.com |
