Extensions: Implement ActorPolicyChecker::GlicApiCanActOnWeb() [chromium/src : main]
Yuheng Huang (Gerrit)
Yuheng Huang voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Theodore Olsauskas-Warren
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Theodore Olsauskas-Warren (Gerrit)
Theodore Olsauskas-Warren added 5 comments![]( "Open in Gerrit")
bool GlicApiCanActOnWeb() const;This name is hard to pass, and had to understand the comparison to CanActOnWeb, though I can't think of in immediately better name. We should add a comment explaining the semantics of this - e.g. "attempts to never return true when a later check might deny access".
bool ActuationDisabledForManagedUser(Profile& profile,I don't think we need this, we can replace with `!ActuationEnabledForManagedUser`
// TODO: Retrieve enterprise account tier information for more accurate check.nit: needs bug
if (enterprise_account) {nit: could just have the IsEnterpriseAccount check here.
if (has_management) {nit: Could just have IsBrowserManaged check here
Related details
- Yuheng Huang
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. |
Yuheng Huang (Gerrit)
Yuheng Huang added 1 comment![]( "Open in Gerrit")
bool GlicApiCanActOnWeb() const;This name is hard to pass, and had to understand the comparison to CanActOnWeb, though I can't think of in immediately better name. We should add a comment explaining the semantics of this - e.g. "attempts to never return true when a later check might deny access".
I agree it is confusing to have 2 similar functions with different implementation. Do you foresee this function will replace CanActOnWeb() at some point, maybe after more testing? Or you think we have to keep both for different purposes?
Related details
- Theodore Olsauskas-Warren
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. |
Yuheng Huang (Gerrit)
Yuheng Huang added 1 comment![]( "Open in Gerrit")
bool ActuationDisabledForManagedUser(Profile& profile,I don't think we need this, we can replace with `!ActuationEnabledForManagedUser`
So in the current implementation, ActuationEnabledForManagedUser will return false if there's not an override from a managed device because kGlicActorEnterprisePrefDefault is set to kDisabledByDefault by default[1]. I assume most Googlers have that override so they won't be affected? What about non-Google users?
Related details
- Theodore Olsauskas-Warren
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. |
Yuheng Huang (Gerrit)
Yuheng Huang added 5 comments![]( "Open in Gerrit")
bool GlicApiCanActOnWeb() const;Yuheng HuangThis name is hard to pass, and had to understand the comparison to CanActOnWeb, though I can't think of in immediately better name. We should add a comment explaining the semantics of this - e.g. "attempts to never return true when a later check might deny access".
I agree it is confusing to have 2 similar functions with different implementation. Do you foresee this function will replace CanActOnWeb() at some point, maybe after more testing? Or you think we have to keep both for different purposes?
Added some comments to clarify the difference of the new method.
bool ActuationDisabledForManagedUser(Profile& profile,Yuheng HuangI don't think we need this, we can replace with `!ActuationEnabledForManagedUser`
So in the current implementation, ActuationEnabledForManagedUser will return false if there's not an override from a managed device because kGlicActorEnterprisePrefDefault is set to kDisabledByDefault by default[1]. I assume most Googlers have that override so they won't be affected? What about non-Google users?
Done
// TODO: Retrieve enterprise account tier information for more accurate check.Yuheng Huangnit: needs bug
Done
nit: could just have the IsEnterpriseAccount check here.
Done
nit: Could just have IsBrowserManaged check here
Done
Related details
- Theodore Olsauskas-Warren
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. |
Theodore Olsauskas-Warren (Gerrit)
Theodore Olsauskas-Warren added 2 comments![]( "Open in Gerrit")
bool GlicApiCanActOnWeb() const;Yuheng HuangThis name is hard to pass, and had to understand the comparison to CanActOnWeb, though I can't think of in immediately better name. We should add a comment explaining the semantics of this - e.g. "attempts to never return true when a later check might deny access".
Yuheng HuangI agree it is confusing to have 2 similar functions with different implementation. Do you foresee this function will replace CanActOnWeb() at some point, maybe after more testing? Or you think we have to keep both for different purposes?
Added some comments to clarify the difference of the new method.
I think we will have to keep both until we can get up-to-date workspace tier information into the client, which is a problem I don't know the solution to - so atleast for sometime.
But once we have that, conceptually yes we should be able to join them.
// If the main Glic check has been split to no longer use the
// can_use_model_execution_features capability (see
// kGlicEligibilitySeparateAccountCapability), then that capability must be
// checked here. This is because actuation currently implements stricter
// account checks.
signin::IdentityManager* identity_manager =
IdentityManagerFactory::GetForProfile(profile_);
CHECK(identity_manager);
// `account_info` is empty if the user has not signed in.
auto can_use_model_execution_features =
identity_manager
->FindExtendedAccountInfoByAccountId(
identity_manager
->GetPrimaryAccountInfo(signin::ConsentLevel::kSignin)
.account_id)
.GetAccountCapabilities()
.can_use_model_execution_features();
if (can_use_model_execution_features != signin::Tribool::kTrue) {
return log_and_return(CanActOutcome::kNo,
CannotActReason::kAccountCapabilityIneligible);
}@ros...@google.com has a refactor in flight that might make this easier, checkout crrev.com/c/7953837
Related details
- Rosie Yang
- Yuheng Huang
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. |
Yuheng Huang (Gerrit)
Yuheng Huang added 2 comments![]( "Open in Gerrit")
bool GlicApiCanActOnWeb() const;Yuheng HuangThis name is hard to pass, and had to understand the comparison to CanActOnWeb, though I can't think of in immediately better name. We should add a comment explaining the semantics of this - e.g. "attempts to never return true when a later check might deny access".
Yuheng HuangI agree it is confusing to have 2 similar functions with different implementation. Do you foresee this function will replace CanActOnWeb() at some point, maybe after more testing? Or you think we have to keep both for different purposes?
Theodore Olsauskas-WarrenAdded some comments to clarify the difference of the new method.
I think we will have to keep both until we can get up-to-date workspace tier information into the client, which is a problem I don't know the solution to - so atleast for sometime.
But once we have that, conceptually yes we should be able to join them.
Acknowledged
// If the main Glic check has been split to no longer use the
// can_use_model_execution_features capability (see
// kGlicEligibilitySeparateAccountCapability), then that capability must be
// checked here. This is because actuation currently implements stricter
// account checks.
signin::IdentityManager* identity_manager =
IdentityManagerFactory::GetForProfile(profile_);
CHECK(identity_manager);
// `account_info` is empty if the user has not signed in.
auto can_use_model_execution_features =
identity_manager
->FindExtendedAccountInfoByAccountId(
identity_manager
->GetPrimaryAccountInfo(signin::ConsentLevel::kSignin)
.account_id)
.GetAccountCapabilities()
.can_use_model_execution_features();
if (can_use_model_execution_features != signin::Tribool::kTrue) {
return log_and_return(CanActOutcome::kNo,
CannotActReason::kAccountCapabilityIneligible);
}@ros...@google.com has a refactor in flight that might make this easier, checkout crrev.com/c/7953837
I think this CL could wait for the refactor one to land first.
Related details
- Rosie Yang
- Theodore Olsauskas-Warren
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. |
Yuheng Huang (Gerrit)
Yuheng Huang added 1 comment![]( "Open in Gerrit")
// If the main Glic check has been split to no longer use the
// can_use_model_execution_features capability (see
// kGlicEligibilitySeparateAccountCapability), then that capability must be
// checked here. This is because actuation currently implements stricter
// account checks.
signin::IdentityManager* identity_manager =
IdentityManagerFactory::GetForProfile(profile_);
CHECK(identity_manager);
// `account_info` is empty if the user has not signed in.
auto can_use_model_execution_features =
identity_manager
->FindExtendedAccountInfoByAccountId(
identity_manager
->GetPrimaryAccountInfo(signin::ConsentLevel::kSignin)
.account_id)
.GetAccountCapabilities()
.can_use_model_execution_features();
if (can_use_model_execution_features != signin::Tribool::kTrue) {
return log_and_return(CanActOutcome::kNo,
CannotActReason::kAccountCapabilityIneligible);
}Yuheng Huang@ros...@google.com has a refactor in flight that might make this easier, checkout crrev.com/c/7953837
I think this CL could wait for the refactor one to land first.
Done
Related details
- Rosie Yang
- Theodore Olsauskas-Warren
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Theodore Olsauskas-Warren (Gerrit)
Theodore Olsauskas-Warren voted Code-Review+1![]( "Open in Gerrit")
- Rosie Yang
- Yuheng Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Justin DeWitt
- Rosie Yang
- Yuheng Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin DeWitt (Gerrit)
Justin DeWitt added 1 comment![]( "Open in Gerrit")
// Whether the Glic API can act on Web. This is currently different than
// CanActOnWeb() in the following ways:why not just replace CanActOnWeb? This is just fixing a bug in our dogfooding logic which should not have consequences elsewhere.
Related details
- Rosie Yang
- Yuheng Huang
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. |
Yuheng Huang (Gerrit)
Yuheng Huang added 1 comment![]( "Open in Gerrit")
// Whether the Glic API can act on Web. This is currently different than
// CanActOnWeb() in the following ways:why not just replace CanActOnWeb? This is just fixing a bug in our dogfooding logic which should not have consequences elsewhere.
@sau...@google.com Can you provide more context why we need a new method here?
Related details
- Justin DeWitt
- Rosie Yang
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. |
Yuheng Huang (Gerrit)
Yuheng Huang added 1 comment![]( "Open in Gerrit")
// Whether the Glic API can act on Web. This is currently different than
// CanActOnWeb() in the following ways:Yuheng Huangwhy not just replace CanActOnWeb? This is just fixing a bug in our dogfooding logic which should not have consequences elsewhere.
@sau...@google.com Can you provide more context why we need a new method here?
The new method disables actuation on enterprise accounts (if not dogfood), so it could be too conservative. In the long term we should check enterprise accounts when tier information is available in Chrome. See previous discussion in https://chromium-review.googlesource.com/c/chromium/src/+/7922896/comment/c1391751_aeb1f91e/
Theodore Olsauskas-Warren (Gerrit)
Theodore Olsauskas-Warren added 1 comment![]( "Open in Gerrit")
// Whether the Glic API can act on Web. This is currently different than
// CanActOnWeb() in the following ways:Yuheng Huangwhy not just replace CanActOnWeb? This is just fixing a bug in our dogfooding logic which should not have consequences elsewhere.
Yuheng Huang@sau...@google.com Can you provide more context why we need a new method here?
The new method disables actuation on enterprise accounts (if not dogfood), so it could be too conservative. In the long term we should check enterprise accounts when tier information is available in Chrome. See previous discussion in https://chromium-review.googlesource.com/c/chromium/src/+/7922896/comment/c1391751_aeb1f91e/
>this is just fixing a bug in our dogfooding logic
Not quite, this also changes the treatment of workspace accounts to always return false, when they might return true for the existing check.
We have this awkward situation where we have G1 subscriber tier information, but _not_ workspace SKU information in the client. Thus the client cannot really know if BD is available, only that _the client is not blocking it_ (see various checks @ [1]).
For the web exposed API, the need is to be (almost) certain the user has BD access, so this new function is explicitly more defensive, preferring to return false for users that _might_ have access, but which cannot be determined here in the client.
Related details
- Justin DeWitt
- Rosie Yang
- Yuheng Huang
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. |
Justin DeWitt (Gerrit)
Justin DeWitt added 1 comment![]( "Open in Gerrit")
// Whether the Glic API can act on Web. This is currently different than
// CanActOnWeb() in the following ways:Yuheng Huangwhy not just replace CanActOnWeb? This is just fixing a bug in our dogfooding logic which should not have consequences elsewhere.
Yuheng Huang@sau...@google.com Can you provide more context why we need a new method here?
Theodore Olsauskas-WarrenThe new method disables actuation on enterprise accounts (if not dogfood), so it could be too conservative. In the long term we should check enterprise accounts when tier information is available in Chrome. See previous discussion in https://chromium-review.googlesource.com/c/chromium/src/+/7922896/comment/c1391751_aeb1f91e/
>this is just fixing a bug in our dogfooding logic
Not quite, this also changes the treatment of workspace accounts to always return false, when they might return true for the existing check.
We have this awkward situation where we have G1 subscriber tier information, but _not_ workspace SKU information in the client. Thus the client cannot really know if BD is available, only that _the client is not blocking it_ (see various checks @ [1]).
For the web exposed API, the need is to be (almost) certain the user has BD access, so this new function is explicitly more defensive, preferring to return false for users that _might_ have access, but which cannot be determined here in the client.
ok. Then can we just 1) adjust dogfooding logic in CanActOnWeb and 2) wrap CanActOnWeb in an additional check to reduce code copying
Related details
- Rosie Yang
- Yuheng Huang
Yuheng Huang (Gerrit)
Yuheng Huang added 1 comment![]( "Open in Gerrit")
// Whether the Glic API can act on Web. This is currently different than
// CanActOnWeb() in the following ways:Yuheng Huangwhy not just replace CanActOnWeb? This is just fixing a bug in our dogfooding logic which should not have consequences elsewhere.
Yuheng Huang@sau...@google.com Can you provide more context why we need a new method here?
Theodore Olsauskas-WarrenThe new method disables actuation on enterprise accounts (if not dogfood), so it could be too conservative. In the long term we should check enterprise accounts when tier information is available in Chrome. See previous discussion in https://chromium-review.googlesource.com/c/chromium/src/+/7922896/comment/c1391751_aeb1f91e/
Justin DeWitt>this is just fixing a bug in our dogfooding logic
Not quite, this also changes the treatment of workspace accounts to always return false, when they might return true for the existing check.
We have this awkward situation where we have G1 subscriber tier information, but _not_ workspace SKU information in the client. Thus the client cannot really know if BD is available, only that _the client is not blocking it_ (see various checks @ [1]).
For the web exposed API, the need is to be (almost) certain the user has BD access, so this new function is explicitly more defensive, preferring to return false for users that _might_ have access, but which cannot be determined here in the client.
ok. Then can we just 1) adjust dogfooding logic in CanActOnWeb and 2) wrap CanActOnWeb in an additional check to reduce code copying
I refactor ComputeActOnWebCapability() to take a boolean disable_for_enterprise, for glic api disable_for_enterprise=true, for the other use case disable_for_enterprise=false. It should fix the dogfood bug for both cases and reuse code. PTAL.
Related details
- Fred Shih
- Justin DeWitt
- Rosie Yang
- Theodore Olsauskas-Warren
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |