1558478 - StorageManager/QuotaManager should listen for removal of the "persistent-storage" permission and revoke the storage grant. (Was: Persistent storage permission granted on "Never Allow")

Status: NEW

(Core :: Storage: StorageManager, defect, P3)

Description

[Paul Zühlcke [:pbz]]

When I allow a website to use persistent storage via the permission prompt and then clear the permission again using the identity panel, persistent storage permission is always granted on subsequent requests, no matter what I select on the permission prompt (Not now, Never Allow, Allow).
This is only fixed once I clear site data.

Tested on https://permission.site/ with Firefox 67 and latest nightly.

Comment 1

[Johann Hofmann [:johannh]]

Uhm, yeah, I can reproduce now. Huh. If I had to guess I'd say this is caused by storage internals (cc asuth) but this probably needs a more thorough investigation...

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

I feel like I already commented on a bug about this, but am having trouble finding it in Thunderbird and I think it's clear it fell off everyone's radar. We can dupe when we find it.

From memory, storage.persist() actually does 2 things when allowed:

1. Sets a permission. This reflects UI state and allows for the next thing to be auto-granted without prompting (maybe?)
2. Sets a bit in the QuotaManager-managed PROFILE/storage/default/ORIGIN/.metadata-v2 which defines QuotaManager's behavior.

Clearing the permission does not change the second thing, which is a bug. Clearing the data for the origin does clear it because the .metadata-v2 gets wiped out.

I think we need some type of glue logic between the permission change action and QuotaManager revoking persistence for the origin. I think the question is what is the best way to do that. Should QuotaManager always be listening for permission changes? Should there instead be a privacy clearing-house module that's instead responsible for listening to permission changes like that and translating them into direct calls into QuotaManager? Maybe that privacy permission clearing-house already exists?

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

At to be clear, I'd prefer the more explicit mechanism where something calls directly into QuotaManager. I feel like using the observer service as an event bus was nice in a world where we had legacy extensions and it was important to surface hooks that everyone can use. But I think it obscures how the system works and makes it harder to reason about. Far better to have explicit calls which make the sequence of calls explicit.

Comment 4

[Tom Tung [:tt, :ttung]]

This is a known issue on Storage API v1 and planned to be handled on v2, IIRC. The implementation is the same as Andrew mentioned in comment #2.

The reason for UX was that "are users aware of persisted storage would be removed once the permission is clear". For instance, if a user goes the about:preference and remove all existing permission, would they expect all the persisted storage will be removed?

At that time, we were not so sure users can bundle these two things together, so we didn't take care of that on v1.

BTW, because persisted storage might have occupied more than group limit, we might need to consider if we just want to remove as a little storge as possible to fix the quota or just remove all the persisted storage. We could just remove the whole the persisted storages once permission is clear if we want to make it simple, though.

Comment 5

[Tom Tung [:tt, :ttung]]

(In reply to Andrew Sutherland [:asuth] (he/him) from comment #2)

I think we need some type of glue logic between the permission change action and QuotaManager revoking persistence for the origin. I think the question is what is the best way to do that. Should QuotaManager always be listening for permission changes? Should there instead be a privacy clearing-house module that's instead responsible for listening to permission changes like that and translating them into direct calls into QuotaManager? Maybe that privacy permission clearing-house already exists?

The first question:
We could, alternatively, let Storage Manager (It's a manager for request the "persist permission") listen for permission changes and send a job/propagate the message to QuotaManager.

One thing I want to mention is that QuotaManager doesn't provide a way to rollback the persistent state. A persisted storage will be removed once it's not persisted. That might indicate a permission change would introduce tons of IO if we want to make this change (permission change would lead to removing persisted storage). (Becuase persisted storage is expected to take lots of usage in general). So that a callback might be required for the function if we want to have a module to propagate works and do that explicitly.

Comment 6

[Johann Hofmann [:johannh]]

At to be clear, I'd prefer the more explicit mechanism where something calls directly into QuotaManager. I feel like using the observer service as an event bus was nice in a world where we had legacy extensions and it was important to surface hooks that everyone can use. But I think it obscures how the system works and makes it harder to reason about. Far better to have explicit calls which make the sequence of calls explicit.

I'm not aware of any service that could call into QuotaManager on permission change at this point, and I think I personally disagree with your assessment. Adding an observer to permission manager and handling those permission changes sounds very straightforward (to implement and understand) and like a completely appropriate task for QuotaManager to do.

The reason for UX was that "are users aware of persisted storage would be removed once the permission is clear". For instance, if a user goes the about:preference and remove all existing permission, would they expect all the persisted storage will be removed?

That functionality doesn't exist, there's no way for the user to clear all permissions, the only way to revoke a persistent storage permission is using security UI on the page you've set it or by clearing site data for that domain.

A persisted storage will be removed once it's not persisted.

I'm unsure if I misunderstand here, from my work last year I was under the impression that with the "new" (persistent-storage permission based) system it would be possible to switch between persisted/non-persisted storage types without data loss. I'm not an expert in storage internals, but I thought the fact that the persistence type was hard-wired into the storage name was a major disadvantage of the "old" (indexedDB permission based) system.

Comment 7

[Tom Tung [:tt, :ttung]]

(In reply to Johann Hofmann [:johannh] from comment #6)

That functionality doesn't exist, there's no way for the user to clear all permissions, the only way to revoke a persistent storage permission is using security UI on the page you've set it or by clearing site data for that domain.

I see. So, if a user revokes a persisted storage permission by clearing site data, then we don't have a problem. So, this is about a user revokes a persistent storage permission by using security UI on the page.

I'm unsure if I misunderstand here, from my work last year I was under the impression that with the "new" (persistent-storage permission based) system it would be possible to switch between persisted/non-persisted storage types without data loss. I'm not an expert in storage internals, but I thought the fact that the persistence type was hard-wired into the storage name was a major disadvantage of the "old" (indexedDB permission based) system.

That's another story. The implementation for old indexedDB system in QuotaManager is to store data for persistent storage on a different directory. We storage default type storage under $profile/storage/default folder and persistent on $profile/storage/permanent folder. That's an issue at the beginning of implementing persist function, though. We fix that by adding an additional flag on the .metadata-v2 file for the persisting storage on the default folder and adding a flag on QuotaManager in-memory objects.

I meant the only way for a QuotaManager to revoke persistent storage is to clear that storage entirely now. The main reason is that a persisted storage can have much quota in QuotaManager. Once it's not a persisted storage, it's possible to exceed the quota for a temporary(best-effort) storage. We wanted to have a mechanism to delete as a little usage as possible while a persisted storage is changed back to temporary storage at first, but it's too complicate and thus we gave up that for v1. (https://bugzilla.mozilla.org/show_bug.cgi?id=1301276)

To elaborate more on the quota/usage, the quota/usage system on QuotaManager is a little complicated. For each persistent/persisted storage, QuotaManager would just restrict them by the global limit. However, for the general temporary storage, QuotaManager would limit them by both the global limit and the group limit.

To elaborate more on the condition where the usage for an origin is greater than a limit. QuotaManager would only have this issue on the stage of initialization. After initialization, quota clients would ask QuotaManager whether they can increase/decrease usage before actually increasing/decreasing. If the incoming increasing would make the usage of a website/origin exceeds the limit, then QuotaManager would evict storages based on LRU. If we are going to allow revoking persistent storage back, then we could have the usage for an origin is greater than the limit after the initialization. Also, the current mechanism for collecting storage to be evicted isn't suitable for revoking persistent storage back (because it might takes a bunch of time; And it's only base on LRU; we probably need to consider the size of each origin in this case).

BTW, I just have an idea that we could maybe just remove the .metadata-v2 file and maybe require a user to restart their firefox for revoking the permssion. It's a hack, but it would't create a new case for QuotaManager. We might still have to check whether the way for collecting origins for eviction is okay or not in this case, though.

Comment 9

[Tom Tung [:tt, :ttung]]

So, I think it's clear that we need to let QuotaManager/StorageManager listen to the permission change.
A thing that is still not clear to me is whether is okay to remove the storage once the permission change and thus cause storage to be removed [1].

Johann, do you think we need to do something to let the user be aware of that from your perspective? Another question is that do we have an async mechanism for the UI? I ask this because the observer stuff is on the main thread and the accessing storage stuff or persisted stuff are on the QuotaManager IO thread. I am thinking that is there something which we need to take care of so that users won't be confusing if they do:

1. Remove the persistent storage permission from the "site information" dropdown next to the address bar.
2. Check the storage.persisted immediately on the web console and this might not be changed if there is other stuff blocking the IO thread.

[1] As I mentioned in comment 7, QuotaManager gives more quota to a persisted origin. If we allowed switching from persisted and best-effort, we will need to either removing all storage under the origin or checking storage for all origins and maybe evict some old storage. The later thing would be complex and might take a while to complete the calculation so we only implemented the former in V1.

Comment 10

[Xiyuan Liu]

Another related issue is that when developing in localhost, clearing cookies and site data does not reset the persisted flag back to false. Any way to get around it?

Comment 11

[Tom Tung [:tt, :ttung]]

(In reply to Xiyuan Liu from comment #10)

Another related issue is that when developing in localhost, clearing cookies and site data does not reset the persisted flag back to false. Any way to get around it?

Thanks for reporting this! I filed bug 1628240 for that. Could you maybe check if the STR in that bug is the same as yours?

Comment 12

[Xiyuan Liu]

Thanks for the prompt response!

In addition to what you listed, it's worthing mentioning that the persistent storage permission is not invoked after clearing the site data.

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #6)

At to be clear, I'd prefer the more explicit mechanism where something calls directly into QuotaManager. I feel like using the observer service as an event bus was nice in a world where we had legacy extensions and it was important to surface hooks that everyone can use. But I think it obscures how the system works and makes it harder to reason about. Far better to have explicit calls which make the sequence of calls explicit.

I'm not aware of any service that could call into QuotaManager on permission change at this point, and I think I personally disagree with your assessment. Adding an observer to permission manager and handling those permission changes sounds very straightforward (to implement and understand) and like a completely appropriate task for QuotaManager to do.

Fair enough. My rationale here was primarily that we messed up in a very similar situation before. But since bug 1526891 QuotaManager gets initialized in nsLayoutStatics::Initialize which eliminates the possibility of QuotaManager not being around to listen for permission changes. Then it's primarily a question of:

1. Test coverage that doesn't get disabled for being flaky.
2. Whether additional runtime checks can be added that run as a fundamental part of the fundamental operation of carrying out the user's privacy choices that are also useful/helpful as introspection for developers, potentially as an audit log for the user, and definitely for improving test fidelity.

• For example, the QuotaManager clearing ops could create a log entry for each origin it clears, also indicating when it had to break held locks, etc. (The lock breaking would be of more interest to tests, maybe not humans other than in an about: page.) It could also indicate when it couldn't find storage for an origin to express that no-op happened but it was expected.
• I think this might be something that's actually extra important as we implement QMv4 as origin directories will no longer exist in their current incarnation, making it much harder to manually inspect what happened. (So the expert-only audit log records would want to include the directory path, etc.)
• There is some overlap with the usage callbacks.

So practically speaking, this might mean that PersistentStoragePermissionPrompt which is already code specific to the permission setting could have something like an expectAuditLog field in promptActions for each action that would return something like ["QuotaManager", "persistence-set", SUBJECT_ORIGIN] for the "ALLOW_ACTION" case (where SUBJECT_ORIGIN would be a magic sentinel that the logic that checks the audit log knows to match up against the exact origin used.)

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

I'm moving this bug to StorageManager since it's not clear there's any action to be taken in the current component.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

That said, in terms of actions to take on an origin that's exceeding the quota it already has, there are really 2 possible courses of action:

1. Setting an explicit quota for the origin that's slightly higher than its current usage. (Slightly higher because sometimes it takes storage to use less storage. I know we'd made efforts related to that for IndexedDB, but just raising the grant slightly isn't so bad.)
2. Informing the user when this happens that allows them to decide to let the origin keep its current usage (Default, the step that we would have already taken), or to clear the origin entirely.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Is there any way to get a resolution on which direction, comment 15, this bug would go? If the backend will handle things as #1, then we could make the extension permission optional relying on the backend to deal with it. If it were decided to go with #2, or we cannot get an assured resultion, we'd probably opt to remove prompting for unlimited storage in extensions.

Comment 17

[Johann Hofmann [:johannh]]

Reading these comments I feel like asking the user "If you clear this permission then we need to reset all data for this site. Would you like to proceed?" is the most straightforward thing to implement which results in the fewest annoying edge cases to maintain. I don't think we need a super fancy UX design for this, we can reuse one of the existing prompts but would need custom strings of course. I can reach out to content strategy to get that if we want to settle on this plan?

In practice it would probably mean that quota manager would just clear the data if it notices the permission change and the UI handles warning the user before clearing the permission. Not 100% sure what that means for the extensions code though :)

Any thoughts?

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Johann Hofmann [:johannh] from comment #17)

Not 100% sure what that means for the extensions code though :)

We're trying to move towards making all permissions optional and providing users with the ability to revoke prior granted permissions. Revoking unlimitedStorage would result in deletion of data, I'm not inclined to going that direction, it's an all or nothing option for users. Given that, we'd probably just not allow unlimited storage as optional, and stop prompting it as a permission. We've discussed this and see little downside to it even though it's not desirable.

Comment 19

[Luca Greco [:rpl] [:luca] [:lgreco]]

If we will opt to make Firefox to clear all the stored data when the persistent storage permission is removed, we may still need to figure out how we should handle the following two scenario:

1. an extension developer removes the "unlimitedStorage" in a new version of the extension (seems unlikely... but still doesn't sound impossible)
2. a user downgrades an extension to a previous version that didn't have yet the "unlimitedStorage" permission (which I think may happen if a new version doesn't work for the user and the user decides to go back to a previous one)

To be fair, downgrading an extension isn't exactly a "supported scenario", many other things can go wrong because of something that the extension itself may be doing wrong, e.g. while migrating its own data internally between different formats, but seems still worth a mention because it is at least an "allowed scenario" (because we do allow the user to downgrade its installed extensions and something that as far as I know we have no plans to prevent the user from doing).

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:perry, could you have a look please?

For more information, please visit auto_nag documentation.