Enable screenshot control based on enterprise rule and data controls [chromium/src : main]
Alex Chen (Gerrit)
Alex Chen abandoned this change.
Alex Chen abandoned this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Chen (Gerrit)
Message from Alex Chen![]( "Open in Gerrit")
Set Ready For Review
Related details
- Alex Chen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau added 2 comments![]( "Open in Gerrit")
public class ScreenshotProtectionController implements TabModelObserver, DestroyObserver {I'm still not sure we need ani JNI for this considering all the tab observer methods you have in TabModelObserver. What do you specifically need from `DataProtectionNavigationController` that you can't do in Java?
if (model.isIncognitoBranded()) {I don't think this should be here, won't this mean you never block screenshots in incognito? DC is supposed to be able to do that.
Related details
- Alex Chen
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. |
Alex Chen (Gerrit)
Alex Chen added 2 comments![]( "Open in Gerrit")
public class ScreenshotProtectionController implements TabModelObserver, DestroyObserver {I'm still not sure we need ani JNI for this considering all the tab observer methods you have in TabModelObserver. What do you specifically need from `DataProtectionNavigationController` that you can't do in Java?
Registering the `setCurrentTabState` cb with `DataProtectionNavigationController` allows this class to be notified, on each navigation commit, if either cloud DLP or DC determines that the newly committed page should not have screenshots allowed. AFAIK blocking navs and getting DLP/DC are not available in Java. This class should only take that notif and control the state of the window property for protecting screenshots.
if (model.isIncognitoBranded()) {I don't think this should be here, won't this mean you never block screenshots in incognito? DC is supposed to be able to do that.
Unlike desktop, by default, clank does not allow any screenshots in incognito at all. Will add a comment explaining that's why we don't need to observe here
Related details
- Dominique Fauteux-Chapleau
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. |
Alex Chen (Gerrit)
Alex Chen added 1 comment![]( "Open in Gerrit")
synced offline, alex to try java navigation observer and only 1 jni interface for "isscreenshotallowed(tab)". concern is race conditions if both dp nav and java nav are listening for the same nav commit event
Related details
- Alex Chen
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. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau added 1 comment![]( "Open in Gerrit")
Removing myself from the attention set for now until you test out what we discussed
Related details
- Alex Chen
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. |
Alex Chen (Gerrit)
Alex Chen added 2 comments![]( "Open in Gerrit")
Removing myself from the attention set for now until you test out what we discussed
Done
public class ScreenshotProtectionController implements TabModelObserver, DestroyObserver {Alex ChenI'm still not sure we need ani JNI for this considering all the tab observer methods you have in TabModelObserver. What do you specifically need from `DataProtectionNavigationController` that you can't do in Java?
Registering the `setCurrentTabState` cb with `DataProtectionNavigationController` allows this class to be notified, on each navigation commit, if either cloud DLP or DC determines that the newly committed page should not have screenshots allowed. AFAIK blocking navs and getting DLP/DC are not available in Java. This class should only take that notif and control the state of the window property for protecting screenshots.
Tried a navigation based approach, see https://chromium-review.git.corp.google.com/c/chromium/src/+/8044884
Related details
- Dominique Fauteux-Chapleau
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau added 3 comments![]( "Open in Gerrit")
boolean enterpriseObserveSelectTab = false;Considering the policies are dynamic, does making this check here really make sense? IIUC this constructor won't be called on every new tab being opened or every navigation, so this could become a gap.
if (!mAllowIncognitoScreenshot) {This is an odd condition to check outside of a `currentTab.isIncognito()` branch. Why can't we run this unconditionally like the previous version of the function?
if (mAllowIncognitoScreenshot) {
return false;
}Same comment for this check, having `isIncognitoBranded()` not checked at all doesn't seem right here.
Related details
- Alex Chen
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. |
Alex Chen (Gerrit)
Alex Chen added 4 comments![]( "Open in Gerrit")
Sorry its gotten a bit large, its not clear how I could split this.
boolean enterpriseObserveSelectTab = false;Considering the policies are dynamic, does making this check here really make sense? IIUC this constructor won't be called on every new tab being opened or every navigation, so this could become a gap.
This is a good point that applies to the code below as well -- `shouldObserveSelectTab(` is also called once on browser startup and does not refresh for policy updates.
The newest patchset observes when the profile's policies change, and re-initializes the controller when they do.
This is an odd condition to check outside of a `currentTab.isIncognito()` branch. Why can't we run this unconditionally like the previous version of the function?
I will add a comment for this. If incog screenshot is allowed, we never need to set screenshot state, so it is skipped. Had updateScreenshot been unconditional, the screen would be protected when in an incognito Chrome Custom Tab AND the incognito screenshot feature is turned on.
if (mAllowIncognitoScreenshot) {
return false;
}Same comment for this check, having `isIncognitoBranded()` not checked at all doesn't seem right here.
Perhaps renaming the variable would make this easier to read -- I made it `mForceAllowIncognitoScreenshot`
Related details
- Dominique Fauteux-Chapleau
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. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau added 5 comments![]( "Open in Gerrit")
if (profile != null && !profile.isIncognitoBranded()) {I'm not sure I understand this condition, won't that skip policy updates on incognito tabs? What about the DC policy being updated in those tabs?
if (!mAllowIncognitoScreenshot) {Alex ChenThis is an odd condition to check outside of a `currentTab.isIncognito()` branch. Why can't we run this unconditionally like the previous version of the function?
I will add a comment for this. If incog screenshot is allowed, we never need to set screenshot state, so it is skipped. Had updateScreenshot been unconditional, the screen would be protected when in an incognito Chrome Custom Tab AND the incognito screenshot feature is turned on.
What if the policies used to disable screenshots, but were then turned off?
for (TabModel model : mTabModelSelector.getModels()) {
if (shouldObserveSelectTab(model)) {Why do we need a loop instead of using something like `TabModelSelector::getCurrentModel` ? Is there a case where this won't return the right one?
}Can we return here since there is only supposed to be one active model?
```suggestion
return;
}
```
If not, then how do we ensure only one callback is registered at a time?
if (profile != null && !profile.isIncognitoBranded()) {Same comment as in the constructor.
Related details
- Alex Chen
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. |
Alex Chen (Gerrit)
Alex Chen added 6 comments![]( "Open in Gerrit")
boolean enterpriseObserveSelectTab = false;Alex ChenConsidering the policies are dynamic, does making this check here really make sense? IIUC this constructor won't be called on every new tab being opened or every navigation, so this could become a gap.
This is a good point that applies to the code below as well -- `shouldObserveSelectTab(` is also called once on browser startup and does not refresh for policy updates.
The newest patchset observes when the profile's policies change, and re-initializes the controller when they do.
Done
if (profile != null && !profile.isIncognitoBranded()) {I'm not sure I understand this condition, won't that skip policy updates on incognito tabs? What about the DC policy being updated in those tabs?
Thanks for pointing this out. This condition indeed needs to be removed, otherwise incognito CCT won't apply machine policy updates.
if (!mAllowIncognitoScreenshot) {Alex ChenThis is an odd condition to check outside of a `currentTab.isIncognito()` branch. Why can't we run this unconditionally like the previous version of the function?
Dominique Fauteux-ChapleauI will add a comment for this. If incog screenshot is allowed, we never need to set screenshot state, so it is skipped. Had updateScreenshot been unconditional, the screen would be protected when in an incognito Chrome Custom Tab AND the incognito screenshot feature is turned on.
What if the policies used to disable screenshots, but were then turned off?
The current code should still listen for policy changes, and re-`initialize()` the listeners after.
for (TabModel model : mTabModelSelector.getModels()) {
if (shouldObserveSelectTab(model)) {Why do we need a loop instead of using something like `TabModelSelector::getCurrentModel` ? Is there a case where this won't return the right one?
We can't use `::getCurrentModel` bc it is possible to directly launch into incognito model, and in that case we would not override the IncognitoScreenshots featureflag
Can we return here since there is only supposed to be one active model?
```suggestion
return;
}
```If not, then how do we ensure only one callback is registered at a time?
Yes, we can return here; we at most only want to observe the regular tabmodel. ensuring only one callback is registered at a time is achieved by only having one tabID (with the implicit non-re-entrence invariant above)
I've refactored this function to hopefully be more clear about its intentions.
if (profile != null && !profile.isIncognitoBranded()) {Same comment as in the constructor.
- Dominique Fauteux-Chapleau
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. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau added 5 comments![]( "Open in Gerrit")
if (!mAllowIncognitoScreenshot) {Alex ChenThis is an odd condition to check outside of a `currentTab.isIncognito()` branch. Why can't we run this unconditionally like the previous version of the function?
Dominique Fauteux-ChapleauI will add a comment for this. If incog screenshot is allowed, we never need to set screenshot state, so it is skipped. Had updateScreenshot been unconditional, the screen would be protected when in an incognito Chrome Custom Tab AND the incognito screenshot feature is turned on.
Alex ChenWhat if the policies used to disable screenshots, but were then turned off?
The current code should still listen for policy changes, and re-`initialize()` the listeners after.
Acknowledged
for (TabModel model : mTabModelSelector.getModels()) {
if (shouldObserveSelectTab(model)) {Alex ChenWhy do we need a loop instead of using something like `TabModelSelector::getCurrentModel` ? Is there a case where this won't return the right one?
We can't use `::getCurrentModel` bc it is possible to directly launch into incognito model, and in that case we would not override the IncognitoScreenshots featureflag
How is that worked around by using a loop? Do you have a guarantee that the incognito model is later in the `mTabModelSelector.getModels()` list?
Beyond that, why is it OK to bypass incognito models? Won't that lead to gaps for DC?
Alex ChenCan we return here since there is only supposed to be one active model?
```suggestion
return;
}
```If not, then how do we ensure only one callback is registered at a time?
Yes, we can return here; we at most only want to observe the regular tabmodel. ensuring only one callback is registered at a time is achieved by only having one tabID (with the implicit non-re-entrence invariant above)
I've refactored this function to hopefully be more clear about its intentions.
Let's keep discussing this in the other comment thread. I feel like there's an implicit assumption about the loop I'm not understanding that might explain why you're not returning early.
if (!model.isIncognito()) {Shouldn't this be removed under the same rationale as this comment?
https://chromium-review.git.corp.google.com/c/chromium/src/+/8036164/comment/df59871d_cda2ae9d/
We should be observing changes to incognito since DC can apply to it.
mForceAllowIncognitoScreenshot =
!hasEnterpriseScreenshotPolicies
&& ChromeFeatureList.sIncognitoScreenshot.isEnabled();I don't know if this is the right way to go about this flag. Right now the implementation would just bypass the flag and always block incognito screenshots if any DC screenshot rule is set, but semantically we should only ignore the flag when the specific incognito tab is blocked by a specific rule.
Related details
- Alex Chen
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. |
Alex Chen (Gerrit)
Alex Chen added 3 comments![]( "Open in Gerrit")
for (TabModel model : mTabModelSelector.getModels()) {
if (shouldObserveSelectTab(model)) {Alex ChenWhy do we need a loop instead of using something like `TabModelSelector::getCurrentModel` ? Is there a case where this won't return the right one?
Dominique Fauteux-ChapleauWe can't use `::getCurrentModel` bc it is possible to directly launch into incognito model, and in that case we would not override the IncognitoScreenshots featureflag
How is that worked around by using a loop? Do you have a guarantee that the incognito model is later in the `mTabModelSelector.getModels()` list?
Beyond that, why is it OK to bypass incognito models? Won't that lead to gaps for DC?
perhaps worth resurfacing that on non-managed clank, screenshot in incognito is always blocked unless the `IncognitoScreenshot` feature flag is turned off.
With that, there's two reasons we want to look at all the tabmodels:
- we want to observe the profiles for all the tabmodels, so that when policies are synced we call `initialize` again.
- during initialization, we want to walk through all tabmodels so that if one or both tabmodel's profile has screenshot policies, we override the feature flag allowing screenshots in incognito.
There's is no ordering guarantee nor a need for one in either.
if (!model.isIncognito()) {Shouldn't this be removed under the same rationale as this comment?
https://chromium-review.git.corp.google.com/c/chromium/src/+/8036164/comment/df59871d_cda2ae9d/We should be observing changes to incognito since DC can apply to it.
I added this in the documentation at the top of the page, but screenshot protection is always enabled for incognito (overriding the feature flag) if there is a screenshot policy or realtimeurl is turned on. this is different from desktop because on clank by default prevents taking screenshots in incognito.
mForceAllowIncognitoScreenshot =
!hasEnterpriseScreenshotPolicies
&& ChromeFeatureList.sIncognitoScreenshot.isEnabled();I don't know if this is the right way to go about this flag. Right now the implementation would just bypass the flag and always block incognito screenshots if any DC screenshot rule is set, but semantically we should only ignore the flag when the specific incognito tab is blocked by a specific rule.
Agreed it would be more user friendly to only ignore the flag when DC blocks a page. I think it could work by using CurrentTabModelObserver and will try that.
Related details
- Alex Chen
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. |
Alex Chen (Gerrit)
Alex Chen added 1 comment![]( "Open in Gerrit")
for (TabModel model : mTabModelSelector.getModels()) {
if (shouldObserveSelectTab(model)) {Alex ChenWhy do we need a loop instead of using something like `TabModelSelector::getCurrentModel` ? Is there a case where this won't return the right one?
Dominique Fauteux-ChapleauWe can't use `::getCurrentModel` bc it is possible to directly launch into incognito model, and in that case we would not override the IncognitoScreenshots featureflag
Alex ChenHow is that worked around by using a loop? Do you have a guarantee that the incognito model is later in the `mTabModelSelector.getModels()` list?
Beyond that, why is it OK to bypass incognito models? Won't that lead to gaps for DC?
perhaps worth resurfacing that on non-managed clank, screenshot in incognito is always blocked unless the `IncognitoScreenshot` feature flag is turned off.
With that, there's two reasons we want to look at all the tabmodels:
- we want to observe the profiles for all the tabmodels, so that when policies are synced we call `initialize` again.
- during initialization, we want to walk through all tabmodels so that if one or both tabmodel's profile has screenshot policies, we override the feature flag allowing screenshots in incognito.
There's is no ordering guarantee nor a need for one in either.
Just to clarify, we don't need ordering because we enable/disable the global tabmodel observation/layoutstate observation based as long as either of the profiles have relevant policies.
Related details
- Alex Chen
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. |