Arvados

Brett Smith wrote:

See #1904 for more background. Work is already underway on this on branch 2044-share-button. But it's not 100% there yet. Right now the blocker bug is that every API request Workbench sends is a POST, which prevents you from using API tokens scoped to GET.

This sounds like scope checking bug in API server. We should be validating scope based on the method being used to route the request to a controller action, not the HTTP verb in the request itself, in cases where those differ (e.g., HTTP POST with _method=GET).

Reviewing 651638a.

This might be my bug on the API server end, but unfortunately, when Workbench builds the list of tokens for sharing, the final results are incorrect. At first glance, it seems to catch any token that doesn't just have the 'all' scope. To reproduce:

• Bring up an API server with test fixtures loaded, and Workbench pointed at that.
• Log in to Workbench with the admin API token from the fixtures.
• Go to a Collection page.

Workbench indicates that every Collection is sharable, using the admin_vm token, which is scoped to viewing a specific virtual machine. The link is not actually functional. There are tests for the API server half of this in services/api/test/functional/arvados/v1/api_client_authorizations_controller_test.rb, but maybe they're incomplete.

The changes to ensure_owner_uuid_is_permitted seem to allow many more changes than were permitted before, and I don't follow why that's necessary for this branch. Checking for new_record? inside the self.owner_uuid_changed? branch seems sensible, but I don't understand why it was necessary to remove all the checks after that branch, covering cases where owner_uuid was not changed. Could you please explain?

Brett Smith wrote:

Reviewing 651638a.

This might be my bug on the API server end, but unfortunately, when Workbench builds the list of tokens for sharing, the final results are incorrect.

I figured it out: say ApiClientAuthorization.filter([['scopes', '=', scopelist]]).results instead of using .where. This will let you take out all the select blocks too.

ec07cd1 looks good to merge. Thanks.