fix: Hide internal data by making it auxiliary #954
Conversation
| @@ -56,6 +56,19 @@ | |||
| "PathBasedAuthorizer:_paths_value": { "@type": "DenyAllAuthorizer" } | |||
| } | |||
| ] | |||
| }, | |||
| { | |||
| "comment": "Marks the /.internal/ storage container as an auxiliary resource, thereby hiding it from container representations.", | |||
There was a problem hiding this comment.
I'm not sure that marking it as an auxiliary resource, just for the effect that has, is the right way.
/.internal/ does otherwise not behave as an auxiliary resource, does it?
There was a problem hiding this comment.
Not really, since most other requests where auxiliary features are relevant will be blocked of by the authorizer preventing access to the /.internal/ container.
I'm also not the biggest fan of this solution, so I'm even ok with reverting this, but it is currently the simplest way we have to hide resources. But it definitely is sort of hacky. I mostly went with it since it's only a config change and doesn't actually change any code.
Another solution would be to have the DataAccessorBasedStore (or a new store type) take a list of regexes to ignore, but that class already does a lot.
There was a problem hiding this comment.
Fine with me—good to proceed.
| @@ -10,7 +10,7 @@ import type { AuxiliaryIdentifierStrategy } from './AuxiliaryIdentifierStrategy' | |||
| */ | |||
| export interface AuxiliaryStrategy extends AuxiliaryIdentifierStrategy { | |||
| /** | |||
| * Whether this auxiliary resource in a root storage container. | |||
| * Whether this auxiliary resource is required in a root storage container. | |||
There was a problem hiding this comment.
I'm not sure if I understood this sentence correctly, but do we mean this?
| * Whether this auxiliary resource is required in a root storage container. | |
| * Whether the root storage container requires this auxiliary resource to be present. |
There was a problem hiding this comment.
Pretty much. The current auxiliary implementation is based on what is needed for acl files and might be overtuned a bit for that, but one of the features there is that you can't delete an acl file that is stored in a pim:Storage container.
| @@ -10,7 +10,7 @@ import type { AuxiliaryIdentifierStrategy } from './AuxiliaryIdentifierStrategy' | |||
| */ | |||
| export interface AuxiliaryStrategy extends AuxiliaryIdentifierStrategy { | |||
| /** | |||
| * Whether this auxiliary resource in a root storage container. | |||
| * Whether this auxiliary resource is required in a root storage container. | |||
| * If yes, this means they can't be deleted individually from such a container. | |||
| * @param identifier - Identifier of the auxiliary resource. | |||
| */ | |||
There was a problem hiding this comment.
breaking suggestion: isRequiredInRoot
joachimvh
force-pushed
the
fix/visible-internal
branch
from
1821252
to
0a81c75
Compare
joachimvh
force-pushed
the
fix/visible-internal
branch
from
0a81c75
to
6c6b0c2
Compare
Closes #892
I did not create an auxiliary strategy that identifies all resources starting with an
.as auxiliary. Some reasons:The strategy requires there to be an associated resource for an auxiliary resource. The most logical choice would be the container they are located in. The issue is that locking happens on the associated resource, so all access to any of those resources (in the same container) would require the same lock. Might not be horrible but perhaps also not great. On the other hand it is also not possible to construct an auxiliary identifier when taking an associated identifier as input. This is used when deleting a resource to first delete its auxiliary resources.
The locking issue is perhaps not that bad though, and the other parts might indicate an issue with the current implementation that can potentially be resolved, so if this is something that would be required we can make a new issue to support those as auxiliary resources.