Project

General

Profile

Actions

Bug #19144

closed

Cannot copy collections on workbench1

Added by Lucas Di Pentima about 1 month ago. Updated about 1 month ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Workbench
Target version:
Start date:
05/26/2022
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

When trying to copy a collection as a non-admin user on Workbench1, we get the following error:

//railsapi.internal/arvados/v1/collections: 403 Forbidden: #<ArvadosModel::PermissionDeniedError: storage_classes_confirmed and storage_classes_confirmed_at attributes cannot be changed, except by setting them to [] and nil respectively> (req-dzkazt4gjfgtbiv3ch5l)

Workbench1 is probably passing the entire object to the create call.

The fix is probably to only copy a specific list of fields.


Subtasks 1 (0 open1 closed)

Task #19157: Review 19144-wb-copy-collections-fixResolvedPeter Amstutz05/26/2022

Actions
Actions #1

Updated by Peter Amstutz about 1 month ago

  • Description updated (diff)
Actions #2

Updated by Peter Amstutz about 1 month ago

  • Assigned To set to Lucas Di Pentima
Actions #3

Updated by Lucas Di Pentima about 1 month ago

  • Status changed from New to In Progress
Actions #4

Updated by Lucas Di Pentima about 1 month ago

I'm not finding any special treatment on the storage_classes_confirmed (and neither on replication_confirmed) field on workbench1 that indicates it gets dropped on a copy operation, so my current hypothesis is that the following RailsAPI callback at the Collection class model needs the proper fields reset:

[...]
  before_validation :strip_signatures_and_update_replication_confirmed
[...]
  def strip_signatures_and_update_replication_confirmed
    if self.manifest_text_changed?
      in_old_manifest = {}
      # manifest_text_was could be nil when dealing with a freshly created snapshot,
      # so we skip this case because there was no real manifest change. (Bug #18005)
      if (not self.replication_confirmed.nil?) and (not self.manifest_text_was.nil?)
        self.class.each_manifest_locator(manifest_text_was) do |match|
          in_old_manifest[match[1]] = true
        end
      end

      stripped_manifest = self.class.munge_manifest_locators(manifest_text) do |match|
        if not self.replication_confirmed.nil? and not in_old_manifest[match[1]]
          # If the new manifest_text contains locators whose hashes
          # weren't in the old manifest_text, storage replication is no
          # longer confirmed.
          self.replication_confirmed_at = nil
          self.replication_confirmed = nil
      end
      [...]
  end
Actions #5

Updated by Lucas Di Pentima about 1 month ago

Upon followup inspection, it seems that workbench1 doesn't include the replication_* fields on its create request, but it does include the storage_classes_* fields. These fields should be treated the same way.

Here's a formatted copy of the params that RailsAPI receives on the create call when attempting to copy a collection from Workbench1

"params":{
    "cluster_id":"",
    "collection":"{
        \"manifest_text\":<removed>,
        \"name\":\"Copy of Source coll 6\",
        \"owner_uuid\":\"ce8i5-tpzed-f7960quf4ivgbxb\",
        \"properties\":{
            \"a\":\"b\",
            \"responsible_person_uuid\":\"ce8i5-tpzed-f7960quf4ivgbxb\" 
        },
        \"storage_classes_confirmed\":[\"default\"],
        \"storage_classes_desired\":[\"default\"],
        \"uuid\":null
    }" 
}

...and here's a copy of the params when copying the same collection from Workbench2:

"params":{
    "cluster_id":"",
    "collection":"{
        \"created_at\":\"2021-12-15T21:54:04.125582000Z\",
        \"current_version_uuid\":\"ce8i5-4zz18-cxxvreo5jstntpg\",
        \"delete_at\":null,
        \"description\":null,
        \"is_trashed\":false,
        \"manifest_text\":<removed>,
        \"modified_at\":\"2022-02-11T19:29:22.555003000Z\",
        \"modified_by_client_uuid\":\"ce8i5-ozdt8-qfoh901onn5potx\",
        \"modified_by_user_uuid\":\"ce8i5-tpzed-f7960quf4ivgbxb\",
        \"name\":\"Copy of: Source coll 6\",
        \"owner_uuid\":\"ce8i5-tpzed-f7960quf4ivgbxb\",
        \"portable_data_hash\":\"4ec1f24c071f9a7dd1e7237a3b0cf214+205\",
        \"preserve_version\":true,
        \"properties\":{
            \"a\":\"b\",
            \"responsible_person_uuid\":\"ce8i5-tpzed-f7960quf4ivgbxb\" 
        },
        \"replication_desired\":null,
        \"storage_classes_desired\":[\"default\"],
        \"trash_at\":null
    }" 
}

Workbench2's code explicitly avoids passing replication_confirmed and storage_classes_confirmed fields, but oddly enough I haven't been able to find code that avoids passing replication_confirmed on Workbench1.

Regarding why the replication_desired field isn't included on WB1's request, I think it is because for some reason, the collection being copied (ce8i5-4zz18-cxxvreo5jstntpg) has it set to null as it is evident on WB2's params.

Actions #6

Updated by Lucas Di Pentima about 1 month ago

Creating a new collection with arv-put with replication_desired set to 2, and then copying it with Workbench1 produced at request that on RailsAPI's side was seen with these params:

"params":{
    "cluster_id":"",
    "collection":"{
        \"manifest_text\":<removed>,
        \"owner_uuid\":\"ce8i5-j7d0g-hcfsyzcwl5bqy4b\",
        \"properties\":{
            \"responsible_person_uuid\":\"ce8i5-tpzed-f7960quf4ivgbxb\" 
        },
        \"storage_classes_confirmed\":[],
        \"storage_classes_desired\":[\"default\"],
        \"uuid\":null
    }" 
}

No replication_desired param on sight. The fact that WB2 request includes this parameter makes me think that there should be code that either whitelist some params for collections or deletes others that aren't allowed, but I haven't been able to find neither of those cases.

I suspect I'm missing something super obvious here...

Actions #7

Updated by Lucas Di Pentima about 1 month ago

Update at 12440f46c - branch 19144-wb-copy-collections-fix
Test run: developer-run-tests: #3161
Failed test re-run: developer-run-tests-remainder: #3311

Quick & dirty fix on WB1 by resetting the affected fields on the actions controller's copy method.
To test against our dev cluster: Use a non-admin user & attempt to copy a collection that already has a storage_classes_confirmed field set to something other than [].

Actions #8

Updated by Peter Amstutz about 1 month ago

Lucas Di Pentima wrote:

Update at 12440f46c - branch 19144-wb-copy-collections-fix
Test run: developer-run-tests: #3161
Failed test re-run: developer-run-tests-remainder: #3311

Quick & dirty fix on WB1 by resetting the affected fields on the actions controller's copy method.
To test against our dev cluster: Use a non-admin user & attempt to copy a collection that already has a storage_classes_confirmed field set to something other than [].

This LGTM

Actions #9

Updated by Lucas Di Pentima about 1 month ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved

Applied in changeset arvados-private:commit:arvados|08c300b0d042f4f9bf80656a533e6233fa121c74.

Actions

Also available in: Atom PDF