-
Notifications
You must be signed in to change notification settings - Fork 3
Reject request when roleIDs not in context #111
base: master
Are you sure you want to change the base?
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. |
@butonic we need to solve this @individual-it @phil-davis when we solved that ^ we need your opinion on the impact of this for the usage of the provisioning API in the test suites. |
@butonic request from ocis-glauth against ocis-accounts with the empty query, which is now forbidden in ocis-accounts:
|
More context:
First request fetches the kopano user (successfully). The second request has an empty search query and therefor fails. |
tricky :-)
|
f6b8044
to
075617b
Compare
This PR enforces permission checks on all endpoints except:
1:
GetAccount
unconditionally2:
ListAccounts
when the request payload has a non-emptyquery
argumentWith this, it is required to have
roleIDs
in the service handler function context.a) for http requests we have a middleware in ocis-pkg that dismantles the
x-access-token
and writes the roleIDs into the context. Example:ocis-accounts/pkg/server/http/server.go
Line 59 in 4598747
b) for grpc/http2 requests you could create the context manually. 🤷 We already do this in grpc tests in ocis-settings. There are security impairments connected to this, but those will be solved separately.
Known issue: glauth makes a
ListAccounts
grpc request with an empty search query, just after login. This breaks the login. When that is solved we will see failing CI, because the provisioning API (see ocis-ocs) uses basic auth, but ocis-proxy doesn't implement basic auth. Thus the access token doesn't get created.outdated description
This is what should happen in the permission checks. No matter where the request is coming from (internal call from another service or external call that came through ocis-proxy) the service handler should reject the request when the required user context, in this case the roleIDs of a user, is missing.
We're facing three issues here:
1: ocis-proxy uses
ListAccounts
with a query for looking up a user: https://github.com/owncloud/ocis-proxy/blob/db95804af53dce6f73803ffa477e7e5eaef1573a/pkg/middleware/account_uuid.go#L24 This request to ocis-accounts happens with claims from a successful authentication. This step is about fetching the ocis account and building the user context which is required for all subsequent permission checks. I.e. in this state we can not run permission checks.2: as already known, the enforced permission check from this PR breaks ocis-ocs when using basic auth. For an oidc scenario we just need to setup a middleware from ocis-pkg like we did in ocis-accounts:
ocis-accounts/pkg/server/http/server.go
Line 59 in 4598747
3: ocis-ocs uses
ListAccounts
with a query for looking up sharees when trying to share files/folders in ocis-web. At the moment we only have an account management permission. For this we also need a share permission, add it to the default user role and implement a permission check in ocis-accounts, next to the account management permission.