Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

Filter settings by permissions #48

Merged
merged 31 commits into from
Aug 26, 2020
Merged

Conversation

kulmann
Copy link
Contributor

@kulmann kulmann commented Aug 19, 2020

This PR introduces filtering for settings and bundles, based on permissions.

Implementation for owncloud/product#99

So far, it is only about reading and listing settings, not about permission checks for saving anything. That has to come in a separate PR, ticket raised here https://github.com/owncloud/ocis-settings/issues/58

@kulmann kulmann changed the base branch from master to add-roles-service August 19, 2020 13:56
@kulmann kulmann self-assigned this Aug 19, 2020
@kulmann kulmann force-pushed the filter-settings-by-permissions branch from 29e4253 to 4f1ad8d Compare August 19, 2020 14:13
@kulmann kulmann mentioned this pull request Aug 19, 2020
28 tasks
@kulmann kulmann changed the base branch from add-roles-service to master August 20, 2020 11:02
@kulmann kulmann force-pushed the filter-settings-by-permissions branch from 83d8ff4 to be6d025 Compare August 21, 2020 08:33
@kulmann kulmann force-pushed the filter-settings-by-permissions branch from 9170e4d to 0c1146e Compare August 21, 2020 15:29
@kulmann
Copy link
Contributor Author

kulmann commented Aug 21, 2020

@individual-it fyi, I needed to rebase this PR to get #56 in

@individual-it individual-it force-pushed the filter-settings-by-permissions branch from 3b3a8a6 to 41db407 Compare August 24, 2020 04:24
}{
{"space",
" ",
"{\"id\":\"ocis-settings\",\"code\":400,\"detail\":\"must be in a valid format\",\"status\":\"Bad Request\"}",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using the go-micro errors in our handlers now. You can define expected errors in tests like this:
merrors.New("ocis-settings", "must be in a valid format", 400),
@individual-it

@kulmann
Copy link
Contributor Author

kulmann commented Aug 24, 2020

@individual-it

  • I removed the accountUUID from the ListBundles and ListRoles requests and take the account uuid from the context instead (like we discussed this morning).
  • ListBundles and GetBundle require to have the account uuid in the context. The GetBundle request now also checks if the authenticated user (= account uuid in the context) has permissions to read the bundle.
  • ListBundles and GetBundle check permissions for the operations READ and READWRITE now, instead of UPDATE

Can you adjust tests accordingly?

The only thing I would like to add to this PR is, that the SaveValue request should require the CREATE, UPDATE, WRITE or READWRITE permission and that the GetValue and ListValues requests should filter for READ or READWRITE permissions.

@individual-it
Copy link
Member

if GetBundle does not have an UUID is that the error message: {"id":"ocis-settings","code":404,"detail":"%!s(\u003cnil\u003e)","status":"Not Found"}?

@kulmann
Copy link
Contributor Author

kulmann commented Aug 25, 2020

if GetBundle does not have an UUID is that the error message: {"id":"ocis-settings","code":404,"detail":"%!s(\u003cnil\u003e)","status":"Not Found"}?

No, the details should not be nil, that's a bug in the code. Other than that it looks good. I will change the code to return proper error details.

@kulmann
Copy link
Contributor Author

kulmann commented Aug 25, 2020

@individual-it pushed a commit that fixes the error details on GetBundle. Now looks like this:

micro call com.owncloud.api.settings BundleService.GetBundle '{"bundle_id": "38071a68-456a-4553-846a-fa67bf5596cc"}'

error calling com.owncloud.api.settings.BundleService.GetBundle: {"id":"ocis-settings","code":404,"detail":"could not read bundle: 38071a68-456a-4553-846a-fa67bf5596cc","status":"Not Found"}

@kulmann
Copy link
Contributor Author

kulmann commented Aug 25, 2020

@individual-it

I have managed to stabilize the grpc tests. In addition to the init() function I added a setup() and teardown() mechanism, which a) takes care that the default roles are recreated for each test and b) that the data folder gets deleted after each test.

There is also one helper at the bottom of the file now, which sets a full readWrite permission on a bundle for an account. When you look at the code where it's used, you will also find examples for using the account uuid on the new metadata context.

There are two important next tests for this PR:

  • save a bundle with multiple settings, but only adding readwrite permission for one of the settings in the bundle. Then showing that the list request will return the bundle, but only with that one setting.
  • save a bundle without giving permission to it or any of it's settings afterwards. Then show, that on a GetBundle request a 404 error is returned.

I can assist in the morning. I would like to finalize this PR before the standup with just these two tests. Everything else can happen in a separate PR. Especially all the roles related tests are not needed in this PR but can happen separately.

@kulmann kulmann marked this pull request as ready for review August 26, 2020 05:58
Copy link
Contributor

@C0rby C0rby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants