Arvados

Comments on 2fc6dde 1904-object-scopes-wip

1. current_api_client_auth_has_scope is a little hard to follow. Suggestions:
   • Does ok_scopes need to be an array? As I understand it, this is the path the scopes are being tested against.
   • Rename 'ok_scopes' to a more descriptive name such as 'request_path'
   • Rename block parameter 'scope' to a more descriptive name such as 'token_scope'
   • If ok_scopes/request_path is not an array, the logic inside block is a little bit simpler.
2. What's the effect of "https!" in #request_with_auth? Add comment
3. test "user list token can only list users" -> What happens with a request path of '/arvados/v1/users/'?
4. test "multiple scopes" -> This test seems to be testing more than just the "multiple scopes" title would suggest. Might be better to break it out into a test more narrowly focused on testing multiple scopes, and a second test related to manipulating scopes.
5. In the API server, having access to the index basically means having access to all the objects in that table that are otherwise readable. This seems to make a mockery of the /arvados/v1/foo vs. /arvados/v1/foo/ distinction we were trying to draw the other day.

Peter Amstutz wrote:

Comments on 2fc6dde 1904-object-scopes-wip

1. current_api_client_auth_has_scope is a little hard to follow.

Refactored mostly as you suggested. Also cleaned up the now-obsolete distinction between require_auth_scope and require_auth_scope_all.

1. What's the effect of "https!" in #request_with_auth? Add comment

HTTPS requests were needed when these were controller tests (not 100% sure why) but seem to be unnecessary for integration tests. Removed.

1. test "user list token can only list users" -> What happens with a request path of '/arvados/v1/users/'?

Added a test for this. It passes.

1. test "multiple scopes" -> This test seems to be testing more than just the "multiple scopes" title would suggest. Might be better to break it out into a test more narrowly focused on testing multiple scopes, and a second test related to manipulating scopes.

I think I see what you're getting at. My feeling is that the extra code is just checking that the POST request actually did go through—not that we care about the particulars of how the new token was created, etc.—and that any test of a POST scope would probably look similar. I made some changes to try to help clarify the goals here. Do they alleviate your concerns?

1. In the API server, having access to the index basically means having access to all the objects in that table that are otherwise readable. This seems to make a mockery of the /arvados/v1/foo vs. /arvados/v1/foo/ distinction we were trying to draw the other day.

Good point. I think this needs further discussion in the team.

Comments on 1904-workbench-temp-token-wip @ 4425dbc002ec66aa18a6769d9c1aba46c8d30586

This behavior seems slightly surprising: when I'm using a "ticket", I don't get to use the privileges or context cues that I would normally have by virtue of being logged in. E.g., the "Logged in as foo@example.com" item in the "account stuff" drop-down will not show my email address, and any clever "should I offer to do this?" code in Workbench would conclude I'm unable to add a bookmark/tag to this shared item.

Is this really what we want, or would it be more desirable to restrict the use of the per-request "ticket" to the "get the requested collection" transaction, for example, and use the usual session token for all the other auxiliary things that happen in Workbench? (Setting aside for a moment the question of how this would be implemented...)

For the purpose of this story, it isn't necessary to support the "anonymous" case, i.e., it's OK if you need to go through the login process and accept a user agreement before seeing the shared item. (We can wait for the more general "anonymous browsing" story to make the login procedure optional.)

Brett Smith wrote:

1. In the API server, having access to the index basically means having access to all the objects in that table that are otherwise readable. This seems to make a mockery of the /arvados/v1/foo vs. /arvados/v1/foo/ distinction we were trying to draw the other day.

Good point. I think this needs further discussion in the team.

This was discussed and the general feeling seems to be that we should go ahead with merging this version. So Peter, 1904-object-scopes-wip is back to you for another round of review.

Tom Clegg wrote:

Comments on 1904-workbench-temp-token-wip @ 4425dbc002ec66aa18a6769d9c1aba46c8d30586

This behavior seems slightly surprising: when I'm using a "ticket", I don't get to use the privileges or context cues that I would normally have by virtue of being logged in. E.g., the "Logged in as foo@example.com" item in the "account stuff" drop-down will not show my email address, and any clever "should I offer to do this?" code in Workbench would conclude I'm unable to add a bookmark/tag to this shared item.

Is this really what we want, or would it be more desirable to restrict the use of the per-request "ticket" to the "get the requested collection" transaction, for example, and use the usual session token for all the other auxiliary things that happen in Workbench? (Setting aside for a moment the question of how this would be implemented...)

I think that's fair, and I don't think it would be difficult to implement. I can go ahead and work toward this.

For the purpose of this story, it isn't necessary to support the "anonymous" case, i.e., it's OK if you need to go through the login process and accept a user agreement before seeing the shared item. (We can wait for the more general "anonymous browsing" story to make the login procedure optional.)

Wouldn't that break the "give someone a Collection link that they can feed to wget -r" case?

Brett Smith wrote:

For the purpose of this story, it isn't necessary to support the "anonymous" case, i.e., it's OK if you need to go through the login process and accept a user agreement before seeing the shared item. (We can wait for the more general "anonymous browsing" story to make the login procedure optional.)

Wouldn't that break the "give someone a Collection link that they can feed to wget -r" case?

Good point.

Perhaps "use api_ticket for everything if no api_token provided" at least for requests like "download a file" that don't do a bunch of other context-sensitive API transactions anyway.

It occurs to me that "wget -r .../collections/uuid" (full blown workbench page) would not be a great way to download. I suppose it would be nice to provide a very plain directory listing at /collections/uuid/. (And that would be another action in the special class: "if api_ticket is given, don't require api_token because the only API transactions we're going to do are the ones that use the ticket anyway".)

Discussed temporary tokens in IRC with Tom some more. We want to explore the possibility of modifying the API server to let clients send along zero or more tokens. Then Workbench can accept those in a parameter and pass them along. Currently investigating what that will take.

Reviewing deb9df3

Stupid style pedantry:

1. api_client_authorizations_controller.rb:
   

   unless @where['scopes'].nil?
   

   
   Double-negative that made me double-take, should be
   

   if @where['scopes']
   

2. In test "admin search filters where scopes exactly match" we're trying to deprecate 'where' in favor of 'filters' for searching.

Looks good otherwise.

Peter Amstutz wrote:

1. In test "admin search filters where scopes exactly match" we're trying to deprecate 'where' in favor of 'filters' for searching.

I implemented support for filters in ApiClientAuthorizationsController#find_objects_for_index, maintaining the existing limits on what's searchable. This also obsoleted the style point you brought up. Ready for another review. Thanks.

Revision 9f7a232
A couple more style points. Being clear is better than clever (especially since Ruby affords infinite potential for writing unreadable clever code.)

1. .andand is not part of the core language and makes me have to think harder about what the code is doing, especially since you're calling a mutator function with #select!

    if @filters
      wanted_scopes.concat(@filters.map { |attr, operator, operand|
        ((attr == 'scopes') and (operator == '=')) ? operand : nil
      })
    end
    @where.andand.select! { |attr, val| attr == 'uuid' }
    @filters.andand.select! { |attr, operator, operand|
      (attr == 'uuid') and (operator == '=')
    }

Would be more clearer with regular old boring 'if'

    if @filters
      wanted_scopes.concat(@filters.map { |attr, operator, operand|
        ((attr == 'scopes') and (operator == '=')) ? operand : nil
      })
      @filters.select! { |attr, operator, operand|
        (attr == 'uuid') and (operator == '=')
      }
    end
    @where.select! { |attr, val| attr == 'uuid' } if @where

2. Why (auth.scopes & scope_list) (auth.scopes | scope_list)? This seems like a convoluted way to say (auth.scopes scope_list) but I'm sure you have a good reason (does & and | implicitly sort the list?). Add a comment.

Notes, looking at 5902a68d3f8672c38949d4a57f12435f40923a23

You don't need to "include ArvadosTestSupport" again in IntegrationTest -- IntegrationTest is a subclass of TestCase.

I'd like an integration test that params[:reader_tokens] can be supplied as JSON. (You might need an #accept_param_as_json along the lines of #accept_attribute_as_json?)

I'd like to see a test for "ticket created by user A scoped to single object X + token created by user B with scope=all + request object Y which is readable by A but not by B = 404.". From my reading it looks like user B could get all of A's objects that way.

(I feel like I should stop here in case that throws a wrench into the whole works...)

Tom Clegg wrote:

Notes, looking at 5902a68d3f8672c38949d4a57f12435f40923a23

All good suggestions. Pushed all these changes.

I'd like to see a test for "ticket created by user A scoped to single object X + token created by user B with scope=all + request object Y which is readable by A but not by B = 404.". From my reading it looks like user B could get all of A's objects that way.

(I feel like I should stop here in case that throws a wrench into the whole works...)

It did require a little retooling, but not as much as you might've feared. And I think the changes actually make for cleaner code in the end.

Looks great.

The only nitpick I found is that admins can't use their is_admin status to create tokens to arbitrary objects/scopes. That's not necessarily a bad thing, as long as the "create link to share" UI is aware of that limitation. The shortcoming I actually ran into is that "index" doesn't include individually scoped objects. But I don't think that's a problem.

So I think you should merge.

At f7dc278

Looks great.

(As for the "no permission -> 404" comment in the commit message, I think this really is the best result. Saying 403 instead can be a real information leak: "Yes, now that you mention it, somebody at this site has indeed been working with that dataset, but nobody has suggested you should ever know anything about that.")