Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1284413004:
Keep track of per-origin security details in the Security panel. (Closed)
|
Created:
5 years, 4 months ago by lgarron Modified:
5 years, 4 months ago CC:
apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, kozyatinskiy+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, yurys+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionKeep track of per-origin security details in the Security panel.
BUG=502118, 503170
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=200868
Patch Set 1 : #
Total comments: 12
Patch Set 2 : Refactor the ResponseReceivedSecurityDetails dispatch method and handle it directly in the Security panel. #
Total comments: 8
Patch Set 3 : Incorporate dgozman@'s suggestions and use MainFrameNavigated instead of LoadEventFired. #
Total comments: 10
Patch Set 4 : Change http security state to neutral. #Patch Set 5 : Fix nits and align with WebInspector.TargetManager.prototype to listen for navigations. #
Total comments: 2
Patch Set 6 : Add type annotation for the Map. #
Messages
Total messages: 32 (14 generated)
Patchset #1 (id:1) has been deleted
lgarron@chromium.org changed reviewers: + dgozman@chromium.org
dgozman@: I'm working on sending updates from the Network domain to the Security panel. This will involve two things for v1: - Sending updates to the Security panel about origin security state changes (partially covered in this CL). - Sending an event to the Security panel every time a mixed content request is created (not in this CL). This CL has some work left to do, but I'd appreciate feedback on whether this is an appropriate way to route events through SecurityModel.
https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityModel.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityModel.js:21: this.origins = { My plan is for SecurityModel as a middleman to keep track of security properties of an origin (e.g. security state, connection details), and calculate+route the appropriate changes to the Security panel so that the Security panel can focus on UI.
pfeldman@chromium.org changed reviewers: + pfeldman@chromium.org
https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/NetworkManager.js:360: this._manager._networkAgent.getCertificateDetails(response.securityDetails.certificateId, (function(error, certificateDetails) { Please name the function and annotate it. https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityModel.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityModel.js:17: this._networkManager.addEventListener(WebInspector.NetworkManager.EventTypes.ReceivedResponseSecurity, this._onReceivedResponseSecurity, this); It is not typical for models to listen to each other's events, it is better to perform the binding on the UI level should the need be. https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:74: _onOriginSecurityChanged: function(event) { code style, annotations
https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/NetworkManager.js:356: // TODO(lgarron): This is ridiculously hacky, and should never land as-is. Calculate the proper origin. Take a look at WebInspector.ParsedURL. https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityModel.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityModel.js:64: else { nit: else on the same line as } https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityModel.js:76: _securityStateDecreased: function(oldState, newState) { nit: { on next line https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:76: console.log(event.data); Please don't commit console.log into production code.
(Not addressing the nits individually, but thanks for those, too.) https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/NetworkManager.js:356: // TODO(lgarron): This is ridiculously hacky, and should never land as-is. Calculate the proper origin. On 2015/08/18 at 18:41:49, dgozman wrote: > Take a look at WebInspector.ParsedURL. Thanks, that's what I need for now! (We discussed having Blink send a canonical "origin" value at the Security panel meeting yesterday, but ParsedURL is right for now.) https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/NetworkManager.js:360: this._manager._networkAgent.getCertificateDetails(response.securityDetails.certificateId, (function(error, certificateDetails) { On 2015/08/18 at 18:35:08, pfeldman wrote: > Please name the function and annotate it. Is it alright if I do this inline, or should I add it WebInspector.NetworkDispatcher.prototype? https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityModel.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityModel.js:17: this._networkManager.addEventListener(WebInspector.NetworkManager.EventTypes.ReceivedResponseSecurity, this._onReceivedResponseSecurity, this); On 2015/08/18 at 18:35:08, pfeldman wrote: > It is not typical for models to listen to each other's events, it is better to perform the binding on the UI level should the need be. Thanks, that's very useful to know. In that case, I presume SecurityModel and NetworkManager should both dispatch events to the SecurityPanel, which decides how to handle them. The other panels don't seem to have a centralized model for handling that, so presume I don't need to?
https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/1284413004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/NetworkManager.js:360: this._manager._networkAgent.getCertificateDetails(response.securityDetails.certificateId, (function(error, certificateDetails) { On 2015/08/18 at 19:37:06, lgarron wrote: > On 2015/08/18 at 18:35:08, pfeldman wrote: > > Please name the function and annotate it. > > Is it alright if I do this inline, or should I add it WebInspector.NetworkDispatcher.prototype? Actually, it seems _initNetworkConditions has a great inline example!
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
dgozman@, pfeldman@: I've reworked the CL to send events directly to the Security panel. Would you mind taking a second look?
Can we have a little bit of UI in this patch? Otherwise it's hard to see what we will do with information available. https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... Source/devtools/front_end/sdk/NetworkManager.js:378: } else { No need for |else| because of return. https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:27: this._origins = {}; Use Map. https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:144: _onWillReloadPage: function() { { on new line everywhere. https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:148: _onLoadEventFired: function() { You can call this |clear| and merge with previous function.
Patchset #3 (id:100001) has been deleted
If you just want to see mocks to know where this is going: - The first comment of the bug has an earlier mockup: https://crbug.com/502118 - A designer mock is at https://folio.googleplex.com/chrome-devtools/028-security-panel/01#%2F02%20-%... I'm currently working on implementing the origin views at https://crrev.com/1301833003 but it still needs more work before it's ready for a CL. I think this CL provides a good logical point of separation, but I can try to land minimal UI code for the origin views in this CL if you prefer. https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... Source/devtools/front_end/sdk/NetworkManager.js:378: } else { On 2015/08/19 at 01:51:35, dgozman wrote: > No need for |else| because of return. Done. https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:27: this._origins = {}; On 2015/08/19 at 01:51:35, dgozman wrote: > Use Map. Oooh, shiny. My first time using Map in Javascript. :-D (Done.) https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:144: _onWillReloadPage: function() { On 2015/08/19 at 01:51:35, dgozman wrote: > { on new line everywhere. I try to avoid it, but I still do this out of habit (from my personal Javascript style). Is there a way for me to force this check for my CLs? The presubmit check just seems to ignore it. (Fixed this one, though.) https://codereview.chromium.org/1284413004/diff/80001/Source/devtools/front_e... Source/devtools/front_end/security/SecurityPanel.js:148: _onLoadEventFired: function() { On 2015/08/19 at 01:51:35, dgozman wrote: > You can call this |clear| and merge with previous function. Done. Always happy to keep things DRY. :-)
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
lgtm https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... File Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/sdk/NetworkManager.js:376: console.error("Unable to get certificate details from the browser (for certificate ID ", response.securityDetails.certificateId, "): ", error); Should we just report empty certificate details? https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/sdk/NetworkManager.js:385: nit: extra blank line https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/security/SecurityPanel.js:27: this._origins = new Map(); Please annotate with type. https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/security/SecurityPanel.js:84: var origin = data.origin; This (and securityState) will need a cast. https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/security/SecurityPanel.js:96: if (data.securityDetails) { nit: no {} for one-liners
Patchset #5 (id:200001) has been deleted
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1284413004/#ps220001 (title: "Fix nits and align with WebInspector.TargetManager.prototype to listen for navigations.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284413004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284413004/220001
https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... File Source/devtools/front_end/sdk/NetworkManager.js (right): https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/sdk/NetworkManager.js:376: console.error("Unable to get certificate details from the browser (for certificate ID ", response.securityDetails.certificateId, "): ", error); On 2015/08/19 at 20:22:30, dgozman wrote: > Should we just report empty certificate details? I didn't want to think about dealing with that case, but it's probably the right thing to do. I've updated the code to do so, and will adjust my follow-up CL. :-) https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/sdk/NetworkManager.js:385: On 2015/08/19 at 20:22:30, dgozman wrote: > nit: extra blank line Removed. https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/security/SecurityPanel.js:27: this._origins = new Map(); On 2015/08/19 at 20:22:30, dgozman wrote: > Please annotate with type. Done. https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/security/SecurityPanel.js:84: var origin = data.origin; On 2015/08/19 at 20:22:30, dgozman wrote: > This (and securityState) will need a cast. Done. https://codereview.chromium.org/1284413004/diff/160001/Source/devtools/front_... Source/devtools/front_end/security/SecurityPanel.js:96: if (data.securityDetails) { On 2015/08/19 at 20:22:30, dgozman wrote: > nit: no {} for one-liners Done.
https://codereview.chromium.org/1284413004/diff/220001/Source/devtools/front_... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1284413004/diff/220001/Source/devtools/front_... Source/devtools/front_end/security/SecurityPanel.js:27: /** @type {!Map<string, !Object>} */ You can even create typedef for this object to check names and types of the fields inside.
https://codereview.chromium.org/1284413004/diff/220001/Source/devtools/front_... File Source/devtools/front_end/security/SecurityPanel.js (right): https://codereview.chromium.org/1284413004/diff/220001/Source/devtools/front_... Source/devtools/front_end/security/SecurityPanel.js:27: /** @type {!Map<string, !Object>} */ On 2015/08/19 at 22:51:49, dgozman wrote: > You can even create typedef for this object to check names and types of the fields inside. Is there a way to do this in one definition, or do I need to define the object properties of the values in a separate place?
On 2015/08/19 22:56:26, lgarron wrote: > https://codereview.chromium.org/1284413004/diff/220001/Source/devtools/front_... > File Source/devtools/front_end/security/SecurityPanel.js (right): > > https://codereview.chromium.org/1284413004/diff/220001/Source/devtools/front_... > Source/devtools/front_end/security/SecurityPanel.js:27: /** @type {!Map<string, > !Object>} */ > On 2015/08/19 at 22:51:49, dgozman wrote: > > You can even create typedef for this object to check names and types of the > fields inside. > > Is there a way to do this in one definition, or do I need to define the object > properties of the values in a separate place? Something like this should work: /** @type {!Map<string, !{securityState: !SecurityAgent.SecurityState, securityDetails: ?SecurityAgent.SecurityDetails}>} */ If you find yourself referencing the origin details type too often, add a typedef: /** @typedef {!{fields go here}} */ WebInspector.SecurityPanel.OriginDetails;
Ah, it seems I missed that (it's only one line under "arrays and objects" at http://usejsdoc.org/tags-type.html). That already looks great for this CL; thanks!
The CQ bit was unchecked by lgarron@chromium.org
The CQ bit was checked by lgarron@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org Link to the patchset: https://codereview.chromium.org/1284413004/#ps240001 (title: "Add type annotation for the Map.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1284413004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1284413004/240001
Message was sent while issue was closed.
Committed patchset #6 (id:240001) as https://src.chromium.org/viewvc/blink?view=rev&revision=200868 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
