Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Shares] New endpoint that announces (sharing) roles for clients #4848

Closed
michaelstingl opened this issue Oct 18, 2022 · 15 comments
Closed

[Shares] New endpoint that announces (sharing) roles for clients #4848

michaelstingl opened this issue Oct 18, 2022 · 15 comments
Labels
Category:Enhancement Add new functionality

Comments

@michaelstingl
Copy link
Contributor

oCIS comes with a new, roles-based sharing concept. It would be beneficial, if clients wouldn't need to hardcode the different roles and permissions, but instead could request them from the backend, to also support future changes. (new, dynamic roles?)

Hardcoded example from ownCloud web:
https://github.com/owncloud/web/blob/master/packages/web-client/src/helpers/share/role.ts

For inspiration, maybe check the oC10 implementation for roles-based public links:

/cc @TheOneRing @felix-schwarz @kulmann @micbar @tbsbdr @pmaier1

@felix-schwarz
Copy link

felix-schwarz commented Oct 19, 2022

For the iOS SDK, I used role.ts and using the actual web UI as a guide to get an overview of the many different roles and how they technically differ under the hood depending on resource (folder, file, whole space, …) and target (user, group, link, …).

I ended up with this list of roles: https://github.com/owncloud/ios-sdk/blob/feature/graph-api/ownCloudSDK/Core/Sharing/OCCore%2BSharing.m#L360

They can all be described with these properties:

Property Type Description
type string abstract type for internal identification (viewer, uploader, editor, contributor, manager, custom)
identifier string unique identifier per role (f.ex. Viewer, SpaceViewer, …)
shareTypes array/flags types of share (link, group, user) covered by this scope
permissions array/flags permissions set on this share (read, update, create, delete, share)
customizablePermissions array/flags permissions contained in permissions that the user is allowed to change for this role. Typically empty/0 except for the roles of type = custom. By including a permission (f.ex. read) in permissions, but not in customizablePermissions, the permission is set and can't be turned off by the user.
locations array/flags Locations that this role can be used with (file, folder, drive)
iconName string Name of the image resource to use as an icon.
roleName string Localized name of the scope.
roleDescription string Localized description for the scope.
isLocalized bool (optional) Indicates if roleName and roleDescription are localized. If false, these strings are in English and localization should take place on the respective client. (compromise / intermediate solution, see below)

What I imagine an API to look like:

General properties

  • JSON-based
  • uses arrays of strings instead of flags for properties such as permissions(f.ex. ["read", "share"] instead of 17 (= 1 | 16))
  • localized parts use the Accept-Language header to return strings in a language matching the client's UI. If server-side localization is too far off the roadmap at this point, a good compromise could be to return them in English and indicate via isLocalized to the client that it should run those strings through its native/local localization capabilities.

Actual API

One endpoint allowing two different types of query:

  1. list: list of all available roles: requires no parameter to be passed
  2. item: list of available roles for a particular item. The item is identified…
  • for items: by a pair of parameters: driveID and path (or driveID + fileID - whatever suits the server best).
  • for whole drives: just by driveID

That moves the logic which roles are available for which item entirely to the server.

Client usage

Clients can use the result of the item query to determine

  • which roles to offer to the user
  • which role is active for a specific share by going through the list of roles and
  1. checking if type covers the share type

a) comparing share permissions (bitfield value) with the share's permissions.
b) if that doesn't turn up any match, comparing if share permissions (bitfield value) are covered by a custom role and fully covered by the role's permissions

Alternative:

  • each role could gain an additional identifier property that's stable + unique for every role
  • the shares endpoints add the active roleID to the set of information they return for each share

@micbar
Copy link
Contributor

micbar commented Oct 19, 2022

@michaelstingl @felix-schwarz Thanks for the writeup.

The sharing roles and permissions should be maintained by the backend and are different per space. That is already the case today. A Viewer (sharing in personal spaces) already has different permissions than a SpaceViewer.

IMO we need unique identifiers per sharing role. The bitmask will completely disappear because we do not have a 1:1 relationship between bitmask and a role anymore.

@micbar micbar added this to the 2.1.0 Service Pack 1 milestone Oct 19, 2022
@micbar
Copy link
Contributor

micbar commented Oct 19, 2022

@pmaier1 @tbsbdr For me this is a post GA topic.

@tbsbdr
Copy link
Contributor

tbsbdr commented Oct 19, 2022

already on the (still raw) list 

Roles and Permissions: allow customizations

@felix-schwarz
Copy link

felix-schwarz commented Oct 19, 2022

@michaelstingl @felix-schwarz Thanks for the writeup.

The sharing roles and permissions should be maintained by the backend and are different per space. That is already the case today. A Viewer (sharing in personal spaces) already has different permissions than a SpaceViewer.

I just added identifier to the table in the proposal above.

IMO we need unique identifiers per sharing role. The bitmask will completely disappear because we do not have a 1:1 relationship between bitmask and a role anymore.

Yeah, I noticed the bitmask values are different depending on file/folder/space.

@pmaier1 @tbsbdr For me this is a post GA topic.

Ok. Until then, is there documentation available describing the current sharing API? (especially regarding changes from OC10's)

@pmaier1
Copy link
Contributor

pmaier1 commented Oct 19, 2022

already on the (still raw) list
Roles and Permissions: allow customizations

This is a different topic. The point here is to allow the clients to retrieve the available roles in sharing from the server in order to avoid hardcoding them in every client.

Agree that it is a post-GA topic. It should be tackled together with enabling sharing on the native clients.

@micbar micbar added the Category:Enhancement Add new functionality label Dec 15, 2022
@michaelstingl michaelstingl changed the title [Shares] New endpoint that announces roles for clients [Shares] New endpoint that announces (sharing) roles for clients Jan 11, 2023
@tbsbdr
Copy link
Contributor

tbsbdr commented Jan 11, 2023

highlevel meeting:

  • cross platform alignment before sprintstart needed @micbar
  • roles.ts from web should be listed in graph endpoint

@michaelstingl
Copy link
Contributor Author

  • roles.ts from web should be listed in graph endpoint

@felix-schwarz would it help to expose what we currently have in role.ts in a new "official" endpoint, and add the nice stuff from #4848 (comment) in new versions?

@butonic
Copy link
Member

butonic commented Jan 11, 2023

After a deep dive into the ms graph appRoles and appRoleAssignments in #5318 (comment) I am quite sure that we will at some point have to overhaul the application permissions. We are currently misusing appRoles and appRoleAssignments or application permissions to do what delegated permissions are supposed to do.

That being said, neither concept is the right thing to misuse for sharing roles. I'd still prefer to have a graph API endpoint that extends the concept of the ms graph sharing roles property as defined in https://learn.microsoft.com/en-us/graph/api/resources/permission?view=graph-rest-1.0#properties as

Property Type Description
roles Collection of String The type of permission, for example, read. See below for the full list of roles. Read-only.

So, actually roles here corresponds to oCIS / oc10 sharing permissions as human readable strings.

I propose building on top of this api by introducing a new sharingRoles resource in the libregraph api which allows fetching a ̀ list of roles with a display_name and roles property, where roles is an array of strings, corresponding to the roles in the MS graph sharing permissions above.

If this endpoint does not exist or does not respond, clients can fall back to a hardcoded set or just display custom permissions. When the list of sharingRoles contains a role with matching set of 'roles' aka permissions the display_name is used instead.

@butonic
Copy link
Member

butonic commented Jan 11, 2023

hm then again, the sharingRoles might be specific to an application, eg collabora, onlyoffice in addition to the ocis internal permissions. so we may also need an applicationID property in the list of sharingRoles:

Property Type Description
display_name String A descriptive human readable name, eg Viewer, Editor, Manager File Drop ...
roles Collection of String The type of permission, for example, create, read, write, delete, share, comment ...
applicationID String Corresponds to an application resource

Two problems:

  1. display_name is not translated ... should the api return a translated name?
  2. roles are string names not ids. While this allows seamlessly extending the existing MS Graph sharing roles I can understand the desire to use uuids ... also to prevent collisions between multiple applications ... or we extend applications so sharingRoles are a property of applications ...

🤔

@felix-schwarz
Copy link

@michaelstingl At least for the iOS client, I see no benefit in having the information from role.ts exposed via a new endpoint as the iOS app's SDK already has that info - and the information in role.ts is not in a format/shape right now that is easily JSON-encodable.

Better to focus on a stable API. Given that the iOS SDK's internal implementation of this is just around ~200 lines of code - with most of these lines spent on all the role definitions - I imagine the effort/time needed for an initial server-side implementation of what I outlined above should be relatively small.

One thing that we haven't discussed yet, however, is what clients should do when connecting to an OC 10 server. If clients should offer the same, role-based UI for these, I see two possibilities:

  1. implement the same server-side API for OC10. But this would require everyone to upgrade their server version, so probably a no-go.
  2. clients still ship with a built-in fallback set of roles as well as the ability to filter them to match an item/share. The logic the server uses to return possible roles for the item endpoint above should then be documented, so clients can implement the same logic as a fallback for (OC10) servers that don't offer the server-side capabilities.

@michaelstingl
Copy link
Contributor Author

No changes planned in oC10. They'll keep the bitmask implementation. When oC10 changes, then they should add the new endpoint too.

@butonic
Copy link
Member

butonic commented Jan 12, 2023

In order to extend the ms graph resources with our concepts, eg. to use permission ids instead of roles (aka named permissions like read, write) I propose a fifth extension type, in addition to the four defined by ms graph:

Similar to Directory extensions libregraph properties we introduce should be prefixed with lg_. This is similar to the extension_{id}_ prefix of Directory extensions but uses a shorter prefix.

@micbar a lg_ prefix would also allow us to extend the normal drive resource with our space specific properties while making obvious were we diverge.

Discarded ideas:

Directory Extensions

They cannot be registered on permissions. They contain a random id which is generated when registering the extension.
We could use extension_libregraph_ but I think that is too long for a prefix.

Use any other of the original four ways to extend ms graph

Each extension can only be applied to specific resource types. In this case permission is not extendable by any of them and the permissions roles property is using names instead of identifiers, so we would be changing the type and meaning if we would just list our permission ids.

Odata Instance Annotations

The example shows that annotations really are meant to add data outside the scope of the actual resource, like influencing rendering:

{
  "@context": "http://host/service/$metadata#Customers",
  "@com.example.customer.setkind": "VIPs",
  "value": [
    {
      "@com.example.display.highlight": true,
      "ID": "ALFKI",
      "CompanyName@com.example.display.style": { "title": true, "order": 1 },
      "CompanyName": "Alfreds Futterkiste",
      "Orders@com.example.display.style#simple": { "order": 2 }
    }
  ]
}

Dot or colon separated prefix

Using a prefix like lg: or lg. is unnecessarily hard or when trying to express json paths like root.permissions.lg_permissions.

@butonic
Copy link
Member

butonic commented Jan 12, 2023

odata supports localization via the Accept-Language header, and ms graph makes use of it eg. in the organizationalBranding

The RFC recommends not to return a 406 Not Acceptable status because end users may be able to use the page with translation software. I think on the libragraph API we should return a 406 if we have no translation for the accepted language. When the client wants to use his translation he can leave out the header, or he can sand a language header and retry if he gets a 406.

@rhafer
Copy link
Contributor

rhafer commented Mar 5, 2024

We have https://owncloud.dev/libre-graph-api/#/roleManagement now so I guess this can be closed.

@rhafer rhafer closed this as completed Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Enhancement Add new functionality
Projects
None yet
Development

No branches or pull requests

7 participants