Api save whitelist by will-moore · Pull Request #5059 · ome/openmicroscopy

will-moore · 2017-01-26T09:56:47Z

What this PR does

Restricts Save (PUT or POST) to a "whitelist" of supported object types.

In the SaveView class that handles PUT and POST, we use a whitelist of supported types Project, Dataset, Screen.
In the ObjectView subclasses that handle DELETE of various objects we use a CAN_DELETE attribute that is True is the base class and False in subclasses where Delete is NOT supported Plate, Image, Well.

Testing this PR

Existing tests confirm that we can create, update & delete Project, Dataset & Screen. (Plate removed from this test).
Added tests that checks Image, Plate & other types cannot be created, updated or deleted.

Related reading

Discussed at web meeting on 2017-01-24

chris-allan

We should add some test cases to ensure that the PUT and POST restrictions are accurate.

chris-allan · 2017-01-26T10:16:42Z

components/tools/OmeroWeb/omeroweb/api/views.py

    POST to create a new Object and PUT to replace existing one.
    """

+    can_put = ['Project', 'Dataset', 'Screen']


As these are constants, let's uppercase them.

snoopycrimecop · 2017-01-30T03:34:05Z

Conflicting PR. Removed from build OMERO-DEV-merge-push#577. See the console output for more details.
Possible conflicts:

PR Api wells #5047 will-moore 'Api wells'
- components/tools/OmeroWeb/test/integration/test_api_images.py
PR Api child count #5058 will-moore 'Api child count'
- components/tools/OmeroWeb/test/integration/test_api_images.py

snoopycrimecop · 2017-02-01T03:33:50Z

Conflicting PR. Removed from build OMERO-DEV-merge-push#579. See the console output for more details.
Possible conflicts:

PR Api wells #5047 will-moore 'Api wells'
- components/tools/OmeroWeb/test/integration/test_api_images.py
PR Api child count #5058 will-moore 'Api child count'
- components/tools/OmeroWeb/test/integration/test_api_images.py

snoopycrimecop · 2017-02-02T03:33:48Z

Conflicting PR. Removed from build OMERO-DEV-merge-push#580. See the console output for more details.
Possible conflicts:

PR Api wells #5047 will-moore 'Api wells'
- components/tools/OmeroWeb/test/integration/test_api_images.py
PR Api child count #5058 will-moore 'Api child count'
- components/tools/OmeroWeb/test/integration/test_api_images.py

snoopycrimecop · 2017-02-03T03:34:14Z

Conflicting PR. Removed from build OMERO-DEV-merge-push#581. See the console output for more details.
Possible conflicts:

PR Api wells #5047 will-moore 'Api wells'
- components/tools/OmeroWeb/test/integration/test_api_images.py
PR Api child count #5058 will-moore 'Api child count'
- components/tools/OmeroWeb/test/integration/test_api_images.py

Since this test is really the counterpart to test_container_crud() above

jburel · 2017-02-15T13:31:21Z

components/tools/OmeroWeb/omeroweb/api/decorators.py

 from django.http import JsonResponse
 from functools import update_wrapper
-from api_exceptions import NotFoundError, BadRequestError, CreatedObject
+from api_exceptions import NotFoundError, BadRequestError, CreatedObject, \


one per line and alphabetical order

jburel · 2017-02-15T13:31:45Z

components/tools/OmeroWeb/omeroweb/api/decorators.py

@@ -75,7 +76,7 @@ def handle_error(self, ex, trace):
        # But we try to handle all 'expected' errors appropriately
        # TODO: handle omero.ConcurrencyException


Will you do that in another PR? i.e. TODO

Yes. Hard to see that we need to support ConcurrencyException since we are doing very simple get/set queries now and no stateful services.

jburel · 2017-02-15T13:32:01Z

components/tools/OmeroWeb/omeroweb/api/views.py

 from omeroweb.connector import Server
 from omeroweb.api.api_exceptions import BadRequestError, NotFoundError, \
-    CreatedObject
+    CreatedObject, MethodNotSupportedError


jburel · 2017-02-15T13:46:41Z

components/tools/OmeroWeb/test/integration/test_api_containers.py

+    @pytest.mark.parametrize("dtype", ['Plate', 'Image', 'Well',
+                                       'Channel', 'foo'])
+    def test_crud_unsupported(self, user1, dtype):
+        """Test create, update & delete are rejected for unsupported types."""


I think you should split the test and not have post/put/delete tests in the same one

@jburel I split the test in 102c6c9

will-moore · 2017-02-15T23:58:38Z

Need to fix failing ```test_api_errors.py```` tests in https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-breaking-integration-Python27/164/testReport/
Haven't had time today, so will close this until I fix tests (tomorrow).

Single test for PUT, POST & DELETE is spilt into separate tests for each method. To try and reduce code duplication, PUT and POST are covered by parametrizing method in a single test.

will-moore · 2017-02-17T00:10:14Z

@jburel After those last commits, this is going to conflict with #5058, so we'll need to get that merged ASAP. I still have 3 further api PRs waiting after this one.
@chris-allan You still have "requested changes" on this PR, which I think are addressed with the added tests?

snoopycrimecop · 2017-02-17T03:32:36Z

Conflicting PR. Removed from build OMERO-DEV-merge-push#592. See the console output for more details.
Possible conflicts:

PR Api child count #5058 will-moore 'Api child count'
- components/tools/OmeroWeb/test/integration/test_api_errors.py

chris-allan

Looks great aside from the exception feedback above.

chris-allan · 2017-02-17T09:44:25Z

components/tools/OmeroWeb/omeroweb/api/api_exceptions.py

+
+    status = 405
+
+    def __init__(self, message, stacktrace=None):


I'm not seeing any use of the stacktrace keyword argument in the implementation. Is it your intention to use it for something? If so, what? If you want to have a unified constructor prototype that is similar to the other exceptions you have above then I would suggest making a class hierarchy so you are not having to implement, and document, this every time.

will-moore · 2017-02-17T11:11:03Z

@chris-allan Thanks. Fixed in eb18edc

jburel · 2017-02-17T19:08:46Z

Exception code fixed.
Tests split and passing
I will merge on Monday am if everything is green

will-moore added 2 commits January 26, 2017 10:02

Basic whitelist for allowing 'save' objects

b92045a

SaveView restricts PUT to can_put whitelist

39618ef

will-moore force-pushed the api_save_whitelist branch from 8bdd52f to 39618ef Compare January 26, 2017 10:02

chris-allan requested changes Jan 26, 2017

View reviewed changes

Uppercase CAN_PUT, CAN_POST. Both allow P/D/S

c1b626a

jburel added the develop label Jan 26, 2017

will-moore added 4 commits January 27, 2017 13:16

Add CAN_DELETE check into ObjectView.delete()

c52550b

Return status=405 'method not supported' for disallowed methods

756ae7d

Add test to check POST, PUT & DELETE not supported for Images

afcf581

flake8 fixes

a1a769c

will-moore closed this Jan 29, 2017

will-moore reopened this Jan 29, 2017

Remove 'Plate' from test_container_crud()

0303161

will-moore closed this Jan 31, 2017

will-moore reopened this Jan 31, 2017

will-moore closed this Jan 31, 2017

will-moore reopened this Jan 31, 2017

snoopycrimecop mentioned this pull request Feb 2, 2017

Api wells2 #5069

Merged

will-moore closed this Feb 3, 2017

will-moore reopened this Feb 14, 2017

will-moore force-pushed the api_save_whitelist branch from b22828f to 0303161 Compare February 14, 2017 16:01

will-moore added 5 commits February 14, 2017 16:01

Tiny fix to test_api_images to avoid merge conflict

d1baadc

Merge remote-tracking branch 'origin/develop' into api_save_whitelist

34aeb5b

Test handling of create, update & delete for unsupported types

fd204cc

Move test_crud_unsupported to test_api_containers.py

da75513

Since this test is really the counterpart to test_container_crud() above

Remove comment

17682f3

flake8 fix

b7e936e

will-moore closed this Feb 14, 2017

will-moore reopened this Feb 14, 2017

jburel reviewed Feb 15, 2017

View reviewed changes

will-moore closed this Feb 15, 2017

Fix failing test_api_errors.py tests

ddd187a

will-moore reopened this Feb 16, 2017

will-moore added 4 commits February 16, 2017 16:50

Tidy api_exceptions imports

4b72827

Split test_crud_unsupported into separate tests

102c6c9

Single test for PUT, POST & DELETE is spilt into separate tests for each method. To try and reduce code duplication, PUT and POST are covered by parametrizing method in a single test.

flake8 fix

a377f3d

flake8 fix

242c551

chris-allan requested changes Feb 17, 2017

View reviewed changes

ApiException base class to avoid code duplication

eb18edc

Merge remote-tracking branch 'origin/develop' into api_save_whitelist

4c73782

chris-allan approved these changes Feb 17, 2017

View reviewed changes

jburel merged commit a74790c into ome:develop Feb 20, 2017

jburel added this to the 5.3.0 milestone Mar 29, 2017

will-moore deleted the api_save_whitelist branch February 18, 2019 04:15

		@@ -75,7 +76,7 @@ def handle_error(self, ex, trace):
		# But we try to handle all 'expected' errors appropriately
		# TODO: handle omero.ConcurrencyException

Conversation

will-moore commented Jan 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

Testing this PR

Related reading

Uh oh!

chris-allan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snoopycrimecop commented Jan 30, 2017

Uh oh!

snoopycrimecop commented Feb 1, 2017

Uh oh!

snoopycrimecop commented Feb 2, 2017

Uh oh!

snoopycrimecop commented Feb 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

will-moore commented Feb 15, 2017

Uh oh!

will-moore commented Feb 17, 2017

Uh oh!

snoopycrimecop commented Feb 17, 2017

Uh oh!

chris-allan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

will-moore commented Feb 17, 2017

Uh oh!

jburel commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

will-moore commented Jan 26, 2017 •

edited

Loading

jburel commented Feb 17, 2017 •

edited

Loading