Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2307373003:
Show extension provided certificates in chrome://settings/certificates (Closed)
|
Created:
4 years, 3 months ago by Ivan Šandrk Modified:
4 years, 3 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow extension provided certificates in chrome://settings/certificates
Extension provided certificates currently aren't shown on the certificates page, and this is bad from a security standpoint. This CL addresses the problem.
BUG=532038
Committed: https://crrev.com/20c70a26a98d340d851b2cb12bf6392aae1cda04
Cr-Commit-Position: refs/heads/master@{#420469}
Patch Set 1 #Patch Set 2 : Fix trybot errors #
Total comments: 4
Patch Set 3 : Fixed broken tests, nits #
Total comments: 4
Patch Set 4 : Pass CertificateProvider around instead of BrowserContext #
Total comments: 4
Patch Set 5 : Matt's nits #
Total comments: 5
Patch Set 6 : thestig's nits #
Messages
Total messages: 41 (22 generated)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
isandrk@chromium.org changed reviewers: + emaxx@chromium.org
Hi Max, PTAL! Thanks, Ivan
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2307373003/diff/20001/chrome/browser/certific... File chrome/browser/certificate_manager_model.cc (right): https://codereview.chromium.org/2307373003/diff/20001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:177: if (std::find(extension_cert_list_.begin(), extension_cert_list_.end(), You could use base::ContainsValue for this check. https://codereview.chromium.org/2307373003/diff/20001/chrome/browser/certific... File chrome/browser/certificate_manager_model.h (right): https://codereview.chromium.org/2307373003/diff/20001/chrome/browser/certific... chrome/browser/certificate_manager_model.h:178: base::WeakPtrFactory<CertificateManagerModel> weak_ptr_factory_; nit: #include "base/memory/weak_ptr.h"
https://codereview.chromium.org/2307373003/diff/20001/chrome/browser/certific... File chrome/browser/certificate_manager_model.cc (right): https://codereview.chromium.org/2307373003/diff/20001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:177: if (std::find(extension_cert_list_.begin(), extension_cert_list_.end(), On 2016/09/06 16:17:03, emaxx wrote: > You could use base::ContainsValue for this check. Done. https://codereview.chromium.org/2307373003/diff/20001/chrome/browser/certific... File chrome/browser/certificate_manager_model.h (right): https://codereview.chromium.org/2307373003/diff/20001/chrome/browser/certific... chrome/browser/certificate_manager_model.h:178: base::WeakPtrFactory<CertificateManagerModel> weak_ptr_factory_; On 2016/09/06 16:17:03, emaxx wrote: > nit: #include "base/memory/weak_ptr.h" Done.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
isandrk@chromium.org changed reviewers: + mattm@chromium.org
Hi Matt, Based on my cs-fu skills you should be familiar with this codebase, could you take a look at my changes? Thanks, Ivan
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2307373003/diff/40001/chrome/browser/certific... File chrome/browser/certificate_manager_model.cc (right): https://codereview.chromium.org/2307373003/diff/40001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:102: browser_context); don't think browser_context is guaranteed to be valid here. I would do the createCertificateProvider in CertificateManagerModel::Create and then pass it through the calls instead of the BrowserContext. https://codereview.chromium.org/2307373003/diff/40001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:141: observer_->CertificatesRefreshed(); a little worried about CertificatesRefreshed being called twice now. If both happen quickly then it might just be jarring/inefficient, and if they happen farther apart it could be more confusing (would seem like it was loaded, you start doing stuff, but then more certs magically appear.)
https://codereview.chromium.org/2307373003/diff/40001/chrome/browser/certific... File chrome/browser/certificate_manager_model.cc (right): https://codereview.chromium.org/2307373003/diff/40001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:102: browser_context); On 2016/09/06 22:41:04, mattm wrote: > don't think browser_context is guaranteed to be valid here. I would do the > createCertificateProvider in CertificateManagerModel::Create and then pass it > through the calls instead of the BrowserContext. Done. https://codereview.chromium.org/2307373003/diff/40001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:141: observer_->CertificatesRefreshed(); On 2016/09/06 22:41:04, mattm wrote: > a little worried about CertificatesRefreshed being called twice now. If both > happen quickly then it might just be jarring/inefficient, and if they happen > farther apart it could be more confusing (would seem like it was loaded, you > start doing stuff, but then more certs magically appear.) The initial idea was to call it just once, but the problem is that extensions do not always play nicely when returning a list of certificates. On a normal day they take a couple of seconds to return the list, and if they misbehave they may take up to 5 minutes (hard limit), and we don't want to block platform provided certificates from showing in the list because of that. Note that extensions are third-party software.
lgtm with nits https://codereview.chromium.org/2307373003/diff/60001/chrome/browser/certific... File chrome/browser/certificate_manager_model.h (right): https://codereview.chromium.org/2307373003/diff/60001/chrome/browser/certific... chrome/browser/certificate_manager_model.h:17: #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h" can you forward declare CertificateProvider and move this include to the .cc? https://codereview.chromium.org/2307373003/diff/60001/chrome/browser/certific... chrome/browser/certificate_manager_model.h:180: std::unique_ptr<chromeos::CertificateProvider> certificate_provider_; I'd suggest %s/certificate_provider/extension_certificate_provider/
https://codereview.chromium.org/2307373003/diff/60001/chrome/browser/certific... File chrome/browser/certificate_manager_model.h (right): https://codereview.chromium.org/2307373003/diff/60001/chrome/browser/certific... chrome/browser/certificate_manager_model.h:17: #include "chrome/browser/chromeos/certificate_provider/certificate_provider.h" On 2016/09/09 01:34:05, mattm wrote: > can you forward declare CertificateProvider and move this include to the .cc? Done. https://codereview.chromium.org/2307373003/diff/60001/chrome/browser/certific... chrome/browser/certificate_manager_model.h:180: std::unique_ptr<chromeos::CertificateProvider> certificate_provider_; On 2016/09/09 01:34:04, mattm wrote: > I'd suggest %s/certificate_provider/extension_certificate_provider/ Good suggestion, I agree.
isandrk@chromium.org changed reviewers: + thestig@chromium.org
Hi Lei, PTAL! Thanks, Ivan
Hi thestig, polite ping! Thanks, Ivan
lgtm with some nits. Erm, I accidentally marked the previous review email as read. Next time, please ping me sooner or ask an alternate reviewer, rather than wait 2 weeks for a review. https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... File chrome/browser/certificate_manager_model.cc (right): https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:168: for (auto cert : extension_cert_list_) { auto& ? https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:189: } should the next if statement be an "else if" now?
On 2016/09/22 17:13:03, Lei Zhang wrote: > Erm, I accidentally marked the previous review email as read. Next time, please > ping me sooner or ask an alternate reviewer, rather than wait 2 weeks for a > review. Right! https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... File chrome/browser/certificate_manager_model.cc (right): https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:168: for (auto cert : extension_cert_list_) { On 2016/09/22 17:13:03, Lei Zhang wrote: > auto& ? extension_cert_list_ is a std::vector<scoped_refptr<X509Certificate>>, so a cert here would be scoped_refptr<...>. I don't know about best practices, is it good to also make it "auto&"? Maybe that avoids creation of a new scoped_refptr and is therefore faster. Sound logic? https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:189: } On 2016/09/22 17:13:03, Lei Zhang wrote: > should the next if statement be an "else if" now? You are right, if the first if is true then the second one cannot be true. I guess another good practice I need to learn.
https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... File chrome/browser/certificate_manager_model.cc (right): https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... chrome/browser/certificate_manager_model.cc:168: for (auto cert : extension_cert_list_) { On 2016/09/22 17:28:09, Ivan Šandrk wrote: > On 2016/09/22 17:13:03, Lei Zhang wrote: > > auto& ? > > extension_cert_list_ is a std::vector<scoped_refptr<X509Certificate>>, so a cert > here would be scoped_refptr<...>. I don't know about best practices, is it good > to also make it "auto&"? Maybe that avoids creation of a new scoped_refptr and > is therefore faster. Sound logic? I generally do const auto& or auto& unless: a) it's a pointer, then auto* b) it's POD, then auto
On 2016/09/22 17:32:19, Lei Zhang wrote: > https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... > File chrome/browser/certificate_manager_model.cc (right): > > https://codereview.chromium.org/2307373003/diff/80001/chrome/browser/certific... > chrome/browser/certificate_manager_model.cc:168: for (auto cert : > extension_cert_list_) { > On 2016/09/22 17:28:09, Ivan Šandrk wrote: > > On 2016/09/22 17:13:03, Lei Zhang wrote: > > > auto& ? > > > > extension_cert_list_ is a std::vector<scoped_refptr<X509Certificate>>, so a > cert > > here would be scoped_refptr<...>. I don't know about best practices, is it > good > > to also make it "auto&"? Maybe that avoids creation of a new scoped_refptr and > > is therefore faster. Sound logic? > > I generally do const auto& or auto& unless: > > a) it's a pointer, then auto* > b) it's POD, then auto Great, thanks!
On 2016/09/22 17:34:46, Ivan Šandrk wrote: > Great, thanks! https://google.github.io/styleguide/cppguide.html#auto also has some good suggestions.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from emaxx@chromium.org, mattm@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2307373003/#ps100001 (title: "thestig's nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Show extension provided certificates in chrome://settings/certificates Extension provided certificates currently aren't shown on the certificates page, and this is bad from a security standpoint. This CL addresses the problem. BUG=532038 ========== to ========== Show extension provided certificates in chrome://settings/certificates Extension provided certificates currently aren't shown on the certificates page, and this is bad from a security standpoint. This CL addresses the problem. BUG=532038 Committed: https://crrev.com/20c70a26a98d340d851b2cb12bf6392aae1cda04 Cr-Commit-Position: refs/heads/master@{#420469} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/20c70a26a98d340d851b2cb12bf6392aae1cda04 Cr-Commit-Position: refs/heads/master@{#420469} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
