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

Open resource links from any organization/sandbox #5892

Merged
merged 90 commits into from
May 26, 2023

Conversation

klakhov
Copy link
Contributor

@klakhov klakhov commented Mar 21, 2023

Motivation and context

Server part:

  • Update swagger docs
  • For object requests: define iam_context according to the obj.organization_id
  • For list and create requests: iam_context is defined according to the query parameters org_id or org (slug)
  • Fix tests

UI part:

  • Change organization when link leads to resource in different organization
  • Fix tests

How has this been tested?

Checklist

  • I submit my changes into the develop branch
  • I have added a description of my changes into the CHANGELOG file
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • I have linked related issues (see GitHub docs)
  • I have increased versions of npm packages if it is necessary
    (cvat-canvas,
    cvat-core,
    cvat-data and
    cvat-ui)

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.

@klakhov klakhov requested a review from bsekachev as a code owner March 21, 2023 08:35
@sizov-kirill sizov-kirill changed the title [WIP] Open resource links from any organization/sandbox Open resource links from any organization/sandbox Apr 10, 2023
@sizov-kirill sizov-kirill changed the title Open resource links from any organization/sandbox [WIP] Open resource links from any organization/sandbox Apr 10, 2023
@sizov-kirill sizov-kirill changed the title [WIP] Open resource links from any organization/sandbox Open resource links from any organization/sandbox Apr 11, 2023
@nmanovic
Copy link
Contributor

I got the error below when I requested a task.

Could not receive the requested project from the server
Error: Request failed with status code 403. {"detail":"You do not have permission to perform this action."}.

@nmanovic
Copy link
Contributor

@kirill-sizov , could you please look at /api/organizations request. The number of queries are growing together with number of organizations. Indeed I believe the number of requests should be less than 10. Please try to minimize.

Screenshot from 2023-05-23 20-29-41

@nmanovic
Copy link
Contributor

@kirill-sizov , /api/jobs request doesn't work in debug at least. Could you please look?

Screenshot from 2023-05-23 20-36-04

@sizov-kirill
Copy link
Contributor

sizov-kirill commented May 25, 2023

@nmanovic I fixed the performance problem and the problem with getting jobs. But confused about this issue:

I got the error below when I requested a task.

Could not receive the requested project from the server
Error: Request failed with status code 403. {"detail":"You do not have permission to perform this action."}.

I have checked accessing tasks with different parameters of privileges and organization roles (this is also covered by our test) and couldn't reproduce this problem. Could please clarify how did you get this error?

Did you get this error in the development environment or in docker?
Does this task belong to the organization or project?
Which privilege does the requester have?
Which organization role does the requester have?
Does the requester assign to the task or corresponding project?

@nmanovic nmanovic merged commit ed3dbe8 into develop May 26, 2023
@nmanovic nmanovic deleted the kl/remove-org-from-id-requests branch May 26, 2023 19:01
return []

return [
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't these values be aligned with the ORGANIZATION_OPEN_API_PARAMETERS somehow? The code can easily diverge.

@@ -228,4 +229,27 @@ class CloudStorageReadSerializerExtension(_CloudStorageSerializerExtension):
class CloudStorageWriteSerializerExtension(_CloudStorageSerializerExtension):
target_class = 'cvat.apps.engine.serializers.CloudStorageWriteSerializer'

ORGANIZATION_OPEN_API_PARAMETERS = [
Copy link
Contributor

@zhiltsov-max zhiltsov-max May 26, 2023

Choose a reason for hiding this comment

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

When this should be used? Shouldn't it be documented and declared in IAM or in the organizations apps?

Copy link
Contributor

Choose a reason for hiding this comment

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

We use this in "schema" decorators for views like here. But yes I agree probably it would be more logical to move it in iam

Comment on lines +15 to +16
# Filter works only for "list" requests and allows to return
# only non-organization objects if org isn't specified
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, these parameters should be documented somehow, especially the (past?) logic about org=''/org=None etc.

@@ -107,35 +78,6 @@ def get_security_definition(self, auto_schema):
return [sessionid_schema, csrftoken_schema]

class CustomAutoSchema(AutoSchema):
Copy link
Contributor

@zhiltsov-max zhiltsov-max May 26, 2023

Choose a reason for hiding this comment

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

This class can be moved to engine if there are no IAM specifics added. The only reason it is here is that we can't define it in the engine and import and inherit here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving this class in apps/engine/schema.py still leads to error due to this line

if oid is None:
return instance.get_organization_id()
return oid
return getattr(instance, "organization_id", None)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this attr be enforced on the objects somehow? It's easy to miss it and it won't be noticed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently objects that have foreign key to Organization has organization_id attribute, for those models that don't have explicit relation with Organization has get_organization_id attribute. This logic is used in few places of code: events, webhooks, permissions. Currently I don't know how this can be done without defining organization_id in each model

Copy link
Contributor

@zhiltsov-max zhiltsov-max left a comment

Choose a reason for hiding this comment

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

Do you know if we can do anything about places like this https://github.com/opencv/cvat/blob/develop/cvat/apps/engine/views.py#L1832 ?

Copy link
Contributor

@zhiltsov-max zhiltsov-max left a comment

Choose a reason for hiding this comment

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

@sizov-kirill sizov-kirill mentioned this pull request May 29, 2023
11 tasks
@sizov-kirill
Copy link
Contributor

Do you know if we can do anything about places like this https://github.com/opencv/cvat/blob/develop/cvat/apps/engine/views.py#L1832 ?

Yes I agree, case with Labels is unusual. Currently the least laborious solution I can suggest is to change iam_organization_field(str) to iam_organizationd_fields(tuple), which would allow multiple organization fields for objects like Label. But I would suggest to postpone this idea until we have other such objects besides labels.

@zhiltsov-max
Copy link
Contributor

But I would suggest to postpone this idea until we have other such objects besides labels.

There is a couple more in the Honey Pot feature endpoints. Probably, an option to provide a list or even a mapping would be useful, similarly to what is done for other list filters.

nmanovic pushed a commit that referenced this pull request May 31, 2023
### Motivation and context
Applied comments:
- [x] Aligning ORGANIZATION_OPEN_API_PARAMETERS
#5892 (comment)
- [x] Moving ORGANIZATION_OPEN_API_PARAMETERS
#5892 (comment)
~~- [ ] Moving CustomerAutoSchema
#5892 (comment) this
uncritical comment that cannot be done easily, [see
answer](#5892 (comment))
- [x] Raise error if cannot get `organization_id` for objects
#5892 (comment)
- [x] Multiply fields for `iam_organization_field`
#5892 (comment)

Co-authored-by: Maxim Zhiltsov <zhiltsov.max35@gmail.com>
@azhavoro azhavoro mentioned this pull request Jun 2, 2023
nmanovic added a commit that referenced this pull request Jun 2, 2023
## \[2.4.5] - 2023-06-02
### Added
- Integrated support for sharepoint and cloud storage files, along with
directories to be omitted during task creation (server)
(<#6074>)
- Enabled task creation with directories from cloud storage or
sharepoint (<#6074>)
- Enhanced task creation to support any data type supported by the
server
by default, from cloud storage without the necessity for the `use_cache`
option (<#6074>)
- Added capability for task creation with data from cloud storage
without the `use_cache` option
(<#6074>)

### Changed
- User can now access resource links from any organization or sandbox,
granted it's available to them
(<#5892>)
- Cloud storage manifest files have been made optional
(<#6074>)
- Updated Django to the 4.2.x version
(<#6122>)
- Renamed certain Nuclio functions to adhere to a common naming
convention. For instance,
`onnx-yolov7` -> `onnx-wongkinyiu-yolov7`, `ultralytics-yolov5` ->
`pth-ultralytics-yolov5`
  (<#6140>)

### Deprecated
- Deprecated the endpoint `/cloudstorages/{id}/content`
(<#6074>)

### Fixed
- Fixed the issue of skeletons dumping on created tasks/projects
(<#6157>)
- Resolved an issue related to saving annotations for skeleton tracks
(<#6075>)

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Boris Sekachev <boris.sekachev@yandex.ru>
Co-authored-by: Roman Donchenko <roman@cvat.ai>
Co-authored-by: Maria Khrustaleva <maya17grd@gmail.com>
Co-authored-by: Boris Sekachev <sekachev.bs@gmail.com>
Co-authored-by: Nikita Manovich <nikita@cvat.ai>
Co-authored-by: Anastasia Yasakova <yasakova_anastasiya@mail.ru>
Co-authored-by: Snyk bot <snyk-bot@snyk.io>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kirill Sizov <kirill.sizov@cvat.ai>
Co-authored-by: Paweł Kotiuk <kotiuk@zohomail.eu>
Co-authored-by: SK <450723+senthilkumarkj@users.noreply.github.com>
Co-authored-by: Kirill Lakhov <kirill.9992@gmail.com>
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants