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

Add restrictor for import_preview #781

Merged
merged 2 commits into from
Oct 3, 2023
Merged

Add restrictor for import_preview #781

merged 2 commits into from
Oct 3, 2023

Conversation

ostcar
Copy link
Member

@ostcar ostcar commented Sep 29, 2023

@r-peschke can you tell me, who can see the new collection import_preview?

@ostcar It is backend internal. The client receives the preview data with the response of the type specific json_upload-action. In the payload to import the data the client uses the id of the import_preview instance.

IMO you should restrict the whole collection always.

@ostcar ostcar added this to the 4.1 milestone Sep 29, 2023
@ostcar ostcar mentioned this pull request Sep 29, 2023
@jsangmeister
Copy link
Contributor

import_preview/name contains the import type. The user needs the respective manage permissions to execute the import, so I think he should also need them to see the preview. The list of current and planned permissions would therefore be:

  • topic: agenda_item.can_manage
  • account: OML can_manage_users
  • participant: user.can_manage
  • committee: OML can_manage_organization
  • motion: motion.can_manage

For the meeting permissions, you will need the meeting id. You could either extract it from the import data, which would require the AU service to need knowledge about the structure of this data (not a fan), or the backend needs to save it in an additional field meeting_id, which would be empty for the organization imports. Also, we should add an enum restricting the values of import_preview/name (also, type might be more appropriate than name then).

@r-peschke @ostcar what do you think?

@ostcar
Copy link
Member Author

ostcar commented Sep 29, 2023

Who can start the import? If only orga-managers (of some level) can do it, I could use the orga permission.

If a meeting admin can start the import, then I would need the meeting_id (please in a separate field) and only let the meeting admin see the data.

You could also add the user_id of the user, that started the import. Then it would be possible, that only this user (and the superadmin) can see the collection

@r-peschke
Copy link
Member

The client don't need to see the import_preview-collection!
The preview data is passed in the request response to the client.
The collection is, like the action_worker-collection, only used for internal use of the backend (transfer data from json_upload-action to be used in import-action.

import_preview/name contains the import type. The user needs the respective manage permissions to execute the import, so I think he should also need them to see the preview. The list of current and planned permissions would therefore be:

* `topic`: `agenda_item.can_manage`

* `account`: OML `can_manage_users`

* `participant`: `user.can_manage`

* `committee`: OML `can_manage_organization`

* `motion`: `motion.can_manage`

The AU don't need this collection at all!

For the meeting permissions, you will need the meeting id. You could either extract it from the import data, which would require the AU service to need knowledge about the structure of this data (not a fan), or the backend needs to save it in an additional field meeting_id, which would be empty for the organization imports. Also, we should add an enum restricting the values of import_preview/name (also, type might be more appropriate than name then).

@r-peschke @ostcar what do you think?

see above.

@r-peschke r-peschke removed their assignment Sep 29, 2023
@jsangmeister
Copy link
Contributor

Right, my bad! I still think the name/type should be constrained by an enum to ensure valid types, but I'll open a backend issue for that.

@ostcar ostcar marked this pull request as ready for review October 3, 2023 13:40
@ostcar
Copy link
Member Author

ostcar commented Oct 3, 2023

The autoupdate-service can not ignore a collection from the model.yml. The models.yml does not support such a ignore feature.

In my opinion, we have the models.yml to define the data needed by the backend, autoupdate and client. If the import_preview is only used as same sort of cache for the backend, then the question is, if the models.yml is the right place.

The action_worker-collection is not a similar case, since the client gets the backend responce via the autoupdate-service.

I added a restriction, so nobody (except the superadmin) can see the collection. This works.

@ostcar ostcar merged commit e2e6b3b into OpenSlides:main Oct 3, 2023
2 checks passed
@ostcar ostcar deleted the models branch October 3, 2023 13:44
@r-peschke
Copy link
Member

The autoupdate-service can not ignore a collection from the model.yml. The models.yml does not support such a ignore feature.

In my opinion, we have the models.yml to define the data needed by the backend, autoupdate and client. If the import_preview is only used as same sort of cache for the backend, then the question is, if the models.yml is the right place.

In my opinion the models.yml essentially is the place to describe the database structure. Using this db-structure is welcome, but an addon.

The action_worker-collection is not a similar case, since the client gets the backend responce via the autoupdate-service.

ok, but this is only an addon to look at the state of some action-process. The principal task is to
control the process.

I added a restriction, so nobody (except the superadmin) can see the collection. This works.

Ok, but at the moment nobody needs this permission, even the superadmin.

@ostcar
Copy link
Member Author

ostcar commented Oct 9, 2023

I added a restriction, so nobody (except the superadmin) can see the collection. This works.

Ok, but at the moment nobody needs this permission, even the superadmin.

So why do you send changes to the collection on the message bus?

@r-peschke
Copy link
Member

I added a restriction, so nobody (except the superadmin) can see the collection. This works.

Ok, but at the moment nobody needs this permission, even the superadmin.

So why do you send changes to the collection on the message bus?

Because the datastore base send it for all changes. Why do we should make an exception? At the moment there is nobody with a subscription.

@jsangmeister
Copy link
Contributor

Additionally, the datastore has no knowledge of the different collections, so we would have to build a way to differentiate between import_preview and other collections.

peb-adr added a commit that referenced this pull request Oct 18, 2023
* commit '84c1bfc03a8ea54e9a0dec9f431d1d50e110da67':
  Bump github.com/klauspost/compress from 1.17.0 to 1.17.1 (#787)
  Bump github.com/alecthomas/kong from 0.8.0 to 0.8.1 (#785)
  Bump golang from 1.21.1-alpine to 1.21.2-alpine (#784)
  Bump golang.org/x/sys from 0.12.0 to 0.13.0 (#783)
  Bump golang.org/x/sync from 0.3.0 to 0.4.0 (#782)
  Add restrictor for import_preview (#781)
  Fix generic relation keys with fields that only exist on one field (#780)
  Bump leonsteinhaeuser/project-beta-automations from 2.1.0 to 2.2.1 (#778)
  Move profiler to internal route (#770)
  Reset cache on projector reset (#776)
  Bump github.com/klauspost/compress from 1.16.7 to 1.17.0 (#773)
  Bump golang.org/x/sys from 0.11.0 to 0.12.0 (#774)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants