Create REST endpoint for POSTing job models #2031

kotfic · 2017-05-25T19:33:13Z

Does not allow posting to /job unless jobs.rest.create_job is set as a scope
on the current token - regardless of whether TokenScope.USER_AUTH is set.

kotfic · 2017-05-25T19:33:37Z

@zachmullen can you comment on whether or not this is the right/best approach? thanks!

kotfic · 2017-05-25T19:34:10Z

See: #2018

kotfic · 2017-05-25T19:38:12Z

btw, I found this to be a little bit confusing - i expected when setting @access.token(scope='FOOBAR') that the endpoint would throw a 403 if the token didn't have scope 'FOOBAR' set on it.

However, because ensureTokenScopes returns immediately if the user is logged in, it effectively means a logged in user has ALL custom scopes defined. I didn't see a built-in way to handle a situation where i wanted to protect an endpoint with a custom scope regardless of user authentication status.

zachmullen · 2017-06-01T14:24:11Z

plugins/jobs/server/job_rest.py

+    @filtermodel(model='job', plugin='jobs')
+    @access.token(scope=constants.REST_CREATE_JOB_TOKEN_SCOPE)
+    @autoDescribeRoute(
+        Description('Create a job model via a RESTful endpoint')


No need to put "via a RESTful endpoint" :)

zachmullen · 2017-06-01T14:29:39Z

plugins/jobs/server/__init__.py


 def load(info):
    info['apiRoot'].job = job_rest.Job()
+    events.bind('rest.post.job.before', 'jobs', _authorizeRestJobCreation)


Since this is all in the same plugin, let's just move this logic into the REST route handler itself. That will improve readability via lexical proximity of the logic, and won't preclude behavioral modification by downstreams.

zachmullen · 2017-06-01T14:30:54Z

plugins/jobs/server/__init__.py

+    if not tokenModel.hasScope(token, constants.REST_CREATE_JOB_TOKEN_SCOPE):
+        raise AccessException(
+            'Invalid token scope.\n'
+            'Required: %s.\n' % (constants.REST_CREATE_JOB_TOKEN_SCOPE))


This pattern appears in a few places, it's probably worth adding a standardized requireScope method into the token model that raises an exception like this.

Would this also help with the recent issue we found where invalidly scoped tokens were throwing 401s and saying "not logged in" when really it was the case that the token didn't have the correct scope? If we can improve that error handling as a part of this PR that would be rad.

It wouldn't help with that particular case. I'm not actually sure how to engineer a solution for that at this time.

zachmullen · 2017-06-01T14:34:02Z

plugins/jobs/server/models/job.py

-from girder.plugins.jobs.constants import JobStatus, JOB_HANDLER_LOCAL
+from girder.plugins.jobs.constants import (JobStatus,
+                                           JOB_HANDLER_LOCAL,
+                                           REST_CREATE_JOB_TOKEN_SCOPE)


This change doesn't seem necessary.

zachmullen · 2017-06-01T14:37:04Z

btw, I found this to be a little bit confusing - i expected when setting @access.token(scope='FOOBAR') that the endpoint would throw a 403 if the token didn't have scope 'FOOBAR' set on it.

Right, that's the behavior we use to support OAuth-provider-like authorization control. One way to support these sorts of use cases is to add another flag to the access decorators, e.g.

@access.token(scope=FOO, required=True)

The required flag (for lack of a more expressive name) would default to False, but if True would actually require the listed scopes. This PR would be a good place to add that, and also the token model requireScope method that would underlie it.

Does not allow posting unless jobs.rest.create_job is set as a scope on the token. This is set automatically when using createJobToken()

dorukozturk · 2017-06-15T19:46:28Z

@kotfic, @zachmullen This is ready for a review.

zachmullen · 2017-06-19T16:52:12Z

girder/models/token.py

+
+        if not self.hasScope(token, scope):
+            raise AccessException('Invalid token scope.'
+                                  'Required: %s.' % (scope))


I think we can fit this all on one line, but if not, we'll need to add a space between the sentences.

zachmullen · 2017-06-19T16:53:06Z

plugins/jobs/server/job_rest.py

+        Description('Create a job model')
+        .param('title', '', required=True)
+        .param('type', '', required=True)
+        .modelParam('parentId', 'Id of the parent job.', model='job',


In user-facing text, we should use "ID" rather than "Id".

@zachmullen I applied those changes.

kotfic mentioned this pull request May 31, 2017

Parent Child Jobs #2046

Merged

kotfic added the feature label Jun 1, 2017

kotfic requested a review from zachmullen June 1, 2017 14:09

kotfic added the security label Jun 1, 2017

zachmullen added domain: server security and removed security labels Jun 1, 2017

zachmullen requested changes Jun 1, 2017

View reviewed changes

dorukozturk force-pushed the create-worker-job-model branch from 68143ea to 8cf5ff2 Compare June 14, 2017 23:23

Create REST endpoint for POSTing job models

809fb33

Does not allow posting unless jobs.rest.create_job is set as a scope on the token. This is set automatically when using createJobToken()

dorukozturk force-pushed the create-worker-job-model branch 2 times, most recently from db04b3c to 0287f0a Compare June 15, 2017 15:35

dorukozturk changed the title ~~[WIP] Create REST endpoint for POSTing job models~~ Create REST endpoint for POSTing job models Jun 15, 2017

msmolens mentioned this pull request Jun 19, 2017

item_tasks: Unify importing tasks from JSON and Slicer CLI #2098

Merged

zachmullen requested changes Jun 19, 2017

View reviewed changes

Doruk Ozturk added 5 commits June 19, 2017 13:09

Move requireScope logic to the token model

ac429dc

Add parentJob model parameter to the createJob endpoint

8d4334a

Test token scope methods

bf9bff4

Test required keyword argument within the access.token context

219c63f

Test the endpoint which creates jobs

0ccf574

dorukozturk force-pushed the create-worker-job-model branch from 76574a0 to 0ccf574 Compare June 19, 2017 17:10

zachmullen approved these changes Jun 19, 2017

View reviewed changes

dorukozturk merged commit db04886 into master Jun 19, 2017

dorukozturk deleted the create-worker-job-model branch June 19, 2017 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create REST endpoint for POSTing job models #2031

Create REST endpoint for POSTing job models #2031

kotfic commented May 25, 2017

kotfic commented May 25, 2017

kotfic commented May 25, 2017

kotfic commented May 25, 2017

zachmullen Jun 1, 2017

zachmullen Jun 1, 2017

zachmullen Jun 1, 2017

mgrauer Jun 1, 2017 •

edited

Loading

zachmullen Jun 1, 2017

zachmullen Jun 1, 2017

zachmullen commented Jun 1, 2017

dorukozturk commented Jun 15, 2017

zachmullen Jun 19, 2017

zachmullen Jun 19, 2017

dorukozturk Jun 19, 2017

Create REST endpoint for POSTing job models #2031

Create REST endpoint for POSTing job models #2031

Conversation

kotfic commented May 25, 2017

kotfic commented May 25, 2017

kotfic commented May 25, 2017

kotfic commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgrauer Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachmullen commented Jun 1, 2017

dorukozturk commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgrauer Jun 1, 2017 •

edited

Loading