POST logic/Permissions/Test fixes #20

jwalz · 2024-03-07T15:29:26Z

WHAT:

Fix the logic for POSTing to AuthorizedStorageAccount
- Infer the appropriate user from the session_user_uri provided by OSF instead of accepting a relationship
- Accept the actual ID for the relationship to the external_storage_service
Fix the logic for POSTing to ConfiguredStorageAddon
- Expect the resource_reference relationship to POST the resource_uri, not the ID
Correct Permssions for AuthorizedStorageAccount creation
- Any user with an OSF account/session should be able to POST and AuthorizedStorageAccount
Correct Permissions for ConfiguredStorageAddon
- Make sure any user who can view the resource onthe OSF can GET the ConfiguredStorageAddon
- Optionally use the resource_uri field in the authoirzed_resource relationship to check OSF permissions
Fix tests
- Replace the @with_mocked_httpx_get decorator with a configurable mock for the OSF
- Fix failing test logic
- Expand certain tests to account for resource visibility on the OSF

aaxelb

wandered thru with some thoughts, looks good!

aaxelb · 2024-03-07T19:42:36Z

addon_service/common/permissions.py

+        resource_uri = None
+        try:
+            resource_uri = ResourceReference.objects.get(
+                id=request.data.get("authoirized_resource", {}).get("id")


Suggested change

id=request.data.get("authoirized_resource", {}).get("id")

id=request.data.get("authorized_resource", {}).get("id")

(nit: could also skip the query when there's no id)

aaxelb · 2024-03-07T20:07:11Z

addon_service/common/permissions.py



-class CanCreateCSA(permissions.BasePermission):
+class SessionUserIsReferencedResourceAdmin(permissions.BasePermission):


this permission class is still specific to ConfiguredStorageAddon (assumes the referenced resource is at authorized_resource) -- not necessarily a problem, but the rename made me think it would be more generic... could take a field_name param

aaxelb · 2024-03-07T20:15:34Z

addon_service/tests/test_by_type/test_authorized_storage_account.py

-            "Some form of auth is necessary or POSTS are ignored."
-        ]
+        self._mock_osf = MockOSF()
+        self.addCleanup(self._mock_osf.stop)


(lil nit/suggestion) if we're on python 3.11+ and MockOSF were usable as context manager (maybe start the patchers in __enter__ and stop them in __exit__), could do

self._mock_osf = MockOSF() self.enterContext(self._mock_osf)

...which isn't much different, but for some reason i'm still glad enterContext was added

Didn't know about enter_context! The only examples I'm seeing are in the context of an ExitStack, so the code might look like

with ExitStack as stack: self._mock_osf = MockOSF().enter_context()

Maybe worth a quick experiment to see if it works, since that would also make MockOSF easier to use in individual test functions.

oh hey that's confusing -- there's contextlib.ExitStack.enter_context and also unittest.TestCase.enterContext (where TestCase kinda has its own exit stack, but in camelCase) -- it's the TestCase one i think would help, and yeah agree using MockOSF as a context manager makes it nicely reusable

aaxelb · 2024-03-07T20:34:13Z

addon_service/configured_storage_addon/serializers.py

@@ -15,7 +15,7 @@
 class AuthorizedResourceField(ResourceRelatedField):
    def to_internal_value(self, data):
        resource_reference, _ = ResourceReference.objects.get_or_create(
-            resource_uri=data["id"]
+            resource_uri=data["resource_uri"]


am i reading correctly that the relationship in json is expected to be

"authorized_resource": { "data": { "resource_uri": "http://...", "type": "resource-references" } }

?

i get the desire to use the resource iri instead of the resource-reference id, but i wonder if there's a more jsonapi-friendly way to do it (rather than a resource identifier object without id/lid) -- maybe a authorized_resource_uri on-create-only attribute?

I'm fine with re-using the "write-only" field approach we agreed on for credentials here instead of the relationship (though, yeah, need to not let it be PATCH-able). That would clean up the Permissions logic above, too

https://openscience.atlassian.net/browse/ENG-5418

This should satisfy the comments on the Permissions class, too

aaxelb · 2024-03-07T20:45:35Z

addon_service/tests/test_by_type/test_authorized_storage_account.py

+            reverse("authorized-storage-accounts-list"), payload, format="vnd.api+json"
+        )
+        self.assertEqual(_resp.status_code, 201)
+        created_account_id = int(_resp.data['url'].rstrip('/').split('/')[-1])


might be half-dozen-of-another, but could avoid parsing a url by rendering the response to json

_created_account_id = _resp.json()['data']['id']

good call, this was just copy-pasted into this class instead of living in its own

aaxelb · 2024-03-07T20:53:00Z

app/authentication.py

@@ -33,6 +33,7 @@ class SkipAuthMethod(exceptions.APIException):


 def authenticate_resource(request, uri, required_permission):
+    print('WHAT?!!!!\n\n\n')


…t into fix_posts

aaxelb

🥪

Jon Walz and others added 3 commits March 6, 2024 16:50

Correct permissions, fix auth mocking, update some POST logic

6449992

Finish fixing existing tests

b9065c4

pre-commit pass

34c2f80

aaxelb reviewed Mar 7, 2024

View reviewed changes

Jon Walz added 6 commits March 8, 2024 10:41

Turn MockOSF into a Context Manager

163af45

Make precommit pass, remove the replaced decorators

c3f904d

Merge branch 'fix_posts' of github.com:CenterForOpenScience/gravyvale…

8e6b661

…t into fix_posts

Remove rogue print statement

0afc22a

Fix rogue merge errors

e67d33d

Remove lingering references to @with_mocked_httpx_get

c3244d2

aaxelb approved these changes Mar 8, 2024

View reviewed changes

aaxelb merged commit a00cf99 into develop Mar 8, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POST logic/Permissions/Test fixes #20

POST logic/Permissions/Test fixes #20

jwalz commented Mar 7, 2024

aaxelb left a comment

aaxelb Mar 7, 2024

aaxelb Mar 7, 2024

aaxelb Mar 7, 2024

jwalz Mar 7, 2024

aaxelb Mar 7, 2024

aaxelb Mar 7, 2024

jwalz Mar 7, 2024

jwalz Mar 8, 2024

aaxelb Mar 7, 2024

jwalz Mar 7, 2024

aaxelb Mar 7, 2024

aaxelb left a comment

	id=request.data.get("authoirized_resource", {}).get("id")
	id=request.data.get("authorized_resource", {}).get("id")



		class CanCreateCSA(permissions.BasePermission):
		class SessionUserIsReferencedResourceAdmin(permissions.BasePermission):

		@@ -33,6 +33,7 @@ class SkipAuthMethod(exceptions.APIException):


		def authenticate_resource(request, uri, required_permission):
		print('WHAT?!!!!\n\n\n')

POST logic/Permissions/Test fixes #20

POST logic/Permissions/Test fixes #20

Conversation

jwalz commented Mar 7, 2024

aaxelb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaxelb left a comment

Choose a reason for hiding this comment