Bug 238524 – Move canAccessStorage() check from SecurityOrigin to ScriptExecutionContext

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Sihui Liu]

...

[Sihui Liu]

Created attachment 456069 [details]
Patch

[Sihui Liu]

Created attachment 456318 [details]
Patch

[Sihui Liu]

Created attachment 456383 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/91318259>

[Sihui Liu]

Created attachment 456785 [details]
Patch

[Chris Dumez]

Comment on attachment 456785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456785&action=review

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:99
> +    CDMPrivate::LocalStorageAccess access = CDMPrivate::LocalStorageAccess::Allowed;

auto

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:101
> +    if (isEphemeral || !document->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage) || !document->canAccessResourceAsThirdParty())

The additional canAccessResourceAsThirdParty() check doesn't look great. Why is this needed now?
Seems to make the code more complicated than before your patch.

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:91
> +    bool isTransient = !context.canAccessResourceAsThirdParty();

It is not directly obvious that these are equivalent.

> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:112
> +    bool isTransient = !context.canAccessResourceAsThirdParty();

ditto.

> Source/WebCore/Modules/storage/StorageManager.cpp:70
> +    if (!context->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage))

This change seems Web observable, why is it OK? Is it the ordering of the spec?

> Source/WebCore/dom/ScriptExecutionContext.cpp:584
> +    if (m_storageBlockingPolicy != StorageBlockingPolicy::BlockThirdParty)

I am not sure I understand why BlockThirdParty behaves differently from BlockAll / AllowAll.

> Source/WebCore/dom/ScriptExecutionContext.h:313
> +    WEBCORE_EXPORT bool canAccessResource(ResourceType);

Can this be const?

> Source/WebCore/dom/ScriptExecutionContext.h:314
> +    bool canAccessResourceAsThirdParty();

ditto.

Also, this is a bit unclear. Which resource?

> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:571
> +    return !frame->document()->canAccessResource(ScriptExecutionContext::ResourceType::ApplicationCache);

I have trouble seeing how this is equivalent. This seems to adduce that the origin of the current document is the same origin as the resource request URL. Why is that guaranteed?

[Sihui Liu]

Comment on attachment 456785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456785&action=review

>> Source/WebCore/Modules/encryptedmedia/CDM.cpp:99
>> +    CDMPrivate::LocalStorageAccess access = CDMPrivate::LocalStorageAccess::Allowed;
> 
> auto

Sure.

>> Source/WebCore/Modules/encryptedmedia/CDM.cpp:101
>> +    if (isEphemeral || !document->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage) || !document->canAccessResourceAsThirdParty())
> 
> The additional canAccessResourceAsThirdParty() check doesn't look great. Why is this needed now?
> Seems to make the code more complicated than before your patch.

As mentioned in the changelog... some types don't do this check here, but use it for deciding persistency.

>> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:91
>> +    bool isTransient = !context.canAccessResourceAsThirdParty();
> 
> It is not directly obvious that these are equivalent.

canAccessDatabase(topOrigin) is equal to canAccessResourceAsThirdParty() && canAccessResource(ResourceType::IndexedDB)
canAccessDatabase(nullptr) is equal to canAccessResource(ResourceType::IndexedDB). 
We already check canAccessResource(ResourceType::IndexedDB) above (with shouldThrowSecurityException), so we only need canAccessResourceAsThirdParty() here.

This patch eliminates a redundant check.

>> Source/WebCore/Modules/indexeddb/IDBFactory.cpp:112
>> +    bool isTransient = !context.canAccessResourceAsThirdParty();
> 
> ditto.

Ditto.

>> Source/WebCore/Modules/storage/StorageManager.cpp:70
>> +    if (!context->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage))
> 
> This change seems Web observable, why is it OK? Is it the ordering of the spec?

Actually we can remove the origin check below. If origin is null, this check will fail.

The spec says: 
If key’s origin is an opaque origin, then return failure.
If the user has disabled storage, then return failure.
The ordering of canAccessResource aligns with it.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:584
>> +    if (m_storageBlockingPolicy != StorageBlockingPolicy::BlockThirdParty)
> 
> I am not sure I understand why BlockThirdParty behaves differently from BlockAll / AllowAll.

I am not sure too, but this is align with original implementation. The patch does not change the original behavior.

>> Source/WebCore/dom/ScriptExecutionContext.h:313
>> +    WEBCORE_EXPORT bool canAccessResource(ResourceType);
> 
> Can this be const?

Sure.

>> Source/WebCore/dom/ScriptExecutionContext.h:314
>> +    bool canAccessResourceAsThirdParty();
> 
> ditto.
> 
> Also, this is a bit unclear. Which resource?

This check has no difference between different types..

>> Source/WebCore/loader/appcache/ApplicationCacheHost.cpp:571
>> +    return !frame->document()->canAccessResource(ScriptExecutionContext::ResourceType::ApplicationCache);
> 
> I have trouble seeing how this is equivalent. This seems to adduce that the origin of the current document is the same origin as the resource request URL. Why is that guaranteed?

I can't find spec of appcache as it is deprecated, but it seems the origin must be the same as the page contains it https://webplatform.github.io/docs/apis/appcache/ApplicationCache/.
And the change passes existing tests according to bots (which might be good enough for a deprecated API?)

[Chris Dumez]

Comment on attachment 456785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456785&action=review

>>> Source/WebCore/Modules/encryptedmedia/CDM.cpp:101
>>> +    if (isEphemeral || !document->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage) || !document->canAccessResourceAsThirdParty())
>> 
>> The additional canAccessResourceAsThirdParty() check doesn't look great. Why is this needed now?
>> Seems to make the code more complicated than before your patch.
> 
> As mentioned in the changelog... some types don't do this check here, but use it for deciding persistency.

I am just saying, there has got to be a way to write your new API so that it is not worse than the old one (in that it results in more verbose / complicated code).

We use to have a single canAccess() for each type. Now we have a canAccess() with the type as parameter but then we have an extra canAccessResourceAsThirdParty()? This seems unnecessarily complicated and I am having trouble reasoning about it.
We don't want people to make mistakes with those permissions so I think it is critical that the new API be very straightforward / easy to use. I personally don't think this achieves it.

[Sihui Liu]

Created attachment 456840 [details]
Patch

[Chris Dumez]

Comment on attachment 456840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456840&action=review

> Source/WebCore/Modules/encryptedmedia/CDM.cpp:101
> +    if (isEphemeral || !document->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage) || !document->canAccessResourceAsThirdParty())

Can you please find a way to make your new API as easy to use as the previous one?

The new code here is more complicated. Security checks should be as easy / straightforward as possible to avoid getting them wrong.

We shouldn't need separate canAccessResource / canAccessResourceAsThirdParty functions when the previous code didn't need to.

[Sihui Liu]

(In reply to Chris Dumez from comment #10)
> Comment on attachment 456840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456840&action=review
> 
> > Source/WebCore/Modules/encryptedmedia/CDM.cpp:101
> > +    if (isEphemeral || !document->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage) || !document->canAccessResourceAsThirdParty())
> 
> Can you please find a way to make your new API as easy to use as the
> previous one?
> 
> The new code here is more complicated. Security checks should be as easy /
> straightforward as possible to avoid getting them wrong.
> 
> We shouldn't need separate canAccessResource / canAccessResourceAsThirdParty
> functions when the previous code didn't need to.

The reason we didn't use separate functions is because the caller passes different parameter (without or without topOrigin) to check for different things (access check & third-party check). It is actually more error-prone for callers as they need to understand when to pass topOrigin and what origin to pass. And for canAccess* function, it can't tell if the topOrigin is empty because of some error, or it's null because the caller doesn't want third-party check (we treat it as no third-party check).

The new API makes that clear by offering two separate functions (motivated by the FIXME in SecurityOrigin::canAccessStorage). 

When a new type is added, the developer needs to update CanAccessResourceType, get a chance to look at the checks (I think the new naming is more clearer about what is checked).

It might be clearer if canAccessResourceType() returns access type (e.g. Deny, Persistent, Session), but not all types have the difference so I didn't add.

[Chris Dumez]

Comment on attachment 456840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456840&action=review

>>> Source/WebCore/Modules/encryptedmedia/CDM.cpp:101
>>> +    if (isEphemeral || !document->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage) || !document->canAccessResourceAsThirdParty())
>> 
>> Can you please find a way to make your new API as easy to use as the previous one?
>> 
>> The new code here is more complicated. Security checks should be as easy / straightforward as possible to avoid getting them wrong.
>> 
>> We shouldn't need separate canAccessResource / canAccessResourceAsThirdParty functions when the previous code didn't need to.
> 
> The reason we didn't use separate functions is because the caller passes different parameter (without or without topOrigin) to check for different things (access check & third-party check). It is actually more error-prone for callers as they need to understand when to pass topOrigin and what origin to pass. And for canAccess* function, it can't tell if the topOrigin is empty because of some error, or it's null because the caller doesn't want third-party check (we treat it as no third-party check).
> 
> The new API makes that clear by offering two separate functions (motivated by the FIXME in SecurityOrigin::canAccessStorage). 
> 
> When a new type is added, the developer needs to update CanAccessResourceType, get a chance to look at the checks (I think the new naming is more clearer about what is checked).
> 
> It might be clearer if canAccessResourceType() returns access type (e.g. Deny, Persistent, Session), but not all types have the difference so I didn't add.

Then maybe there should be a single canAccessFunction() but with an extra mandatory parameter to indicate the behavior with regards to origin checks.

Sorry but canAccessResource(ResourceType) & canAccessResourceAsThirdParty() (without type) are super confusing to me.

What adds to the confusion I think is that canAccessResourceAsThirdParty() doesn't take a resource type and the naming of the function too. Since to indicate we should only call this when in a third-party iframe but clearly that's not the case based on how it is used.

[Sihui Liu]

(In reply to Chris Dumez from comment #12)
> Comment on attachment 456840 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=456840&action=review
> 
> >>> Source/WebCore/Modules/encryptedmedia/CDM.cpp:101
> >>> +    if (isEphemeral || !document->canAccessResource(ScriptExecutionContext::ResourceType::LocalStorage) || !document->canAccessResourceAsThirdParty())
> >> 
> >> Can you please find a way to make your new API as easy to use as the previous one?
> >> 
> >> The new code here is more complicated. Security checks should be as easy / straightforward as possible to avoid getting them wrong.
> >> 
> >> We shouldn't need separate canAccessResource / canAccessResourceAsThirdParty functions when the previous code didn't need to.
> > 
> > The reason we didn't use separate functions is because the caller passes different parameter (without or without topOrigin) to check for different things (access check & third-party check). It is actually more error-prone for callers as they need to understand when to pass topOrigin and what origin to pass. And for canAccess* function, it can't tell if the topOrigin is empty because of some error, or it's null because the caller doesn't want third-party check (we treat it as no third-party check).
> > 
> > The new API makes that clear by offering two separate functions (motivated by the FIXME in SecurityOrigin::canAccessStorage). 
> > 
> > When a new type is added, the developer needs to update CanAccessResourceType, get a chance to look at the checks (I think the new naming is more clearer about what is checked).
> > 
> > It might be clearer if canAccessResourceType() returns access type (e.g. Deny, Persistent, Session), but not all types have the difference so I didn't add.
> 
> Then maybe there should be a single canAccessFunction() but with an extra
> mandatory parameter to indicate the behavior with regards to origin checks.
> 
> Sorry but canAccessResource(ResourceType) & canAccessResourceAsThirdParty()
> (without type) are super confusing to me.
> 
> What adds to the confusion I think is that canAccessResourceAsThirdParty()
> doesn't take a resource type and the naming of the function too. Since to
> indicate we should only call this when in a third-party iframe but clearly
> that's not the case based on how it is used.

Okay, will try making it one function.

[Sihui Liu]

Created attachment 456863 [details]
Patch

[Sihui Liu]

Created attachment 456932 [details]
Patch

[Chris Dumez]

Comment on attachment 456932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456932&action=review

> Source/WebCore/dom/ScriptExecutionContext.cpp:733
> +    case ResourceType::ApplicationCache:

Same comment as for WebSQL, seems we only allow this as first party?

> Source/WebCore/dom/ScriptExecutionContext.cpp:734
> +    case ResourceType::Plugin:

Same comment as for WebSQL.

> Source/WebCore/dom/ScriptExecutionContext.cpp:735
> +    case ResourceType::WebSQL:

Why does ResourceType::WebSQL fall through? Looks to me like we never allow WebSQL access as third party (looking at call sites). Therefore, it seems nonsensical to ever return AccessLevel::ThirdPartyAccess?
Seems we should either return FirstPartyAccess or NoAccess. Then the call sites can check for NoAccess to avoid confusion.

> Source/WebCore/dom/ScriptExecutionContext.cpp:738
> +    case ResourceType::StorageManager:

Isn't this unused?

[Sihui Liu]

Created attachment 457100 [details]
Patch

[Sihui Liu]

Comment on attachment 456932 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456932&action=review

>> Source/WebCore/dom/ScriptExecutionContext.cpp:735
>> +    case ResourceType::WebSQL:
> 
> Why does ResourceType::WebSQL fall through? Looks to me like we never allow WebSQL access as third party (looking at call sites). Therefore, it seems nonsensical to ever return AccessLevel::ThirdPartyAccess?
> Seems we should either return FirstPartyAccess or NoAccess. Then the call sites can check for NoAccess to avoid confusion.

Made it return NoAccess for third-party with these types.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:738
>> +    case ResourceType::StorageManager:
> 
> Isn't this unused?

Nice catch! Fixed to use it in StorageManager.cpp

[Chris Dumez]

Comment on attachment 457100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457100&action=review

> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:50
> +    if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) != ScriptExecutionContext::AccessLevel::FirstPartyAccess)

I think this would be clearer as:
if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) == ScriptExecutionContext::AccessLevel::NoAccess)

That is why I asked to make the previous changes.

> Source/WebCore/dom/ScriptExecutionContext.cpp:747
> +            return AccessLevel::ThirdPartyAccess;

Something is still confusing here... `m_storageBlockingPolicy == StorageBlockingPolicy::BlockThirdParty` so this seems to indicate that third party storage access is forbidden.
However, we're returning AccessLevel::ThirdPartyAccess (which seems to indicate we're allowing access as third-party, since it is not NoAccess).

Either I am misreading or we have a bug or the naming is still confusing.

[Sihui Liu]

Comment on attachment 457100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457100&action=review

>> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:50
>> +    if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) != ScriptExecutionContext::AccessLevel::FirstPartyAccess)
> 
> I think this would be clearer as:
> if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) == ScriptExecutionContext::AccessLevel::NoAccess)
> 
> That is why I asked to make the previous changes.

Oh I see, will update.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:747
>> +            return AccessLevel::ThirdPartyAccess;
> 
> Something is still confusing here... `m_storageBlockingPolicy == StorageBlockingPolicy::BlockThirdParty` so this seems to indicate that third party storage access is forbidden.
> However, we're returning AccessLevel::ThirdPartyAccess (which seems to indicate we're allowing access as third-party, since it is not NoAccess).
> 
> Either I am misreading or we have a bug or the naming is still confusing.

As I said in the initial patch, a storage API can have three states: no storage allowed (no access),  in-memory storage only, persistent storage; so there are three access levels here.
If we return NoAccess, the caller can't tell the difference.
The confusion is caused by different types having different "block" policy; some types only have two states.

[Chris Dumez]

(In reply to Sihui Liu from comment #20)
> Comment on attachment 457100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457100&action=review
> 
> >> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:50
> >> +    if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) != ScriptExecutionContext::AccessLevel::FirstPartyAccess)
> > 
> > I think this would be clearer as:
> > if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) == ScriptExecutionContext::AccessLevel::NoAccess)
> > 
> > That is why I asked to make the previous changes.
> 
> Oh I see, will update.
> 
> >> Source/WebCore/dom/ScriptExecutionContext.cpp:747
> >> +            return AccessLevel::ThirdPartyAccess;
> > 
> > Something is still confusing here... `m_storageBlockingPolicy == StorageBlockingPolicy::BlockThirdParty` so this seems to indicate that third party storage access is forbidden.
> > However, we're returning AccessLevel::ThirdPartyAccess (which seems to indicate we're allowing access as third-party, since it is not NoAccess).
> > 
> > Either I am misreading or we have a bug or the naming is still confusing.
> 
> As I said in the initial patch, a storage API can have three states: no
> storage allowed (no access),  in-memory storage only, persistent storage; so
> there are three access levels here.
> If we return NoAccess, the caller can't tell the difference.
> The confusion is caused by different types having different "block" policy;
> some types only have two states.

Do you mean that when StorageBlockingPolicy::BlockThirdParty policy is used and a third-party iframe is trying to access storage, we allow access but it uses a special in-memory only access and not the persistent storage?
If so, I believe the issue may be with the naming. Maybe AccessLevel should be something like, NoAccess/FullAccess/InMemoryAccess ?

[Sihui Liu]

Comment on attachment 457100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457100&action=review

>>>> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:50
>>>> +    if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) != ScriptExecutionContext::AccessLevel::FirstPartyAccess)
>>> 
>>> I think this would be clearer as:
>>> if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) == ScriptExecutionContext::AccessLevel::NoAccess)
>>> 
>>> That is why I asked to make the previous changes.
>> 
>> Oh I see, will update.
> 
> Do you mean that when StorageBlockingPolicy::BlockThirdParty policy is used and a third-party iframe is trying to access storage, we allow access but it uses a special in-memory only access and not the persistent storage?
> If so, I believe the issue may be with the naming. Maybe AccessLevel should be something like, NoAccess/FullAccess/InMemoryAccess ?

Yes, you got it! I think the action should not be decided by this function, it should be decided by the caller... The function should basically say: "you should deny/allow/treat it as third party", and the caller gets to decide what action to take... That's why I used FALLTHOUGH for WebSQL and the others types. They can decide how they want to treat 3rd party.

[Chris Dumez]

(In reply to Sihui Liu from comment #22)
> Comment on attachment 457100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457100&action=review
> 
> >>>> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:50
> >>>> +    if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) != ScriptExecutionContext::AccessLevel::FirstPartyAccess)
> >>> 
> >>> I think this would be clearer as:
> >>> if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) == ScriptExecutionContext::AccessLevel::NoAccess)
> >>> 
> >>> That is why I asked to make the previous changes.
> >> 
> >> Oh I see, will update.
> > 
> > Do you mean that when StorageBlockingPolicy::BlockThirdParty policy is used and a third-party iframe is trying to access storage, we allow access but it uses a special in-memory only access and not the persistent storage?
> > If so, I believe the issue may be with the naming. Maybe AccessLevel should be something like, NoAccess/FullAccess/InMemoryAccess ?
> 
> Yes, you got it! I think the action should not be decided by this function,
> it should be decided by the caller... The function should basically say:
> "you should deny/allow/treat it as third party", and the caller gets to
> decide what action to take... That's why I used FALLTHOUGH for WebSQL and
> the others types. They can decide how they want to treat 3rd party.

I still think we need to tweak the enum naming somehow to clarify this. Trying to come up with a proposal...

[Sihui Liu]

(In reply to Chris Dumez from comment #23)
> (In reply to Sihui Liu from comment #22)
> > Comment on attachment 457100 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=457100&action=review
> > 
> > >>>> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:50
> > >>>> +    if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) != ScriptExecutionContext::AccessLevel::FirstPartyAccess)
> > >>> 
> > >>> I think this would be clearer as:
> > >>> if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) == ScriptExecutionContext::AccessLevel::NoAccess)
> > >>> 
> > >>> That is why I asked to make the previous changes.
> > >> 
> > >> Oh I see, will update.
> > > 
> > > Do you mean that when StorageBlockingPolicy::BlockThirdParty policy is used and a third-party iframe is trying to access storage, we allow access but it uses a special in-memory only access and not the persistent storage?
> > > If so, I believe the issue may be with the naming. Maybe AccessLevel should be something like, NoAccess/FullAccess/InMemoryAccess ?
> > 
> > Yes, you got it! I think the action should not be decided by this function,
> > it should be decided by the caller... The function should basically say:
> > "you should deny/allow/treat it as third party", and the caller gets to
> > decide what action to take... That's why I used FALLTHOUGH for WebSQL and
> > the others types. They can decide how they want to treat 3rd party.
> 
> I still think we need to tweak the enum naming somehow to clarify this.
> Trying to come up with a proposal...

Sure, I am looking forward to your solution to this

[Chris Dumez]

(In reply to Chris Dumez from comment #23)
> (In reply to Sihui Liu from comment #22)
> > Comment on attachment 457100 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=457100&action=review
> > 
> > >>>> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:50
> > >>>> +    if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) != ScriptExecutionContext::AccessLevel::FirstPartyAccess)
> > >>> 
> > >>> I think this would be clearer as:
> > >>> if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) == ScriptExecutionContext::AccessLevel::NoAccess)
> > >>> 
> > >>> That is why I asked to make the previous changes.
> > >> 
> > >> Oh I see, will update.
> > > 
> > > Do you mean that when StorageBlockingPolicy::BlockThirdParty policy is used and a third-party iframe is trying to access storage, we allow access but it uses a special in-memory only access and not the persistent storage?
> > > If so, I believe the issue may be with the naming. Maybe AccessLevel should be something like, NoAccess/FullAccess/InMemoryAccess ?
> > 
> > Yes, you got it! I think the action should not be decided by this function,
> > it should be decided by the caller... The function should basically say:
> > "you should deny/allow/treat it as third party", and the caller gets to
> > decide what action to take... That's why I used FALLTHOUGH for WebSQL and
> > the others types. They can decide how they want to treat 3rd party.
> 
> I still think we need to tweak the enum naming somehow to clarify this.
> Trying to come up with a proposal...

HasResourceAccess { No, Yes, DefaultForThirdParty } ?
or
HasResourceAccess { No, Yes, MaybeForThirdParty } ?
or
HasResourceAccess { No, Yes, YesAsThirdParty } ?

Consider the Cookies/Geolocation access for example. In your patch, it returns FirstPartyAccess no matter if first party or not, which I find confusing. I think it returning HasResourceAccess::Yes would be clearer. Mentioning "FirstParty" when we didn't do any first party checks is not ideal.

[Chris Dumez]

(In reply to Chris Dumez from comment #25)
> (In reply to Chris Dumez from comment #23)
> > (In reply to Sihui Liu from comment #22)
> > > Comment on attachment 457100 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=457100&action=review
> > > 
> > > >>>> Source/WebCore/Modules/webdatabase/DOMWindowWebDatabase.cpp:50
> > > >>>> +    if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) != ScriptExecutionContext::AccessLevel::FirstPartyAccess)
> > > >>> 
> > > >>> I think this would be clearer as:
> > > >>> if (document->canAccessResource(ScriptExecutionContext::ResourceType::WebSQL) == ScriptExecutionContext::AccessLevel::NoAccess)
> > > >>> 
> > > >>> That is why I asked to make the previous changes.
> > > >> 
> > > >> Oh I see, will update.
> > > > 
> > > > Do you mean that when StorageBlockingPolicy::BlockThirdParty policy is used and a third-party iframe is trying to access storage, we allow access but it uses a special in-memory only access and not the persistent storage?
> > > > If so, I believe the issue may be with the naming. Maybe AccessLevel should be something like, NoAccess/FullAccess/InMemoryAccess ?
> > > 
> > > Yes, you got it! I think the action should not be decided by this function,
> > > it should be decided by the caller... The function should basically say:
> > > "you should deny/allow/treat it as third party", and the caller gets to
> > > decide what action to take... That's why I used FALLTHOUGH for WebSQL and
> > > the others types. They can decide how they want to treat 3rd party.
> > 
> > I still think we need to tweak the enum naming somehow to clarify this.
> > Trying to come up with a proposal...
> 
> HasResourceAccess { No, Yes, DefaultForThirdParty } ?
> or
> HasResourceAccess { No, Yes, MaybeForThirdParty } ?
> or
> HasResourceAccess { No, Yes, YesAsThirdParty } ?
> 
> Consider the Cookies/Geolocation access for example. In your patch, it
> returns FirstPartyAccess no matter if first party or not, which I find
> confusing. I think it returning HasResourceAccess::Yes would be clearer.
> Mentioning "FirstParty" when we didn't do any first party checks is not
> ideal.

For the record, I changed my mind on `HasResourceAccess { No, Yes, YesAsThirdParty }`. YesAsThirdParty would be confusing since WebSQL denies access for third-party (so it is not really a 'Yes' anymore). For this reason, I like `{ No, Yes, DefaultForThirdParty }` better.

[Chris Dumez]

Comment on attachment 457100 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457100&action=review

> Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBPersistence.mm:289
> +    [[configuration preferences] _setStorageBlockingPolicy:_WKStorageBlockingPolicyAllowAll];

Also please explain (in the changelog) why this is needed. This seems to indicate you changed behavior in some way, which is a bit unexpected in a large refactoring patch.
We may want to consider doing the behavior change (assuming it is an intentional bug fix) in a separate patch for clarity (unless you're very confident we can do it in the same patch).

[Sihui Liu]

Created attachment 457123 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #27)
> Comment on attachment 457100 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457100&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/IndexedDBPersistence.mm:289
> > +    [[configuration preferences] _setStorageBlockingPolicy:_WKStorageBlockingPolicyAllowAll];
> 
> Also please explain (in the changelog) why this is needed. This seems to
> indicate you changed behavior in some way, which is a bit unexpected in a
> large refactoring patch.
> We may want to consider doing the behavior change (assuming it is an
> intentional bug fix) in a separate patch for clarity (unless you're very
> confident we can do it in the same patch).

That change turns out to be not needed; removed.

[Chris Dumez]

Comment on attachment 457123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457123&action=review

r=me

> Source/WebCore/dom/ScriptExecutionContext.h:313
> +    enum class HasResourceAccess : uint8_t {

nit: this enum could definitely be on one line.

[Sihui Liu]

Created attachment 457149 [details]
Patch for landing

[EWS]

Committed r292677 (249469@main): <https://commits.webkit.org/249469@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457149 [details].