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

Roles and permissions #35

Closed
wants to merge 85 commits into from
Closed

Roles and permissions #35

wants to merge 85 commits into from

Conversation

kulmann
Copy link
Member

@kulmann kulmann commented Jul 7, 2020

Work In Progress 🚧

Implementation for owncloud/product#85
Also fixes #32

Required cleanup before review/merge:

  • bring back the commented tests
  • document folder structure of bundles and values
  • replace path with filepath
  • protect all API endpoints with respective permissions
    • on writing values (especially when the values are meant to be global, i.e. when setting has resource type SYSTEM)
    • on listing settings bundles (separate from listing roles)
    • on saving a bundle
    • on reading a bundle
    • on adding a setting to a bundle
    • on removing a setting from a bundle
    • on listing values
    • on saving a value
    • on reading a value
    • on listing roles
    • on listing role assignments
    • on adding role assignments
    • on removing role assignments
  • additional check: is the authenticated user (account UUID in the access token) allowed to perform the request for the account UUID in the request?
  • avoid duplicates when adding role assignments

Create and merge pull requests for adapting to the new data model and using the new roles & permissions system in

  • ocis-accounts: adapt to new keys in settings and settings bundles
  • ocis-proxy: new middleware for fetching roles and minting them into the token
  • ocis-hello: change registering settings bundles to new identifier
  • owncloud-sdk: copy the regenerated api client from ocis-settings into the vendor dir and adapt code to new identifier
  • phoenix: release with new owncloud-sdk and data model changes
  • ocis-phoenix: release with new phoenix

@refs
Copy link
Member

refs commented Jul 8, 2020

@kulmann made some changes to align the mount path for the settings and as a result got the following tree:

/var/tmp/ocis-settings
❯ tree .
.
├── bundles
│   └── ocis-settings
│       ├── admin.json
│       ├── guest.json
│       └── user.json
└── ocis-settings
    └── bundles
        └── ocis-settings
            ├── admin.json
            ├── guest.json
            └── user.json

5 directories, 6 files

The nested ocis-settings seems redundant.

@refs
Copy link
Member

refs commented Jul 8, 2020

With my patch now the folder structure is:

/var/tmp/ocis-settings
❯ tree /var/tmp/ocis-settings
/var/tmp/ocis-settings
└── bundles
    └── ocis-settings
        ├── admin.json
        ├── guest.json
        └── user.json

2 directories, 3 files

Testing to check the core functionality doesn't break.

kulmann and others added 25 commits July 20, 2020 14:46
As tracked in #32 we
want to have consistent naming for the identifier fields. This is better
now.
- a SettingsBundle is about a resource. One type of resource is the
system itself.
- a Setting is about a resource as well. In this, a SettingsBundle could
be about a group and settings about specific resources (e.g. files or
users) of that group.
- PermissionSettings exist for allowing an operation on the given resource.
- bundles are now stored in a path depending on the resource provided
with the settings bundle request (list/get/save)
- added basic unit tests (still without assertions) from @refs

Co-authored-by: Benedikt Kulmann <benedikt@kulmann.biz>
Co-authored-by: Alex Unger <zyxancf@gmail.com>"
Added two new endpoints for SettingsBundle management to our protobuf
definition:
- AddSettingToSettingsBundle
- RemoveSettingFromSettingsBundle
We were using triple-identifiers for settings (extension-name,
bundle-key, setting-key) and tuple-identifiers for
bundles (extension-name, bundle-key). As it turns out this is not
working out well for referencing e.g. settings from within other
settings, which is required for adding permissions as settings. Thus we
changed our protobuf definition to have uuids insteads of complex
identifiers.
Along with changing to identifiers we needed to rewrite our persistence
implementation. GRPC tests are broken and will be fixed in one of the
next commits.
Reason for this is, that writing the empty role definition again would
remove all settings / permissions that were already added to the role by
external services.
At the moment the permissions check statically returns true (next step
for the implementation), so this just wires everything to have a
permission check in place.
It is needed in frontend code to walk settings by their names. Not very
developer-friendly to require the knowledge of the correct UUID for a
setting.
@kulmann kulmann mentioned this pull request Aug 19, 2020
@kulmann kulmann mentioned this pull request Aug 19, 2020
@kulmann
Copy link
Member Author

kulmann commented Aug 19, 2020

For the record, we moved the code into four separate PRs:

@kulmann
Copy link
Member Author

kulmann commented Aug 19, 2020

Tracked account uuid validation here https://github.com/owncloud/ocis-settings/issues/50

@kulmann
Copy link
Member Author

kulmann commented Aug 19, 2020

this entire PR was split into multiple other PRs and is not needed anymore.

@kulmann kulmann closed this Aug 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consistency in Identifier fields
2 participants