[Sharing NG] Resource in personal drive list extra permission

amrita-shrestha commented 9 months ago

Describe the bug

File resource list extra permission like Editor(file or folder) and Uploader. Folder resource list extra permission like Editor(Allows reading and updating file) Related issue https://github.com/owncloud/ocis/issues/8131

Steps to reproduce

create a file resource parent.txt and folder resource testFolder

send API request to list permission of parent.txt item,
curl -vk 'https://host.docker.internal:9200/graph/v1beta1/drives/{drives-id}/items/{items-id}/permissions'

{
"@libre.graph.permissions.actions.allowedValues": [
    "libre.graph/driveItem/permissions/create",
    "libre.graph/driveItem/children/create",
    "libre.graph/driveItem/standard/delete",
    "libre.graph/driveItem/path/read",
    "libre.graph/driveItem/quota/read",
    "libre.graph/driveItem/content/read",
    "libre.graph/driveItem/upload/create",
    "libre.graph/driveItem/permissions/read",
    "libre.graph/driveItem/children/read",
    "libre.graph/driveItem/versions/read",
    "libre.graph/driveItem/deleted/read",
    "libre.graph/driveItem/path/update",
    "libre.graph/driveItem/permissions/delete",
    "libre.graph/driveItem/deleted/delete",
    "libre.graph/driveItem/versions/update",
    "libre.graph/driveItem/deleted/update",
    "libre.graph/driveItem/basic/read",
    "libre.graph/driveItem/permissions/update",
    "libre.graph/driveItem/permissions/deny"
],
"@libre.graph.permissions.roles.allowedValues": [
    {
        "@libre.graph.weight": 1,
        "description": "Allows upload file or folder",
        "displayName": "Uploader",
        "id": "1c996275-f1c9-4e71-abdf-a42f6495e960"
    },
    {
        "@libre.graph.weight": 2,
        "description": "Allows reading the shared file or folder",
        "displayName": "Viewer",
        "id": "b1e2218d-eef8-4d4c-b82d-0f1a1b48f3b5"
    },
    {
        "@libre.graph.weight": 3,
        "description": "Allows reading and updating file",
        "displayName": "Editor",
        "id": "2d00ce52-1fc2-4dbc-8b95-a73b73395f5a"
    },
    {
        "@libre.graph.weight": 4,
        "description": "Allows creating, reading, updating and deleting the shared file or folder",
        "displayName": "Editor",
        "id": "fb6c3e19-e378-47e5-b277-9732f9de6e21"
    }
]
}

share invite with Editor(file or folder) and Uploader role API request returns 400 status code

send API request to list permission of testFolder item,

{
"@libre.graph.permissions.actions.allowedValues": [
    "libre.graph/driveItem/permissions/create",
    "libre.graph/driveItem/children/create",
    "libre.graph/driveItem/standard/delete",
    "libre.graph/driveItem/path/read",
    "libre.graph/driveItem/quota/read",
    "libre.graph/driveItem/content/read",
    "libre.graph/driveItem/upload/create",
    "libre.graph/driveItem/permissions/read",
    "libre.graph/driveItem/children/read",
    "libre.graph/driveItem/versions/read",
    "libre.graph/driveItem/deleted/read",
    "libre.graph/driveItem/path/update",
    "libre.graph/driveItem/permissions/delete",
    "libre.graph/driveItem/deleted/delete",
    "libre.graph/driveItem/versions/update",
    "libre.graph/driveItem/deleted/update",
    "libre.graph/driveItem/basic/read",
    "libre.graph/driveItem/permissions/update",
    "libre.graph/driveItem/permissions/deny"
],
"@libre.graph.permissions.roles.allowedValues": [
    {
        "@libre.graph.weight": 1,
        "description": "Allows upload file or folder",
        "displayName": "Uploader",
        "id": "1c996275-f1c9-4e71-abdf-a42f6495e960"
    },
    {
        "@libre.graph.weight": 2,
        "description": "Allows reading the shared file or folder",
        "displayName": "Viewer",
        "id": "b1e2218d-eef8-4d4c-b82d-0f1a1b48f3b5"
    },
    {
        "@libre.graph.weight": 3,
        "description": "Allows reading and updating file",
        "displayName": "Editor",
        "id": "2d00ce52-1fc2-4dbc-8b95-a73b73395f5a"
    },
    {
        "@libre.graph.weight": 4,
        "description": "Allows creating, reading, updating and deleting the shared file or folder",
        "displayName": "Editor",
        "id": "fb6c3e19-e378-47e5-b277-9732f9de6e21"
    }
]
}

Expected behavior

Editor(file or folder) and Uploader permission shouldn't listed for file resources and EDitor(Allows reading and updating file) should not be listed on folder resource

Actual behavior

four permission listed for both file and folder resource

Uploader
Viewer
Editor
Editor("Allows creating, reading, updating and deleting the shared file or folder")

Setup

Please describe how you started the server and provide a list of relevant environment variables or configuration files.

```console OCIS_XXX= master OCIS_YYY= 8.0.0-rc.1 PROXY_XXX=somevalue ```

Additional context

Add any other context about the problem here.

amrita-shrestha commented 9 months ago

cc @rhafer

rhafer commented 9 months ago

Hm, currently we can not do any better than that :disappointed:.

The two different editor rules are pretty ugly. Currently the condtitions on the roleDefinitions do not allow to distinguish between Folders and files. That's why we are always returning both roles.

Also we always return the full list of of actions, ignoring whether they makes sense for the requested resource.

We have to find at least some way to avoid the two different Editor roles.

micbar commented 8 months ago

@rhafer @butonic @fschade I propose to use the same "Editor" Role on files and folders. Semantically that should not break any behavior.

rhafer commented 6 months ago

I propose to use the same "Editor" Role on files and folders. Semantically that should not break any behavior.

Hm, looking at the reva code, I think that would break stuff. The Editor role does have the Delete Permission while the FileEditor role does not have that. I.e. assigning the Editor role to a user on a file, would allow that user to delete the file. Which we do not want I think.

rhafer commented 6 months ago

For the roles we might want to rethink our usage for the Condition property in RolePermissions. We currently allow the following there hardcoded conditions:

// UnifiedRoleConditionSelf defines constraint where the principal matches the target resource
UnifiedRoleConditionSelf = "@Subject.objectId == @Resource.objectId"
// UnifiedRoleConditionOwner defines constraints when the principal is the owner of the target resource
UnifiedRoleConditionOwner = "@Subject.objectId Any_of @Resource.owners"
// UnifiedRoleConditionGrantee does not exist in MS Graph, but we use it to express permissions on shared resources
UnifiedRoleConditionGrantee = "@Subject.objectId Any_of @Resource.grantee"

UnifiedRoleConditionSelf is basically unused in our codebase, we just have it because it's one of the two conditions that MSGraph defines :man_shrugging:

We use UnifiedRoleConditionOwner for indicating that a set of AllowedResourceActions can be applied to project spaces. (This condition is also defined in MSGraph as the "owner" condition.)

The third one UnifiedRoleConditionGrantee is not defined MSGraph. We use it do indicate that a set for AllowedResourceActions can be applied to other shared resource (files/directories)

IMO this is all but intuitive. It's hard to understand. The syntax is not explained anywhere (at least AFAIK). And the two conditions we're actively using look just arbitrary.

How about if we define a set of conditions that are somewhat more explicit. E.g. depending on the driveitem properties:

@Resource.File != null to match files
@Resource.Folder != null to match folders
@Resource.Root != null to match spaces

I must admit that I don't grasp the whole RolePermission.Conditions thing enough to judge whether this makes any sense. Who does? (Really!)

With the above condtions we could in theory just define a single Editor role that expands to different ResourceActions depending on the resource type it is assigned to. Something like:

func NewEditorUnifiedRole() *libregraph.UnifiedRoleDefinition {
    return &libregraph.UnifiedRoleDefinition{
        Id:          proto.String(UnifiedRoleEditorID),
        Description: proto.String("View, download, upload and edit."),
        DisplayName: "Can Edit"
        RolePermissions: []libregraph.UnifiedRolePermission{
            {
                AllowedResourceActions: convert(conversion.NewSpaceEditorRole()),
                Condition:              "@Resource.Root != null",
            },
            {
                AllowedResourceActions: convert(conversion.NewEditorRole()),
                Condition:              "@Resource.Folder != null",
            },
            {
                AllowedResourceActions: convert(conversion.NewFileEditorRole()),
                Condition:              "@Resource.File != null",
            },
        },
        LibreGraphWeight: proto.Int32(0),
    }
}

But in order to keep the "Descriptions" more accurate we could also keep the current roles as they are and just filter them properly using the new conditions.

@micbar @butonic I think this needs your input.

micbar commented 6 months ago

I would agree with the observation that the current conditions are not clean enough.

The suggestion is also ok, the naming is understandable.

Let us please decide quickly how we can handle the idea of a Role that can expand into different Resource Actions.

@butonic @kobergj @kulmann @JammingBen

We need to really understand what model makes it "easier" to understand. (harhar)

butonic commented 6 months ago

Regarding Edit vs Delete permissions

This discussion is quite old:

If you can edit a file you can delete all content. That can be reverted with revisions.
If you can delete a file the revisions are gone as well. This can be reverted with the trash. These are arguments against seperating the two permissions.

So why even differentiate between the two permissions?

Not all storage drivers support file individual revisions
Not all storage drivers support a trash These are arguments for soperating the two permissions.

IMO we need to seperate them.

Regarding the Condition Syntax

AFAICT it follows the Security Descriptor Definition Language (SDDL) for Conditional ACEs - Conditional expressions:

AttributeName Operator Value

I think rules like this will become relevant in the future:

Policy

Allow read access if
the user has logged in with a smart card,
is a backup operator, and
is connecting from a machine with Bitlocker enabled.

SDDL

D:(XA; ;FR;;;S-1-1-0; (Member_of {SID(Smartcard_SID), SID(BO)} && @Device.Bitlocker))

That should clarify the syntax. But we should probably

[ ] link to the SDDL conditional expressions docs in the libregraph openapi spec description for the unifiedRolePermission Condition.

Regarding other Conditions

I already deviated from MS graph because we needed permissions that only applied to shared resources, hence the "@Subject.objectId Any_of @Resource.grantee" condition.

Instead of @Resource.Root != null I would prefer exists @Resource.Root

AFAICT the Conditions do not need to be parsed by anyone, do they? So I'm ok with changing the way we write them, as long as they follow the SDDL Conditional Expressions.

rhafer commented 6 months ago

Regarding the Condition Syntax

AFAICT it follows the Security Descriptor Definition Language (SDDL) for Conditional ACEs - Conditional expressions:

AttributeName Operator Value

I think rules like this will become relevant in the future:

Policy
Allow read access if
the user has logged in with a smart card,
is a backup operator, and
is connecting from a machine with Bitlocker enabled.
SDDL
D:(XA; ;FR;;;S-1-1-0; (Member_of {SID(Smartcard_SID), SID(BO)} && @Device.Bitlocker))
That should clarify the syntax.

Yeah. Obviously. :exploding_head:

I really have strong doubts going down that rabbithole. But that's a discussion to be had when it becomes relevant.

Instead of @Resource.Root != null I would prefer exists @Resource.Root

Fine with me.

owncloud / ocis