update wb v1 auth payload to include intention

felliott · felliott · commit 181be3cce3db · 2020-10-21T17:05:04.000-04:00
* When requesting auth from the OSF, WB includes metadata about the
   requested action.  This metadata had been overloaded to mean both
   "what is the user trying to do" and "what permissions does the user
   need to do it".  In the past it was easy to infer the latter from
   the former, but with the addition of osfstorage quotas, that is no
   longer the case.  The overloading also made it very confusing to
   reason about both the OSF and WB sides of this request.  Factor out
   this code into a new method, `_determine_actions`, and update the
   auth payload to include the intent separate from the permissions.

 * Update v1's movecopy to pass `path` to the auth object's `fetch`
   method.  Everyone else does it, why shouldn't they?

 * Update tests for new permissions &amp; intent fields.

 [ENG-2249]
diff --git a/tests/auth/osf/test_handler.py b/tests/auth/osf/test_handler.py
@@ -81,6 +81,7 @@ async def test_permissions_post_copy_source_destination(self):
                     'nid': 'test',
                     'provider': 'test',
                     'action': 'download',
+                    'intent': 'copyfrom',
                     'path': '',
                     'version': None,
                     'metrics': {
@@ -95,6 +96,7 @@ async def test_permissions_post_copy_source_destination(self):
                     'nid': 'test',
                     'provider': 'test',
                     'action': 'upload',
+                    'intent': 'copyto',
                     'path': '',
                     'version': None,
                     'metrics': {
@@ -109,24 +111,47 @@ async def test_permissions_post_copy_source_destination(self):
 class TestActionMapping:
 
     @pytest.mark.asyncio
-    @pytest.mark.parametrize('method, action, auth_type, headers, query_args, expected', [
-        ['post', 'copy',   AuthType.SOURCE,      None, None, 'download'],
-        ['post', 'copy',   AuthType.DESTINATION, None, None, 'upload'],
-        ['post', 'move',   AuthType.SOURCE,      None, None, 'upload'],  # TODO: really?
-        ['post', 'move',   AuthType.DESTINATION, None, None, 'upload'],
-        ['post', 'rename', AuthType.SOURCE,      None, None, 'upload'],  # TODO: really?
-        ['post', 'rename', AuthType.DESTINATION, None, None, 'upload'],
-
-        ['head', None, None, {settings.MFR_ACTION_HEADER: 'render'}, None, 'render'],
-        ['head', None, None, {settings.MFR_ACTION_HEADER: 'export'}, None, 'export'],
-
-        ['get',    None, None, None, None,             'download'],
-        ['put',    None, None, None, None,             'upload'],
-        ['delete', None, None, None, None,             'delete'],
-        ['get',    None, None, None, {'meta': 1},      'metadata'],
-        ['get',    None, None, None, {'revisions': 1}, 'revisions'],
+    @pytest.mark.parametrize('method, action, path, auth_type, headers, query_args, exp_perm_action, exp_intent', [
+        ['post', 'copy',   '/folder/', AuthType.SOURCE,      None, None, 'download', 'copyfrom'],
+        ['post', 'copy',   '/folder/', AuthType.DESTINATION, None, None, 'upload',   'copyto'],
+        ['post', 'move',   '/folder/', AuthType.SOURCE,      None, None, 'delete',   'movefrom'],
+        ['post', 'move',   '/folder/', AuthType.DESTINATION, None, None, 'upload',   'moveto'],
+        ['post', 'rename', '/folder/', AuthType.SOURCE,      None, None, 'upload',   'rename'],
+        ['post', 'rename', '/folder/', AuthType.DESTINATION, None, None, 'upload',   'rename'],
+
+        ['post', 'copy',   '/file', AuthType.SOURCE,      None, None, 'download', 'copyfrom'],
+        ['post', 'copy',   '/file', AuthType.DESTINATION, None, None, 'upload',   'copyto'],
+        ['post', 'move',   '/file', AuthType.SOURCE,      None, None, 'delete',   'movefrom'],
+        ['post', 'move',   '/file', AuthType.DESTINATION, None, None, 'upload',   'moveto'],
+        ['post', 'rename', '/file', AuthType.SOURCE,      None, None, 'upload',   'rename'],
+        ['post', 'rename', '/file', AuthType.DESTINATION, None, None, 'upload',   'rename'],
+
+        ['head', None, '/folder/', None, {settings.MFR_ACTION_HEADER: 'render'}, None, 'render', 'render'],
+        ['head', None, '/folder/', None, {settings.MFR_ACTION_HEADER: 'export'}, None, 'export', 'export'],
+
+        ['head', None, '/file', None, {settings.MFR_ACTION_HEADER: 'render'}, None, 'render', 'render'],
+        ['head', None, '/file', None, {settings.MFR_ACTION_HEADER: 'export'}, None, 'export', 'export'],
+
+        ['get',    None, '/folder/', None, None, None,               'metadata',  'metadata'],
+        ['head',   None, '/folder/', None, None, None,               'metadata',  'metadata'],
+        ['put',    None, '/folder/', None, None, {'kind': 'folder'}, 'upload',    'create_dir'],
+        ['put',    None, '/folder/', None, None, {'kind': 'file'},   'upload',    'create_file'],
+        ['delete', None, '/folder/', None, None, None,               'delete',    'delete'],
+        ['get',    None, '/folder/', None, None, {'meta': 1},        'metadata',  'metadata'],
+        ['get',    None, '/folder/', None, None, {'revisions': 1},   'metadata',  'metadata'],
+        ['get',    None, '/folder/', None, None, {'zip': 1},         'download',  'daz'],
+
+        ['get',    None, '/file', None, None, None,                        'download',  'download'],
+        ['head',   None, '/file', None, None, None,                        'metadata',  'metadata'],
+        ['put',    None, '/file', None, None, None,                        'upload',    'update_file'],
+        ['delete', None, '/file', None, None, None,                        'delete',    'delete'],
+        ['get',    None, '/file', None, None, {'meta': 1},                 'metadata',  'metadata'],
+        ['get',    None, '/file', None, None, {'revisions': 1},            'revisions', 'revisions'],
+        ['get',    None, '/file', None, None, {'zip': 1},                  'download',  'download'],
+        ['get',    None, '/file', None, None, {'meta': 1, 'revisions': 1}, 'metadata',  'metadata'],
     ])
-    async def test_action_type(self, method, action, auth_type, headers, query_args, expected):
+    async def test_action_type(self, method, action, path, auth_type, headers, query_args,
+                               exp_perm_action, exp_intent):
 
         handler = OsfAuthHandler()
 
@@ -136,7 +161,7 @@ async def test_action_type(self, method, action, auth_type, headers, query_args,
         request.query_arguments = query_args if query_args is not None else {}
         request.cookies = {}
 
-        kwargs = {}
+        kwargs = {'path': path}
         if action is not None:
             kwargs['action'] = action
         if auth_type is not None:
@@ -151,7 +176,8 @@ async def test_action_type(self, method, action, auth_type, headers, query_args,
 
         handler.build_payload.asssert_called_once()
         args, _ = handler.build_payload.call_args
-        assert args[0]['action'] == expected
+        assert args[0]['action'] == exp_perm_action
+        assert args[0]['intent'] == exp_intent
 
     @pytest.mark.asyncio
     async def test_unhandled_mfr_action(self):
diff --git a/waterbutler/auth/osf/handler.py b/waterbutler/auth/osf/handler.py
@@ -102,37 +102,14 @@ async def fetch(self, request, bundle):
 
     async def get(self, resource, provider, request, action=None, auth_type=AuthType.SOURCE,
                   path='', version=None):
-        """Used for v1"""
-        method = request.method.lower()
+        """Used for v1.  Requests credentials and configuration from the OSF for the given resource
+        (project) and provider.  Auth credentials sent by the user to WB are passed onto the OSF so
+        it can determine the user.  Auth payload also includes some metrics and metadata to help
+        the OSF with resource tallies and permission optimizations.
+        """
 
-        if method == 'post' and action:
-            post_action_map = {
-                'copy': 'download' if auth_type is AuthType.SOURCE else 'upload',
-                'rename': 'upload',
-                'move': 'upload',
-            }
-
-            try:
-                osf_action = post_action_map[action.lower()]
-            except KeyError:
-                raise exceptions.UnsupportedActionError(method, supported=post_action_map.keys())
-        elif method == 'head' and settings.MFR_ACTION_HEADER in request.headers:
-            mfr_action_map = {'render': 'render', 'export': 'export'}
-            mfr_action = request.headers[settings.MFR_ACTION_HEADER].lower()
-            try:
-                osf_action = mfr_action_map[mfr_action]
-            except KeyError:
-                raise exceptions.UnsupportedActionError(mfr_action, supported=mfr_action_map.keys())
-        else:
-            try:
-                if method == 'get' and 'meta' in request.query_arguments:
-                    osf_action = 'metadata'
-                elif method == 'get' and 'revisions' in request.query_arguments:
-                    osf_action = 'revisions'
-                else:
-                    osf_action = self.ACTION_MAP[method]
-            except KeyError:
-                raise exceptions.UnsupportedHTTPMethodError(method, supported=self.ACTION_MAP.keys())
+        (permissions_req, intent) = self._determine_actions(resource, provider, request, action,
+                                                            auth_type, path, version)
 
         headers = {'Content-Type': 'application/json'}
 
@@ -155,7 +132,8 @@ async def get(self, resource, provider, request, action=None, auth_type=AuthType
             self.build_payload({
                 'nid': resource,
                 'provider': provider,
-                'action': osf_action,
+                'action': permissions_req,  # what permissions does the user need?
+                'intent': intent,           # what is the user trying to do?
                 'path': path,
                 'version': version,
                 'metrics': {
@@ -171,3 +149,94 @@ async def get(self, resource, provider, request, action=None, auth_type=AuthType
 
         payload['auth']['callback_url'] = payload['callback_url']
         return payload
+
+    def _determine_actions(self, resource, provider, request, action=None,
+                           auth_type=AuthType.SOURCE, path='', version=None):
+        """Decide what the user is trying to achieve and what permissions they need to achieve it.
+        Returns two values, ``osf_action`` and ``intended_action``.  ``intended_action`` is a tag
+        that describes what the user is trying to accomplish.  This tag can have many values,
+        currently including:
+
+        ``copyfrom``, ``copyto``, ``create_dir``, ``create_file``, ``daz``, ``delete``,
+        ``download``, ``export``, ``metadata``, ``movefrom``, ``moveto``, ``rename``, ``render``,
+        ``revisions``, ``update_file``
+
+        ``osf_action`` describes the permissions needed, but ofuscated with some legacy cruft.  In
+        theory, it only needs to be one of two values, ``readonly`` or `readwrite``.  However, it
+        used to be overloaded to convey intent in addition to permissions.  For back-compat reasons,
+        we are not changing the current returned value.  Its possible values are a subset of the
+        ``intended_actions`` from which the OSF can infer whether ``ro`` or ``rw`` permissions are
+        needed.  The current mapping is:
+
+        Implies ``readonly``: ``metadata``, ``download``, ``render``, ``export``.
+
+        Implies ``readwrite``: ``upload``, ``delete``.
+        """
+
+        method = request.method.lower()
+        osf_action, intended_action = None, None
+
+        if method == 'post' and action:
+            post_action_map = {
+                'copy': 'download' if auth_type is AuthType.SOURCE else 'upload',
+                'move': 'delete' if auth_type is AuthType.SOURCE else 'upload',
+                'rename': 'upload',
+            }
+
+            intended_post_action_map = {
+                'copy': 'copyfrom' if auth_type is AuthType.SOURCE else 'copyto',
+                'move': 'movefrom' if auth_type is AuthType.SOURCE else 'moveto',
+                'rename': 'rename',
+            }
+
+            try:
+                osf_action = post_action_map[action.lower()]
+                intended_action = intended_post_action_map[action.lower()]
+            except KeyError:
+                raise exceptions.UnsupportedActionError(method, supported=post_action_map.keys())
+        elif method == 'put':
+            osf_action = 'upload'
+            if not path.endswith('/'):
+                intended_action = 'update_file'
+            elif request.query_arguments.get('kind', '') == 'folder':
+                intended_action = 'create_dir'
+            else:
+                intended_action = 'create_file'
+        elif method == 'head' and settings.MFR_ACTION_HEADER in request.headers:
+            mfr_action_map = {'render': 'render', 'export': 'export'}
+            mfr_action = request.headers[settings.MFR_ACTION_HEADER].lower()
+            try:
+                osf_action = mfr_action_map[mfr_action]
+                intended_action = mfr_action
+            except KeyError:
+                raise exceptions.UnsupportedActionError(mfr_action, supported=mfr_action_map.keys())
+        else:
+            try:
+                if method == 'get':
+                    if path.endswith('/'):  # path isa folder
+                        # folders ignore 'revisions' query param
+                        if 'zip' in request.query_arguments:
+                            osf_action = 'download'
+                            intended_action = 'daz'
+                        else:
+                            osf_action = 'metadata'
+                            intended_action = 'metadata'
+                    else:  # path isa file, not folder
+                        # files ignore 'zip' query param
+                        # precedence order is 'meta', 'zip', default
+                        if 'meta' in request.query_arguments:
+                            osf_action = 'metadata'
+                            intended_action = 'metadata'
+                        elif 'revisions' in request.query_arguments:
+                            osf_action = 'revisions'
+                            intended_action = 'revisions'
+                        else:
+                            osf_action = 'download'
+                            intended_action = 'download'
+                else:
+                    osf_action = self.ACTION_MAP[method]
+                    intended_action = self.ACTION_MAP[method]
+            except KeyError:
+                raise exceptions.UnsupportedHTTPMethodError(method, supported=self.ACTION_MAP.keys())
+
+        return (osf_action, intended_action)
diff --git a/waterbutler/server/api/v1/provider/movecopy.py b/waterbutler/server/api/v1/provider/movecopy.py
@@ -123,6 +123,7 @@ async def move_or_copy(self):
                 self.json.get('provider', self.provider.NAME),
                 self.request,
                 action=auth_action,
+                path=path,
                 auth_type=AuthType.DESTINATION,
             )
             self.dest_provider = make_provider(

Original file line number	Diff line number	Diff line change
`@@ -123,6 +123,7 @@ async def move_or_copy(self):`
`123`	`123`	`self.json.get('provider', self.provider.NAME),`
`124`	`124`	`self.request,`
`125`	`125`	`action=auth_action,`
	`126`	`+ path=path,`
`126`	`127`	`auth_type=AuthType.DESTINATION,`
`127`	`128`	`)`
`128`	`129`	`self.dest_provider = make_provider(`