IWA: Implement entitlement enforcement for user-installed apps [chromium/src : main]
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 5 comments![]( "Open in Gerrit")
(as discussed offline, this requires splitting)
&web_app::IsolatedWebAppInstallSource::FromExternalPolicy)InstallWithSource(web_app::IsolatedWebAppInstallSource::FromExternalPolicy) isn't great for multiple reasons. We shouldn't do this.
IsolatedWebAppInstallSource::FromDevCommandLine(Why is this change needed? Are we sure it doesn't break any underlying assumptions?
defines += [ "ENABLE_SMART_CARD" ]Why is this needed? `runtime_data` knows nothing about the specifics of the smart card API.
void set_allow_all_user_installs_with_all_entitlements(bool allow_all) {That would be a -1 from me -- let's either update the affected tests or introduce a separate fake provider that returns true for any entitlement requests.
Related details
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
2 comments
IsolatedWebAppInstallSource::FromDevCommandLine(Why is this change needed? Are we sure it doesn't break any underlying assumptions?
It should not. Essentially this is about tests that do not use IWA test harness, and as such do not have auto-injected fake entitlements provider. Editing all of those by hand and adding entitlements would be a bit of a pain.
defines += [ "ENABLE_SMART_CARD" ]Why is this needed? `runtime_data` knows nothing about the specifics of the smart card API.
It's about the general effort of not compiling smart card specific stuff on systems that do not support the API, it's similar in many other places. Here it's about iffing-out smart card permissions policy related stuff if the browser does not support the API anyway.
Related details
- Andrew Rayskiy
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
return IwaKeyDistributionInfoProvider::GetInstanceForTesting()Oh, I didn't notice that too. It's even worse, so please don't :/
Related details
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 1 comment![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
1 comment
return IwaKeyDistributionInfoProvider::GetInstanceForTesting()Oh, I didn't notice that too. It's even worse, so please don't :/
I don't remember why I did that, frankly, probably a relic of one of the past approaches.
Related details
- Andrew Rayskiy
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted Commit-Queue+0![]( "Open in Gerrit")
| Commit-Queue | +0 |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 7 comments![]( "Open in Gerrit")
->InstallWithSource(As discussed offline, we'd better turn this into a proper policy install via the update server
allowed_app_1_url_info_ =As discussed offline, we'd better turn this into a proper policy install via the update server
ApplyEntitlements(const web_app::IwaPermissionsPolicyCache::CacheEntry& policy,This function is totally overloaded with various branches merged together; ideally this must only be called if the preconditions are fulfilled.
```
auto url_info = web_app::IsolatedWebAppUrlInfo::CreateFromSignedWebBundleId(
origin.web_bundle_id());
auto app_id = url_info.app_id();
if (!registrar.AppMatches(app_id, IsIsolatedApp()) {
return false;
}
if (registrar.AppMatches(app_id, IwaPolicy() | IwaDevMode()) {
return policy_as_is;
}
return ApplyEntitlements(profile, policy_as_is, url_info)
``` const bool is_managed =We don't do that here.
```
registrar.AppMAtches(app_id, WebAppFilter::<X>)
```
On top of that, this isn't a correct check; you should be checking the presence of a managed source and not the absence of user install source
ChromeIwaRuntimeDataProviderMixin::data_provider_ = nullptr;This looks weird AF. Why?
void SetAllowaAllUserInstallsWithAllEntitlements(bool allow_all);I'm still strictly opposed to this as discussed offline. There are not that many API tests on the //chrome level overall, and they're all relatively easy to migrate to proper user install allowlist with relevant entitlements.
By using the wildcard approach, we basically scatter the problem around: instead of having both the installation routine as well as the permissions policy routine rely on the user install allowlist by calling `GetUserInstallAllowlistData()`, installation in existing tests will be guarded by `AddTrustedWebBundleIdForTesting()` (which is an integral part of the `->Install()` call of the `IsolatedWebAppBuilder`, and the PP checks will fetch this custom allowlist stuff.
Related details
- Zgroza (Luke) Klimek
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
void SetAllowaAllUserInstallsWithAllEntitlements(bool allow_all);I'm still strictly opposed to this as discussed offline. There are not that many API tests on the //chrome level overall, and they're all relatively easy to migrate to proper user install allowlist with relevant entitlements.
By using the wildcard approach, we basically scatter the problem around: instead of having both the installation routine as well as the permissions policy routine rely on the user install allowlist by calling `GetUserInstallAllowlistData()`, installation in existing tests will be guarded by `AddTrustedWebBundleIdForTesting()` (which is an integral part of the `->Install()` call of the `IsolatedWebAppBuilder`, and the PP checks will fetch this custom allowlist stuff.
Have around ~40 entries overall :) Just launch gemini-cli and come back in an hour
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 2 comments![]( "Open in Gerrit")
#if !BUILDFLAG(IS_ANDROID)Zgroza (Luke) Klimek?
Hm, probably some rebase accident at some point. Removed.
ChromeIwaRuntimeDataProviderMixin::data_provider_ = nullptr;This looks weird AF. Why?
Because derived constructor is ran first, which causes dangling raw_ptr error when base constructor is called (this is owned by the unique_ptr in the derived class).
Related details
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
ChromeIwaRuntimeDataProviderMixin::data_provider_ = nullptr;Zgroza (Luke) KlimekThis looks weird AF. Why?
Because derived constructor is ran first, which causes dangling raw_ptr error when base constructor is called (this is owned by the unique_ptr in the derived class).
I see, fair. Let's then do the following:
```
1. Make the parent class own the unique_ptr
2. Expose a T* getter on the parent class
3. Do a static_cast<DataProvider*> in the child class
```
Related details
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 6 comments![]( "Open in Gerrit")
As discussed offline, we'd better turn this into a proper policy install via the update server
Done
As discussed offline, we'd better turn this into a proper policy install via the update server
Done
ApplyEntitlements(const web_app::IwaPermissionsPolicyCache::CacheEntry& policy,This function is totally overloaded with various branches merged together; ideally this must only be called if the preconditions are fulfilled.
```
auto url_info = web_app::IsolatedWebAppUrlInfo::CreateFromSignedWebBundleId(
origin.web_bundle_id());
auto app_id = url_info.app_id();
if (!registrar.AppMatches(app_id, IsIsolatedApp()) {
return false;
}if (registrar.AppMatches(app_id, IwaPolicy() | IwaDevMode()) {
return policy_as_is;
}
return ApplyEntitlements(profile, policy_as_is, url_info)
```
Done
We don't do that here.
```
registrar.AppMAtches(app_id, WebAppFilter::<X>)
```On top of that, this isn't a correct check; you should be checking the presence of a managed source and not the absence of user install source
Done
ChromeIwaRuntimeDataProviderMixin::data_provider_ = nullptr;Zgroza (Luke) KlimekThis looks weird AF. Why?
Andrew RayskiyBecause derived constructor is ran first, which causes dangling raw_ptr error when base constructor is called (this is owned by the unique_ptr in the derived class).
I see, fair. Let's then do the following:
```
1. Make the parent class own the unique_ptr
2. Expose a T* getter on the parent class
3. Do a static_cast<DataProvider*> in the child class
```
Done
void SetAllowaAllUserInstallsWithAllEntitlements(bool allow_all);Andrew RayskiyI'm still strictly opposed to this as discussed offline. There are not that many API tests on the //chrome level overall, and they're all relatively easy to migrate to proper user install allowlist with relevant entitlements.
By using the wildcard approach, we basically scatter the problem around: instead of having both the installation routine as well as the permissions policy routine rely on the user install allowlist by calling `GetUserInstallAllowlistData()`, installation in existing tests will be guarded by `AddTrustedWebBundleIdForTesting()` (which is an integral part of the `->Install()` call of the `IsolatedWebAppBuilder`, and the PP checks will fetch this custom allowlist stuff.
Have around ~40 entries overall :) Just launch gemini-cli and come back in an hour
- Andrew Rayskiy
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
void SetAllowaAllUserInstallsWithAllEntitlements(bool allow_all);Andrew RayskiyI'm still strictly opposed to this as discussed offline. There are not that many API tests on the //chrome level overall, and they're all relatively easy to migrate to proper user install allowlist with relevant entitlements.
By using the wildcard approach, we basically scatter the problem around: instead of having both the installation routine as well as the permissions policy routine rely on the user install allowlist by calling `GetUserInstallAllowlistData()`, installation in existing tests will be guarded by `AddTrustedWebBundleIdForTesting()` (which is an integral part of the `->Install()` call of the `IsolatedWebAppBuilder`, and the PP checks will fetch this custom allowlist stuff.
Zgroza (Luke) KlimekHave around ~40 entries overall :) Just launch gemini-cli and come back in an hour
Hmm, sure. PTAL at it now
Or, as I said, let's do a simple (a really simple!!!) intermediate step with checking the `GetTrustedWebBundleIdsForTesting()` in the meantime and then migrate the rest incrementally.
(IT WILL BE EASIER!!!!!!)
Related details
- Zgroza (Luke) Klimek
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 8 comments![]( "Open in Gerrit")
GetBaselinePermissionsPolicyForIsolatedWebApp(Let's not wrap this as an optional -- this no longer makes sense
if (!entitlement) {Pretty much all if-s here are nested up to a length of three. I'm sure we can do better!
```
bool is_feature_allowed = [&] {
if (!entitlement) {
return !network::IsPermissionsPolicyFeatureGuardedByIsolatedContext(
entry.feature);
}
return allowlist_data && IsEntitlementGiven(allowlist_data, entitlement, version);
}();
```
Profile* profile = Profile::FromBrowserContext(browser_context);I'm a hater of early returns, so let's be a bit more humane.
```
if (!content::AreIsolatedWebAppsEnabled(browser_context)) {
return {};
}
ASSIGN_OR_RETURN(web_app::IwaOrigin origin,
web_app::IwaOrigin::Create(iwa_origin.GetURL()),
[](auto) { return {}; });
auto* policy = web_app::IwaPermissionsPolicyCacheFactory::GetForProfile(...)->GetPolicy(...);
if (!policy) {
return {};
}
...
const web_app::WebAppRegistrar& registrar = web_app::WebAppProvider::GetForWebApps(...)->registrar_unsafe();
const auto* iwa = registrar.GetAppById(app_id, WebAppFilter::IsIsolatedApp());
if (!iwa) {
return {};
}
if (registrar.AppMatches(<complex_condition>) || ForTest()) {
return as_is;
}return Apply()
```
blink::features::kSmartCard,It's already enabled by default -- just remove this
ditto printing
#if defined(ENABLE_SMART_CARD)I still have no idea why this define is so valuable. It basically completely disrupts the reading flow and turns a two-line decl into a four-line decl so that it falls completely out of line.
set.entitlements.push_back(I remember there once used to be a thing that allowed you to simplify manual operations. Something like `for`? Not sure.
EXPECT_TRUE(IsFeatureAllowed(frame, "direct-sockets"));ditto here
if (bypass_entitlements &&Let's reuse the same AddTrustedWebBundleIdForTesting from above :) As I said, it will be easier to migrate everything altogether.
Related details
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 1 comment![]( "Open in Gerrit")
#if defined(ENABLE_SMART_CARD)I still have no idea why this define is so valuable. It basically completely disrupts the reading flow and turns a two-line decl into a four-line decl so that it falls completely out of line.
`network::mojom::PermissionsPolicyFeature::kSmartCard` is available only when SmartCard feature is available. Which is only on ChromeOS. So it has to remain here.
Related details
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek added 8 comments![]( "Open in Gerrit")
Let's not wrap this as an optional -- this no longer makes sense
Yeah, it was in progress—just You commented on a rebase patchset.
Pretty much all if-s here are nested up to a length of three. I'm sure we can do better!
```
bool is_feature_allowed = [&] {
if (!entitlement) {
return !network::IsPermissionsPolicyFeatureGuardedByIsolatedContext(
entry.feature);
}
return allowlist_data && IsEntitlementGiven(allowlist_data, entitlement, version);
}();
```
Done
Profile* profile = Profile::FromBrowserContext(browser_context);I'm a hater of early returns, so let's be a bit more humane.
```
if (!content::AreIsolatedWebAppsEnabled(browser_context)) {
return {};
}ASSIGN_OR_RETURN(web_app::IwaOrigin origin,
web_app::IwaOrigin::Create(iwa_origin.GetURL()),
[](auto) { return {}; });auto* policy = web_app::IwaPermissionsPolicyCacheFactory::GetForProfile(...)->GetPolicy(...);
if (!policy) {
return {};
}...
const web_app::WebAppRegistrar& registrar = web_app::WebAppProvider::GetForWebApps(...)->registrar_unsafe();
const auto* iwa = registrar.GetAppById(app_id, WebAppFilter::IsIsolatedApp());
if (!iwa) {
return {};
}if (registrar.AppMatches(<complex_condition>) || ForTest()) {
return as_is;
}return Apply()
```
Done
It's already enabled by default -- just remove this
ditto printing
Printing yes, done. But smart card is another thing, as for now the sets of platforms that:
1. have its code compiled (`ENABLE_SMART_CARD`)
2. have this launched to stable
are the same ({ChromeOS}). However if we go to prototyping on any other platforms, we'll need to re-add this to all tests. So I'll keep the flag here and remove only when we just remove the flag altogether or launch to stable everywhere, whichever comes first.
set.entitlements.push_back(Zgroza (Luke) KlimekI remember there once used to be a thing that allowed you to simplify manual operations. Something like `for`? Not sure.
Done
EXPECT_TRUE(IsFeatureAllowed(frame, "direct-sockets"));Zgroza (Luke) Klimekditto here
Done
void SetAllowaAllUserInstallsWithAllEntitlements(bool allow_all);I'm still strictly opposed to this as discussed offline. There are not that many API tests on the //chrome level overall, and they're all relatively easy to migrate to proper user install allowlist with relevant entitlements.
By using the wildcard approach, we basically scatter the problem around: instead of having both the installation routine as well as the permissions policy routine rely on the user install allowlist by calling `GetUserInstallAllowlistData()`, installation in existing tests will be guarded by `AddTrustedWebBundleIdForTesting()` (which is an integral part of the `->Install()` call of the `IsolatedWebAppBuilder`, and the PP checks will fetch this custom allowlist stuff.
Zgroza (Luke) KlimekHave around ~40 entries overall :) Just launch gemini-cli and come back in an hour
Andrew RayskiyHmm, sure. PTAL at it now
Or, as I said, let's do a simple (a really simple!!!) intermediate step with checking the `GetTrustedWebBundleIdsForTesting()` in the meantime and then migrate the rest incrementally.
(IT WILL BE EASIER!!!!!!)
Done
Let's reuse the same AddTrustedWebBundleIdForTesting from above :) As I said, it will be easier to migrate everything altogether.
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 3 comments![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Code-Review | +1 |
3 comments
LGTM
if (!cache) {Guaranteed by content::AreIsolatedWebAppsEnabled
if (!provider) {Guaranteed by content::AreIsolatedWebAppsEnabled
Related details
- Zgroza (Luke) Klimek
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 3 comments![]( "Open in Gerrit")
#include "base/containers/flat_set.h"nit: not needed
bool is_feature_allowed = [&] {nit: since this function depends on a single param (entry.feature), it could be structured like
```
bool is_feature_allowed = [&](feature) {
if (auto entitlement = web_app::GetEntitlementForFeature(feature)) {
return allowlist_data && IsEntitlementGranted(*allowlist_data, app_version, *entitlement);
}
return !network::IsPermissionsPolicyFeatureGuardedByIsolatedContext(
entry.feature);
};
for (const auto& entry: policy) {
if (is_feature_allowed(entry.feature)) {
...
}
}
```Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 5 comments![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
5 comments
#include "base/containers/flat_set.h"Zgroza (Luke) Klimeknit: not needed
Done
#include <optional>Zgroza (Luke) Klimeknit: not needed
Done
nit: since this function depends on a single param (entry.feature), it could be structured like
```
bool is_feature_allowed = [&](feature) {
if (auto entitlement = web_app::GetEntitlementForFeature(feature)) {
return allowlist_data && IsEntitlementGranted(*allowlist_data, app_version, *entitlement);
}
return !network::IsPermissionsPolicyFeatureGuardedByIsolatedContext(
entry.feature);
};for (const auto& entry: policy) {
if (is_feature_allowed(entry.feature)) {
...
}
}
```
Done
if (!cache) {Zgroza (Luke) KlimekGuaranteed by content::AreIsolatedWebAppsEnabled
Hm, I kinda like to ensure those anyway just in case. But probably this is indeed unnecessary here.
if (!provider) {Zgroza (Luke) KlimekGuaranteed by content::AreIsolatedWebAppsEnabled
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted and added 1 comment![]( "Open in Gerrit")
Votes added by Andrew Rayskiy
| Code-Review | +1 |
1 comment
const PermissionsPolicyCacheEntry* policy =Btw, I just realized that I'm missing something. Why is the entitlement application logic enforced here and not in the PermissionsPolicyCache itself?
Related details
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek abandoned this change.
Zgroza (Luke) Klimek abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 1 comment![]( "Open in Gerrit")
CHECK(iwa_origin.has_value());This is gonna be an epic crash if someone decides to navigate to `isolated-app://meow`.
Related details
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 2 comments![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
2 comments
Btw, I just realized that I'm missing something. Why is the entitlement application logic enforced here and not in the PermissionsPolicyCache itself?
Hm, fair point, logging there also would be easy out of the box.
This is gonna be an epic crash if someone decides to navigate to `isolated-app://meow`.
*meow*
(actually no, checked—it just does nothing) (but still, let me change that)
Related details
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy added 4 comments![]( "Open in Gerrit")
CHECK(iwa_origin.has_value());Zgroza (Luke) KlimekThis is gonna be an epic crash if someone decides to navigate to `isolated-app://meow`.
*meow*
(actually no, checked—it just does nothing) (but still, let me change that)
Ah, alright, that's because we're lucky and this func is only called if there's a response provided for this URL.
auto* cache = IwaPermissionsPolicyCacheFactory::GetForProfile(profile());Just use cache directly, cmon. It's even used directly in the same file already (not to mention that the throttle is guarded by content::AreIsolatedWebAppsEnabled())
if (registrar.AppMatches(I guess I already asked this, but... wouldn't it make more sense to have a flag on each filtered/unfiltered pair telling whether filtering is needed overall (even just an `std::optional<base::Version> version_if_filtering_needed_` will suffice)? So that we don't have to re-check this bunch of conditions every time?
void AddDeferredConsoleMessage(blink::mojom::ConsoleMessageLevel level,Why not call the public method on the RFH during `WillProcessResponse()` though? This will spare you some additional reviewers for sure :)
Related details
- Zgroza (Luke) Klimek
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. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Zgroza (Luke) Klimek
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. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted and added 3 comments![]( "Open in Gerrit")
Votes added by Zgroza (Luke) Klimek
| Commit-Queue | +1 |
3 comments
auto* cache = IwaPermissionsPolicyCacheFactory::GetForProfile(profile());Just use cache directly, cmon. It's even used directly in the same file already (not to mention that the throttle is guarded by content::AreIsolatedWebAppsEnabled())
Done
I guess I already asked this, but... wouldn't it make more sense to have a flag on each filtered/unfiltered pair telling whether filtering is needed overall (even just an `std::optional<base::Version> version_if_filtering_needed_` will suffice)? So that we don't have to re-check this bunch of conditions every time?
Done
void AddDeferredConsoleMessage(blink::mojom::ConsoleMessageLevel level,Why not call the public method on the RFH during `WillProcessResponse()` though? This will spare you some additional reviewers for sure :)
- Andrew Rayskiy
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Andrew Rayskiy (Gerrit)
Andrew Rayskiy voted Code-Review+1![]( "Open in Gerrit")
- Zgroza (Luke) Klimek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zgroza (Luke) Klimek (Gerrit)
Zgroza (Luke) Klimek voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
IWA: Implement entitlement enforcement for user-installed apps
This change implements the mechanism to restrict access to powerful APIs
in user-installed Isolated Web Apps (IWAs) based on an allowlist of
entitlements.
Key changes:
- IwaPermissionsPolicyCache::SetPolicy now filters an IWA's Permissions
Policy. Features guarded by IsolatedContext (e.g., Direct Sockets,
Controlled Frame, Smart Card) require an explicit entitlement granted
to the app's Web Bundle ID.
- Managed installs (via enterprise policy) and Developer Mode installs
bypass these entitlement checks as they are implicitly trusted.
- Added a testing bypass mechanism and used it in
IsolatedWebAppBuilder::Install to keep existing tests passing.
- Refactored ChromeContentBrowserClient to delegate IWA permissions
policy resolution to the IWA-specific part.
- Entitlement violations are logged to dev console on navigation as
warnings in the throttle.
- M chrome/browser/chrome_content_browser_client.cc
- M chrome/browser/web_applications/BUILD.gn
- M chrome/browser/web_applications/isolated_web_apps/chrome_content_browser_client_isolated_web_apps_part.cc
- M chrome/browser/web_applications/isolated_web_apps/chrome_content_browser_client_isolated_web_apps_part.h
- A chrome/browser/web_applications/isolated_web_apps/isolated_web_app_entitlements_browsertest.cc
- M chrome/browser/web_applications/isolated_web_apps/isolated_web_app_throttle.cc
- M chrome/browser/web_applications/isolated_web_apps/isolated_web_app_throttle.h
- M chrome/browser/web_applications/isolated_web_apps/isolated_web_app_throttle_unittest.cc
- M chrome/browser/web_applications/isolated_web_apps/isolated_web_app_trust_checker.cc
- M chrome/browser/web_applications/isolated_web_apps/isolated_web_app_trust_checker.h
- M chrome/browser/web_applications/isolated_web_apps/iwa_permissions_policy_cache.cc
- M chrome/browser/web_applications/isolated_web_apps/iwa_permissions_policy_cache.h
Code-Review: +1 by Andrew Rayskiy
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |