[ios] Implement profile-level client certificate provisioning [chromium/src : main]
Sébastien Lalancette (Gerrit)
Sébastien Lalancette added 5 comments![]( "Open in Gerrit")
ProfileSelection::kOwnInstanceInIncognito) {This is different than for other non-iOS platforms. Is that expected?
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/enterprise/client_certificates/certificate_provisioning_service_factory.cc;l=61
For the other platforms, this is the service creation pattern w.r.t. Profile types:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/profiles/profile_selections.h;l=75-84
Seems like `kNoInstanceInIncognito` is more inline with non-iOS?
I think you'll want to also override `ServiceIsCreatedWithBrowserContext` and have it return true. The idea is that the provisioning service watches the policy value and will start provisioning the client cert as soon as the policy becomes enabled. This is an optimization to have the certificate get created/loaded before it is needed for a connection (which the user will then see as a loading spinner).
By overriding `ServiceIsCreatedWithBrowserContext`, the service will be created whenever the profile objects gets created, and then the service will start monitoring the policy preference.
Also, if you do override `ServiceIsCreatedWithBrowserContext`, I would suggest also overriding `ServiceIsNULLWhileTesting` to reduce side-effects in other people's EarlGrey/Browser tests :-).
ProfileSelection::kOwnInstanceInIncognito) {Same comment as for the other factory.
CHECK(profile_);Do you actually need the Profile?
It's generally preferred to inject the dependencies you need from a Profile in the constructor instead of the whole Profile's pointer, to reduce coupling.
// TODO: flush stored certificatesAdd a bug for TODOs.
```suggestion
// TODO(crbug.com/XXXX): Flush stored certificates.
```
https://google.github.io/styleguide/cppguide.html#TODO_Comments
Related details
- Anatoli Hancharou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne added 1 comment![]( "Open in Gerrit")
I think you'll want to also override `ServiceIsCreatedWithBrowserContext` and have it return true. The idea is that the provisioning service watches the policy value and will start provisioning the client cert as soon as the policy becomes enabled. This is an optimization to have the certificate get created/loaded before it is needed for a connection (which the user will then see as a loading spinner).
By overriding `ServiceIsCreatedWithBrowserContext`, the service will be created whenever the profile objects gets created, and then the service will start monitoring the policy preference.
Also, if you do override `ServiceIsCreatedWithBrowserContext`, I would suggest also overriding `ServiceIsNULLWhileTesting` to reduce side-effects in other people's EarlGrey/Browser tests :-).
drive-by:
Those methods are not overridable when you inherit `ProfileKeyedServiceFactoryIOS`.
Instead you need to pass `ServiceCreation::kCreateWithProfile` (to force creation with the profile) and `TestingCreation::kNoServiceForTests` (to avoid creating the service for tests, and required if you use `ServiceCreation::kCreateWithProfile`).
Anyway, this is just a minor tweak to what suggested Sébastien (and I agree with his comments).
Related details
- Anatoli Hancharou
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anatoli Hancharou (Gerrit)
Anatoli Hancharou added 5 comments![]( "Open in Gerrit")
This is different than for other non-iOS platforms. Is that expected?
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/enterprise/client_certificates/certificate_provisioning_service_factory.cc;l=61For the other platforms, this is the service creation pattern w.r.t. Profile types:
https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/profiles/profile_selections.h;l=75-84Seems like `kNoInstanceInIncognito` is more inline with non-iOS?
Done
Sylvain DefresneI think you'll want to also override `ServiceIsCreatedWithBrowserContext` and have it return true. The idea is that the provisioning service watches the policy value and will start provisioning the client cert as soon as the policy becomes enabled. This is an optimization to have the certificate get created/loaded before it is needed for a connection (which the user will then see as a loading spinner).
By overriding `ServiceIsCreatedWithBrowserContext`, the service will be created whenever the profile objects gets created, and then the service will start monitoring the policy preference.
Also, if you do override `ServiceIsCreatedWithBrowserContext`, I would suggest also overriding `ServiceIsNULLWhileTesting` to reduce side-effects in other people's EarlGrey/Browser tests :-).
drive-by:
Those methods are not overridable when you inherit `ProfileKeyedServiceFactoryIOS`.
Instead you need to pass `ServiceCreation::kCreateWithProfile` (to force creation with the profile) and `TestingCreation::kNoServiceForTests` (to avoid creating the service for tests, and required if you use `ServiceCreation::kCreateWithProfile`).
Anyway, this is just a minor tweak to what suggested Sébastien (and I agree with his comments).
Done
Same comment as for the other factory.
Done
Do you actually need the Profile?
It's generally preferred to inject the dependencies you need from a Profile in the constructor instead of the whole Profile's pointer, to reduce coupling.
Done
Add a bug for TODOs.
```suggestion
// TODO(crbug.com/XXXX): Flush stored certificates.
```https://google.github.io/styleguide/cppguide.html#TODO_Comments
- Sylvain Defresne
- Sébastien Lalancette
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sébastien Lalancette (Gerrit)
Sébastien Lalancette voted Code-Review+1![]( "Open in Gerrit")
- Anatoli Hancharou
- Sylvain Defresne
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Sylvain Defresne (Gerrit)
Sylvain Defresne voted and added 1 comment![]( "Open in Gerrit")
Votes added by Sylvain Defresne
| Code-Review | +1 |
1 comment
Related details
- Anatoli Hancharou
- Gauthier Ambard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |