-
Notifications
You must be signed in to change notification settings - Fork 182
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
graph: add appRoleAssignments and minimal application resource #5318
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
25467f0
to
25ef08c
Compare
💥 Acceptance test localApiTests-apiSpacesShares-ocis failed. Further test are cancelled... |
1df02b4
to
0341a7b
Compare
}} | ||
}, nil) | ||
|
||
r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$expand=appRoleAssignments", nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- the
/graph/v1.0/me/users
URLs further above are all bonkers -> prepare a subsequent PR to correct them
@@ -74,7 +74,7 @@ var _ = Describe("EducationUsers", func() { | |||
service.WithGatewayClient(gatewayClient), | |||
service.EventsPublisher(&eventsPublisher), | |||
service.WithIdentityEducationBackend(identityEducationBackend), | |||
//service.WithRoleService(roleService), | |||
service.WithRoleService(roleService), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this now works because I actually use the Role service passed in service.go
if options.RoleService == nil {
svc.roleService = settingssvc.NewRoleService("com.owncloud.api.settings", grpc.DefaultClient())
} else {
svc.roleService = options.RoleService
}
@@ -54,7 +54,7 @@ require ( | |||
github.com/onsi/ginkgo/v2 v2.5.0 | |||
github.com/onsi/gomega v1.24.1 | |||
github.com/orcaman/concurrent-map v1.0.0 | |||
github.com/owncloud/libre-graph-api-go v1.0.1 | |||
github.com/owncloud/libre-graph-api-go v1.0.2-0.20230105141655-9384face4d5d |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- bump to a release
5ed604b
to
307b8bd
Compare
@micbar @kulmann the permissions in msgraph have a technical layout:
I recommend skimming the MS Graph permissions reference to get a feeling of how granular they are. Regarding appRoleAssignments I found this explanation that clarifies how the value property of an appRole is used:
AFAICT there is no direct permission lookup mechanism. Instead, the naming pattern of the role value
As given in the table above Our space managemer role should be Well ... there are only
To me it seems that they express specific permissions through a 'custom' constraint like Oh ... they actually have a list of Permission IDs, eg.:
So what are delegated permissions? Lets take the files delegated permissions:
vs the Files application permissions:
Maybe it helps to clarify what delegated vs application permissions are:
or in short:
But ... if I look at the comparison ... :
... access on behalf of users should be granted by an oAuth2PermissionGrant. Its Properties have a
and they can be listed at:
An interesting aside is Grant or revoke API permissions programmatically ... it also talks about granting delegated access to an application on behalf of all users in a tenant... The Thinking about our space managemer role, why is there no mention of an admin that can create additional drives? AFAICT because all groups implicitly have a drive, which means all classes get a drive ... but who can manage quota? ah yeah the msgraph does not expose an api for that ... duh Ok, looking at the permissions reference, |
Trying to map our current roles to MS Graph permissions: First a set of relevant permissions:
Then our current user roles would be expressed as
Taking the list of roles and permissions @mmattel came up with in https://github.com/owncloud/docs-ocis/pull/268/files
This can be rewritten as (remember: no constraint = Own):
It seems the settings and read write language cannot be expressed that granular. They all fold into |
554b502
to
a52ef00
Compare
328f24b
to
3b81f43
Compare
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
…AppRoleAssignment, configurable app id and displayname Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
…ng and logging Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, performance needs to be looked at later.
Co-authored-by: Michael Barz <mbarz@owncloud.com>
Kudos, SonarCloud Quality Gate passed! |
Author: Jörn Friedrich Dreyer <jfd@owncloud.com> Date: Thu Jan 12 16:09:34 2023 +0100 graph: add appRoleAssignments and minimal application resource (#5318) * bump libregraph-go lib Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * add appRoleAssignment stubs Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * add get application stub Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fetch appRoles for application from settings service Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * initial list appRoleAssignments implementation Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * initial create appRoleAssignment implementation, extract assignmentToAppRoleAssignment, configurable app id and displayname Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * initial delete appRoleAssignment implementation, changed error handling and logging Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * initial expand appRoleAssignment on users Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * test user expand appRoleAssignment Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * test appRoleAssignment Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fix education test by actually using the mocked roleManager Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * test getapplication Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * list assignments Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * use common not exists error handling Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * default to just 'ownCloud Infinite Scale' as application name Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fix store_test Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * roll application uuid on init Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * fix tests Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * extract method Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> * Apply suggestions from code review Co-authored-by: Michael Barz <mbarz@owncloud.com> Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de> Co-authored-by: Michael Barz <mbarz@owncloud.com>
|
||
// Application defines the available graph application configuration. | ||
type Application struct { | ||
ID string `yaml:"id" env:"GRAPH_APPLICATION_ID" desc:"The ocis application id shown in the graph. All app roles are tied to this."` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@butonic could you add a changelog item here? Just pulled the latest oCIS docker image and had it fail with an error until I added the GRAPH_APPLICATION_ID
;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AKA "I went to the changelog first, didn't find anything obvious and ended up here in your PR"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, yes ... I hope you got at least a meaningful error message?
Unfortunately, rolling a random application id also is also suboptimal because we would have to write it to the yaml config, which might be readonly ... I'm thinking about a solution for that. maybe store it in the metadata instead of the yaml file ...
For now, I decided to intdroduce an application that is configured with
GRAPH_APPLICATION_ID
andGRAPH_APPLICATION_DISPLAYNAME
. They are necessary to represent an application that owns the roles. In our case that is oCIS.First question: is it only oCIS Web? I currently default to
"oCIS Web"
.Second question: the application id should be randomized on init. AFAICT it resembles the oc10 instance id. Although I'm not sure if it is the web client or the server side, see first question. The more I type the more I think it is the server side.
Third question: if we make the id configurable then we require clients to know the application id in advance. Should we implement listing applications at
/applications
? That way clients could discover the applications that are available. The WebUI could discover not only ocis web, but also onlyoffice and other applications. I strongly encourage everyone to check out the MS Graph Application Properties. It has things like a logo stream, endpoints for redirects of web applications, links to terms of service, support urls, ... If we entertain a publicly hosted oCIS web instance that could discover a graph endpoint using webfinger, similar to the openid connect discovery, being able to discover the application id, a displayname and a logo is desireable. Also ... I don't want to have to configure the wab ui to know the application id in advance. An in case the user management web ui (soon to become admin web ui) encounters multiple applications it will have to be able to deal with more than one application and multiple sets of appRoleIDs.In light of the above, I propose to use just
"oCIS"
as the default application name, roll a uuid for the applcation id onocis init
and implement the ListApplications endpoint.@kulmann @michaelstingl @felix-schwarz I just noticed how ms graph allows applications to register as a handler for certain filetypes: