[ARVADOS] created: 2.1.0-2110-g0ffd9efd1
Git user
git at public.arvados.org
Thu Mar 17 18:30:31 UTC 2022
at 0ffd9efd1c9e088ec713e2303012463465008d40 (commit)
commit 0ffd9efd1c9e088ec713e2303012463465008d40
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Thu Mar 17 14:26:07 2022 -0400
18180: Support for requesting preemptible instances in CWL
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/doc/user/cwl/cwl-extensions.html.textile.liquid b/doc/user/cwl/cwl-extensions.html.textile.liquid
index dd78e989f..d6148d7ee 100644
--- a/doc/user/cwl/cwl-extensions.html.textile.liquid
+++ b/doc/user/cwl/cwl-extensions.html.textile.liquid
@@ -63,6 +63,9 @@ hints:
cudaComputeCapabilityMin: "9.0"
deviceCountMin: 1
deviceCountMax: 1
+
+ arv:UsePreemptible:
+ usePreemptible: true
{% endcodeblock %}
h2(#RunInSingleContainer). arv:RunInSingleContainer
@@ -164,6 +167,14 @@ table(table table-bordered table-condensed).
|deviceCountMin|integer|Minimum number of GPU devices to allocate on a single node. Required.|
|deviceCountMax|integer|Maximum number of GPU devices to allocate on a single node. Optional. If not specified, same as @minDeviceCount at .|
+h2(#UsePreemptible). arv:UsePreemptible
+
+Specify whether a workflow step should request preemptible (e.g. AWS Spot market) instances. Such instances are generally cheaper, but can be taken back by the cloud provider at any time (preempted) causing the step to fail. When this happens, Arvados will automatically re-try the step, up to the configuration value of @Containers.MaxRetryAttempts@ (default 3) times.
+
+table(table table-bordered table-condensed).
+|_. Field |_. Type |_. Description |
+|usePreemptible|boolean|Required, true to opt-in to using preemptible instances, false to opt-out.|
+
h2. arv:dockerCollectionPDH
This is an optional extension field appearing on the standard @DockerRequirement at . It specifies the portable data hash of the Arvados collection containing the Docker image. If present, it takes precedence over @dockerPull@ or @dockerImageId at .
diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index 656385cc1..8bbc33ba0 100644
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -915,11 +915,6 @@ Clusters:
# If false, containers are scheduled on preemptible instances
# only when requested by the submitter.
#
- # Note that arvados-cwl-runner does not currently offer a
- # feature to request preemptible instances, so this value
- # effectively acts as a cluster-wide decision about whether to
- # use preemptible instances.
- #
# This flag is ignored if no preemptible instance types are
# configured, and has no effect on top-level containers.
AlwaysUsePreemptibleInstances: true
diff --git a/sdk/cwl/arvados_cwl/__init__.py b/sdk/cwl/arvados_cwl/__init__.py
index 826467cc0..c73b358ec 100644
--- a/sdk/cwl/arvados_cwl/__init__.py
+++ b/sdk/cwl/arvados_cwl/__init__.py
@@ -213,6 +213,10 @@ def arg_parser(): # type: () -> argparse.ArgumentParser
parser.add_argument("--http-timeout", type=int,
default=5*60, dest="http_timeout", help="API request timeout in seconds. Default is 300 seconds (5 minutes).")
+ exgroup = parser.add_mutually_exclusive_group()
+ exgroup.add_argument("--enable-preemptible", dest="enable_preemptible", default=None, action="store_true", help="Use preemptible instances. Control individual steps with arv:UsePreemptible hint.")
+ exgroup.add_argument("--disable-preemptible", dest="enable_preemptible", default=None, action="store_false", help="Don't use preemptible instances.")
+
parser.add_argument(
"--skip-schemas",
action="store_true",
@@ -255,7 +259,8 @@ def add_arv_hints():
"http://arvados.org/cwl#ClusterTarget",
"http://arvados.org/cwl#OutputStorageClass",
"http://arvados.org/cwl#ProcessProperties",
- "http://commonwl.org/cwltool#CUDARequirement"
+ "http://commonwl.org/cwltool#CUDARequirement",
+ "http://arvados.org/cwl#UsePreemptible",
])
def exit_signal_handler(sigcode, frame):
diff --git a/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.0.yml b/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.0.yml
index 6e2d4f1d9..443a027ae 100644
--- a/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.0.yml
+++ b/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.0.yml
@@ -385,3 +385,18 @@ $graph:
doc: |
Maximum number of GPU devices to request. If not specified,
same as `cudaDeviceCountMin`.
+
+- name: UsePreemptible
+ type: record
+ extends: cwl:ProcessRequirement
+ inVocab: false
+ doc: |
+ Specify a workflow step should opt-in or opt-out of using preemptible (spot) instances.
+ fields:
+ class:
+ type: string
+ doc: "Always 'arv:ProcessProperties"
+ jsonldPredicate:
+ _id: "@type"
+ _type: "@vocab"
+ usePreemptible: boolean
diff --git a/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.1.yml b/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.1.yml
index 0e81347d7..633edaad2 100644
--- a/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.1.yml
+++ b/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.1.yml
@@ -328,3 +328,18 @@ $graph:
doc: |
Maximum number of GPU devices to request. If not specified,
same as `cudaDeviceCountMin`.
+
+- name: UsePreemptible
+ type: record
+ extends: cwl:ProcessRequirement
+ inVocab: false
+ doc: |
+ Specify a workflow step should opt-in or opt-out of using preemptible (spot) instances.
+ fields:
+ class:
+ type: string
+ doc: "Always 'arv:ProcessProperties"
+ jsonldPredicate:
+ _id: "@type"
+ _type: "@vocab"
+ usePreemptible: boolean
diff --git a/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.2.yml b/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.2.yml
index e9f70bf1c..8ca064a6e 100644
--- a/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.2.yml
+++ b/sdk/cwl/arvados_cwl/arv-cwl-schema-v1.2.yml
@@ -330,3 +330,18 @@ $graph:
doc: |
Maximum number of GPU devices to request. If not specified,
same as `cudaDeviceCountMin`.
+
+- name: UsePreemptible
+ type: record
+ extends: cwl:ProcessRequirement
+ inVocab: false
+ doc: |
+ Specify a workflow step should opt-in or opt-out of using preemptible (spot) instances.
+ fields:
+ class:
+ type: string
+ doc: "Always 'arv:ProcessProperties"
+ jsonldPredicate:
+ _id: "@type"
+ _type: "@vocab"
+ usePreemptible: boolean
diff --git a/sdk/cwl/arvados_cwl/arvcontainer.py b/sdk/cwl/arvados_cwl/arvcontainer.py
index 2a5ff3a13..8c468dd22 100644
--- a/sdk/cwl/arvados_cwl/arvcontainer.py
+++ b/sdk/cwl/arvados_cwl/arvcontainer.py
@@ -300,6 +300,17 @@ class ArvadosContainer(JobBase):
"hardware_capability": aslist(cuda_req["cudaComputeCapability"])[0]
}
+ if runtimeContext.enable_preemptible is False:
+ scheduling_parameters["preemptible"] = False
+ else:
+ preemptible_req, _ = self.get_requirement("http://arvados.org/cwl#UsePreemptible")
+ if preemptible_req:
+ scheduling_parameters["preemptible"] = preemptible_req["usePreemptible"]
+ elif runtimeContext.enable_preemptible is True:
+ scheduling_parameters["preemptible"] = True
+ elif runtimeContext.enable_preemptible is None:
+ pass
+
if self.timelimit is not None and self.timelimit > 0:
scheduling_parameters["max_run_time"] = self.timelimit
@@ -550,6 +561,12 @@ class RunnerContainer(Runner):
if self.enable_dev:
command.append("--enable-dev")
+ if runtimeContext.enable_preemptible is True:
+ command.append("--enable-preemptible")
+
+ if runtimeContext.enable_preemptible is False:
+ command.append("--disable-preemptible")
+
command.extend([workflowpath, "/var/lib/cwl/cwl.input.json"])
container_req["command"] = command
diff --git a/sdk/cwl/arvados_cwl/context.py b/sdk/cwl/arvados_cwl/context.py
index 4239dd3b5..316250106 100644
--- a/sdk/cwl/arvados_cwl/context.py
+++ b/sdk/cwl/arvados_cwl/context.py
@@ -37,6 +37,7 @@ class ArvRuntimeContext(RuntimeContext):
self.always_submit_runner = False
self.collection_cache_size = 256
self.match_local_docker = False
+ self.enable_preemptible = None
super(ArvRuntimeContext, self).__init__(kwargs)
diff --git a/sdk/cwl/arvados_cwl/runner.py b/sdk/cwl/arvados_cwl/runner.py
index ad17950a2..38e2c4d80 100644
--- a/sdk/cwl/arvados_cwl/runner.py
+++ b/sdk/cwl/arvados_cwl/runner.py
@@ -40,7 +40,7 @@ import schema_salad.validate as validate
import arvados.collection
from .util import collectionUUID
-import ruamel.yaml as yaml
+from ruamel.yaml import YAML
from ruamel.yaml.comments import CommentedMap, CommentedSeq
import arvados_cwl.arvdocker
@@ -265,7 +265,8 @@ def upload_dependencies(arvrunner, name, document_loader,
textIO = StringIO(text.decode('utf-8'))
else:
textIO = StringIO(text)
- return yaml.safe_load(textIO)
+ yamlloader = YAML(typ='safe', pure=True)
+ return yamlloader.load(textIO)
else:
return {}
diff --git a/sdk/cwl/tests/test_container.py b/sdk/cwl/tests/test_container.py
index 3de90c8d8..798c5af28 100644
--- a/sdk/cwl/tests/test_container.py
+++ b/sdk/cwl/tests/test_container.py
@@ -1234,6 +1234,103 @@ class TestContainer(unittest.TestCase):
body=JsonDiffMatcher(container_request))
+ # The test passes no builder.resources
+ # Hence the default resources will apply: {'cores': 1, 'ram': 1024, 'outdirSize': 1024, 'tmpdirSize': 1024}
+ @mock.patch("arvados.commands.keepdocker.list_images_in_arv")
+ def test_run_preemptible_hint(self, keepdocker):
+ arvados_cwl.add_arv_hints()
+ for enable_preemptible in (None, True, False):
+ for preemptible_hint in (None, True, False):
+ arv_docker_clear_cache()
+
+ runner = mock.MagicMock()
+ runner.ignore_docker_for_reuse = False
+ runner.intermediate_output_ttl = 0
+ runner.secret_store = cwltool.secrets.SecretStore()
+ runner.api._rootDesc = {"revision": "20210628"}
+
+ keepdocker.return_value = [("zzzzz-4zz18-zzzzzzzzzzzzzz3", "")]
+ runner.api.collections().get().execute.return_value = {
+ "portable_data_hash": "99999999999999999999999999999993+99"}
+
+ if preemptible_hint is not None:
+ hints = [{
+ "class": "http://arvados.org/cwl#UsePreemptible",
+ "usePreemptible": preemptible_hint
+ }]
+ else:
+ hints = []
+
+ tool = cmap({
+ "inputs": [],
+ "outputs": [],
+ "baseCommand": "ls",
+ "arguments": [{"valueFrom": "$(runtime.outdir)"}],
+ "id": "",
+ "class": "CommandLineTool",
+ "cwlVersion": "v1.2",
+ "hints": hints
+ })
+
+ loadingContext, runtimeContext = self.helper(runner)
+
+ runtimeContext.name = 'test_run_enable_preemptible_'+str(enable_preemptible)+str(preemptible_hint)
+ runtimeContext.enable_preemptible = enable_preemptible
+
+ arvtool = cwltool.load_tool.load_tool(tool, loadingContext)
+ arvtool.formatgraph = None
+
+ # Test the interactions between --enable/disable-preemptible
+ # and UsePreemptible hint
+
+ if enable_preemptible is None:
+ if preemptible_hint is None:
+ sched = {}
+ else:
+ sched = {'preemptible': preemptible_hint}
+ else:
+ if preemptible_hint is None:
+ sched = {'preemptible': enable_preemptible}
+ else:
+ sched = {'preemptible': enable_preemptible and preemptible_hint}
+
+ for j in arvtool.job({}, mock.MagicMock(), runtimeContext):
+ j.run(runtimeContext)
+ runner.api.container_requests().create.assert_called_with(
+ body=JsonDiffMatcher({
+ 'environment': {
+ 'HOME': '/var/spool/cwl',
+ 'TMPDIR': '/tmp'
+ },
+ 'name': runtimeContext.name,
+ 'runtime_constraints': {
+ 'vcpus': 1,
+ 'ram': 268435456
+ },
+ 'use_existing': True,
+ 'priority': 500,
+ 'mounts': {
+ '/tmp': {'kind': 'tmp',
+ "capacity": 1073741824
+ },
+ '/var/spool/cwl': {'kind': 'tmp',
+ "capacity": 1073741824 }
+ },
+ 'state': 'Committed',
+ 'output_name': 'Output for step '+runtimeContext.name,
+ 'owner_uuid': 'zzzzz-8i9sb-zzzzzzzzzzzzzzz',
+ 'output_path': '/var/spool/cwl',
+ 'output_ttl': 0,
+ 'container_image': '99999999999999999999999999999993+99',
+ 'command': ['ls', '/var/spool/cwl'],
+ 'cwd': '/var/spool/cwl',
+ 'scheduling_parameters': sched,
+ 'properties': {},
+ 'secret_mounts': {},
+ 'output_storage_classes': ["default"]
+ }))
+
+
class TestWorkflow(unittest.TestCase):
def setUp(self):
diff --git a/sdk/cwl/tests/test_submit.py b/sdk/cwl/tests/test_submit.py
index 10443359b..61892bf2a 100644
--- a/sdk/cwl/tests/test_submit.py
+++ b/sdk/cwl/tests/test_submit.py
@@ -1468,6 +1468,49 @@ class TestSubmit(unittest.TestCase):
self.assertEqual(exited, 0)
+ @stubs
+ def test_submit_enable_preemptible(self, stubs):
+ exited = arvados_cwl.main(
+ ["--submit", "--no-wait", "--api=containers", "--debug", "--enable-preemptible",
+ "tests/wf/submit_wf.cwl", "tests/submit_test_job.json"],
+ stubs.capture_stdout, sys.stderr, api_client=stubs.api, keep_client=stubs.keep_client)
+
+ expect_container = copy.deepcopy(stubs.expect_container_spec)
+ expect_container['command'] = ['arvados-cwl-runner', '--local', '--api=containers',
+ '--no-log-timestamps', '--disable-validate', '--disable-color',
+ '--eval-timeout=20', '--thread-count=0',
+ '--enable-reuse', "--collection-cache-size=256", '--debug', '--on-error=continue',
+ '--enable-preemptible',
+ '/var/lib/cwl/workflow.json#main', '/var/lib/cwl/cwl.input.json']
+
+ stubs.api.container_requests().create.assert_called_with(
+ body=JsonDiffMatcher(expect_container))
+ self.assertEqual(stubs.capture_stdout.getvalue(),
+ stubs.expect_container_request_uuid + '\n')
+ self.assertEqual(exited, 0)
+
+ @stubs
+ def test_submit_disable_preemptible(self, stubs):
+ exited = arvados_cwl.main(
+ ["--submit", "--no-wait", "--api=containers", "--debug", "--disable-preemptible",
+ "tests/wf/submit_wf.cwl", "tests/submit_test_job.json"],
+ stubs.capture_stdout, sys.stderr, api_client=stubs.api, keep_client=stubs.keep_client)
+
+ expect_container = copy.deepcopy(stubs.expect_container_spec)
+ expect_container['command'] = ['arvados-cwl-runner', '--local', '--api=containers',
+ '--no-log-timestamps', '--disable-validate', '--disable-color',
+ '--eval-timeout=20', '--thread-count=0',
+ '--enable-reuse', "--collection-cache-size=256", '--debug', '--on-error=continue',
+ '--disable-preemptible',
+ '/var/lib/cwl/workflow.json#main', '/var/lib/cwl/cwl.input.json']
+
+ stubs.api.container_requests().create.assert_called_with(
+ body=JsonDiffMatcher(expect_container))
+ self.assertEqual(stubs.capture_stdout.getvalue(),
+ stubs.expect_container_request_uuid + '\n')
+ self.assertEqual(exited, 0)
+
+
class TestCreateWorkflow(unittest.TestCase):
existing_workflow_uuid = "zzzzz-7fd4e-validworkfloyml"
expect_workflow = StripYAMLComments(
commit 87a5dba8731b14031a7520b1e33481d04cd49b4a
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 15 10:30:56 2022 -0400
18691: Expand on "frozen" behavior, mention "...even by admins."
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index cf4c346fa..4d57079b2 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -34,7 +34,7 @@ table(table table-bordered table-condensed).
|trash_at|datetime|If @trash_at@ is non-null and in the past, this group and all objects directly or indirectly owned by the group will be hidden from API calls. May be untrashed.||
|delete_at|datetime|If @delete_at@ is non-null and in the past, the group and all objects directly or indirectly owned by the group may be permanently deleted.||
|is_trashed|datetime|True if @trash_at@ is in the past, false if not.||
-|frozen_by_uuid|string|For a frozen project, indicates the user who froze the project; null in all other cases. When a project is frozen, no further changes can be made to the project or its contents.||
+|frozen_by_uuid|string|For a frozen project, indicates the user who froze the project; null in all other cases. When a project is frozen, no further changes can be made to the project or its contents, even by admins. Attempting to add new items or modify, rename, trash, or delete the project or its contents, including any subprojects, will return an error.||
h3. Frozen projects
commit 02e59d25bb5a8b4e672b4ec92eb536284fe21bc5
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 15 10:21:40 2022 -0400
18691: Rephrase expression.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 6deab8952..c71996a37 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -249,9 +249,9 @@ class ArvadosModel < ApplicationRecord
# Return [] if this is a frozen project and the current user can't
# unfreeze
return [] if respond_to?(:frozen_by_uuid) && frozen_by_uuid &&
- !(Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
- current_user.andand.is_admin :
- current_user.can?(manage: uuid))
+ (Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
+ !current_user.andand.is_admin :
+ !current_user.can?(manage: uuid))
# Return [] if nobody can write because this object is inside a
# frozen project
return [] if FrozenGroup.where(uuid: owner_uuid).any?
commit ffc1fff72583dcceb983f76e392dfb8dfc05cde0
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 15 02:58:36 2022 -0400
18691: Comment excluded_trash sql, add more parentheses.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 3ddbafcdb..6deab8952 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -302,6 +302,15 @@ class ArvadosModel < ApplicationRecord
# For details on how the trashed_groups table is constructed, see
# see db/migrate/20200501150153_permission_table.rb
+ # excluded_trash is a SQL expression that determines whether a row
+ # should be excluded from the results due to being trashed.
+ # Trashed items inside frozen projects are invisible to regular
+ # (non-admin) users even when using include_trash, so we have:
+ #
+ # (item_trashed || item_inside_trashed_project)
+ # &&
+ # (!caller_requests_include_trash ||
+ # (item_inside_frozen_project && caller_is_not_admin))
if (admin && include_trash) || sql_table == "api_client_authorizations"
excluded_trash = "false"
else
@@ -322,7 +331,7 @@ class ArvadosModel < ApplicationRecord
# on trashed items.
if !include_trash && sql_table != "api_client_authorizations"
# Only include records where the owner is not trashed
- sql_conds = "NOT #{excluded_trash}"
+ sql_conds = "NOT (#{excluded_trash})"
end
else
# The core of the permission check is a join against the
@@ -422,7 +431,7 @@ class ArvadosModel < ApplicationRecord
" WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 3))) "
end
- sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) AND NOT #{excluded_trash}"
+ sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) AND NOT (#{excluded_trash})"
end
commit fa9d109dd6d719cd840fb83f2f8a0f1ba2eaba21
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 15 02:56:09 2022 -0400
18691: Refactor frozen_groups check.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index ffae867c1..3ddbafcdb 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -629,8 +629,12 @@ class ArvadosModel < ApplicationRecord
if check_uuid.nil?
# old_owner_uuid is nil? New record, no need to check.
elsif !current_user.can?(write: check_uuid)
- logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
- errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
+ if FrozenGroup.where(uuid: check_uuid).any?
+ errors.add :owner_uuid, "cannot be set or changed because #{which} owner is frozen"
+ else
+ logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
+ errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
+ end
raise PermissionDeniedError
elsif rsc_class == Group && Group.find_by_uuid(owner_uuid).group_class != "project"
errors.add :owner_uuid, "must be a project"
@@ -640,8 +644,12 @@ class ArvadosModel < ApplicationRecord
else
# If the object already existed and we're not changing
# owner_uuid, we only need write permission on the object
- # itself.
- if !current_user.can?(write: self.uuid)
+ # itself. (If we're in the act of unfreezing, we only need
+ # :unfreeze permission, which means "what write permission would
+ # be if target weren't frozen")
+ unless ((respond_to?(:frozen_by_uuid) && frozen_by_uuid_in_database && !frozen_by_uuid) ?
+ current_user.can?(unfreeze: uuid) :
+ current_user.can?(write: uuid))
logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} without write permission"
errors.add :uuid, " #{uuid} is not writable by #{current_user.uuid}"
raise PermissionDeniedError
@@ -658,14 +666,7 @@ class ArvadosModel < ApplicationRecord
end
def permission_to_create
- if !current_user.andand.is_active
- return false
- end
- if self.respond_to?(:owner_uuid) && FrozenGroup.where(uuid: owner_uuid).any?
- errors.add :owner_uuid, "#{owner_uuid} is frozen"
- return false
- end
- return true
+ return current_user.andand.is_active
end
def permission_to_update
@@ -682,13 +683,6 @@ class ArvadosModel < ApplicationRecord
logger.warn "User #{current_user.uuid} tried to change uuid of #{self.class.to_s} #{self.uuid_was} to #{self.uuid}"
return false
end
- if self.respond_to?(:owner_uuid)
- frozen = FrozenGroup.where(uuid: [owner_uuid, owner_uuid_in_database]).to_a
- if frozen.any?
- errors.add :uuid, "#{uuid} cannot be modified in frozen project #{frozen[0]}"
- return false
- end
- end
return true
end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 811cd8975..7829a1875 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -87,6 +87,7 @@ class User < ArvadosModel
VAL_FOR_PERM =
{:read => 1,
:write => 2,
+ :unfreeze => 2,
:manage => 3}
@@ -141,6 +142,21 @@ SELECT 1 FROM #{PERMISSION_VIEW}
).any?
return false
end
+
+ if action == :write
+ if FrozenGroup.where(uuid: [target_uuid, target_owner_uuid]).any?
+ # self or parent is frozen
+ return false
+ end
+ elsif action == :unfreeze
+ # "unfreeze" permission means "could write if target weren't
+ # frozen", which is relevant when a user is un-freezing a
+ # project. If the permission query above allows :write, and
+ # the parent isn't also frozen, then un-freeze is allowed.
+ if FrozenGroup.where(uuid: target_owner_uuid).any?
+ return false
+ end
+ end
end
true
end
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 2c084cc88..11c7da090 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -320,7 +320,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
parent = Group.create!(group_class: 'project', name: 'freeze-test-parent', owner_uuid: users(:active).uuid)
proj = Group.create!(group_class: 'project', name: 'freeze-test', owner_uuid: parent.uuid)
proj_inner = Group.create!(group_class: 'project', name: 'freeze-test-inner', owner_uuid: proj.uuid)
- coll = Collection.create!(name: 'foo', manifest_text: '', owner_uuid: proj_inner.uuid)
+ coll = Collection.create!(name: 'freeze-test-collection', manifest_text: '', owner_uuid: proj_inner.uuid)
# Cannot set frozen_by_uuid to a different user
assert_raises do
@@ -423,7 +423,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
# Once project is frozen, cannot change name/contents, move,
# trash, or delete the project or anything beneath it
[proj, proj_inner, coll].each do |frozen|
- assert_raises(StandardError, "should reject rename of #{frozen.uuid} with parent #{frozen.owner_uuid}") do
+ assert_raises(StandardError, "should reject rename of #{frozen.uuid} (#{frozen.name}) with parent #{frozen.owner_uuid}") do
frozen.update_attributes!(name: 'foo2')
end
frozen.reload
@@ -486,6 +486,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
parent.update_attributes!(delete_at: db_current_time)
end
parent.reload
+ assert_nil parent.frozen_by_uuid
act_as_user users(:admin) do
# Even admin cannot change frozen_by_uuid to someone else's UUID.
commit 41947ac005824c35a3eee0a0cb77c78ad2fd8483
Author: Tom Clegg <tom at curii.com>
Date: Mon Mar 14 15:01:16 2022 -0400
18691: Recommend postgresql 10+ (9 is unsupported anyway).
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/doc/install/install-postgresql.html.textile.liquid b/doc/install/install-postgresql.html.textile.liquid
index 1413890cd..a9614b9be 100644
--- a/doc/install/install-postgresql.html.textile.liquid
+++ b/doc/install/install-postgresql.html.textile.liquid
@@ -9,7 +9,7 @@ Copyright (C) The Arvados Authors. All rights reserved.
SPDX-License-Identifier: CC-BY-SA-3.0
{% endcomment %}
-Arvados requires at least version *9.4* of PostgreSQL.
+Arvados requires at least version *9.4* of PostgreSQL. We recommend using version 10 or newer.
* "AWS":#aws
* "CentOS 7":#centos7
commit ff6e60eeb9300b12c3f6a6e645b0c6111cb40713
Author: Tom Clegg <tom at curii.com>
Date: Mon Mar 14 14:54:57 2022 -0400
18691: Show readable_by query plan in test suite.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 647139d9e..efc43dfde 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -602,4 +602,17 @@ class PermissionTest < ActiveSupport::TestCase
end
end
end
+
+ # Show query plan for readable_by query. The plan for a test db
+ # might not resemble the plan for a production db, but it doesn't
+ # hurt to show the test db plan in test logs, and the .
+ [false, true].each do |include_trash|
+ test "query plan, include_trash=#{include_trash}" do
+ sql = Collection.readable_by(users(:active), include_trash: include_trash).to_sql
+ sql = "explain analyze #{sql}"
+ STDERR.puts sql
+ q = ActiveRecord::Base.connection.exec_query(sql)
+ q.rows.each do |row| STDERR.puts(row) end
+ end
+ end
end
commit b7f4367d5a6257aa7e2a138d4ee70786b4b04238
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 8 15:23:24 2022 -0500
18691: Prevent freezing trashed project / trashing frozen project.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 216397a1b..ee8690acb 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -116,6 +116,9 @@ class Group < ArvadosModel
if !new_record? && !current_user.can?(manage: uuid)
raise PermissionDeniedError
end
+ if trash_at || delete_at || !new_record? && TrashedGroup.where(group_uuid: uuid).any?
+ errors.add(:frozen_by_uuid, "cannot be set on a trashed project")
+ end
if frozen_by_uuid_was.nil?
if Rails.configuration.API.FreezeProjectRequiresDescription && !attribute_present?(:description)
errors.add(:frozen_by_uuid, "can only be set if description is non-empty")
@@ -131,36 +134,38 @@ class Group < ArvadosModel
end
def update_trash
- if saved_change_to_trash_at? or saved_change_to_owner_uuid?
- # The group was added or removed from the trash.
- #
- # Strategy:
- # Compute project subtree, propagating trash_at to subprojects
- # Remove groups that don't belong from trash
- # Add/update groups that do belong in the trash
-
- temptable = "group_subtree_#{rand(2**64).to_s(10)}"
- ActiveRecord::Base.connection.exec_query %{
-create temporary table #{temptable} on commit drop
-as select * from project_subtree_with_trash_at($1, LEAST($2, $3)::timestamp)
-},
- 'Group.update_trash.select',
- [[nil, self.uuid],
- [nil, TrashedGroup.find_by_group_uuid(self.owner_uuid).andand.trash_at],
- [nil, self.trash_at]]
+ return unless saved_change_to_trash_at? || saved_change_to_owner_uuid?
- ActiveRecord::Base.connection.exec_delete %{
-delete from trashed_groups where group_uuid in (select target_uuid from #{temptable} where trash_at is NULL);
-},
- "Group.update_trash.delete"
+ # The group was added or removed from the trash.
+ #
+ # Strategy:
+ # Compute project subtree, propagating trash_at to subprojects
+ # Ensure none of the newly trashed descendants were frozen (if so, bail out)
+ # Remove groups that don't belong from trash
+ # Add/update groups that do belong in the trash
- ActiveRecord::Base.connection.exec_query %{
-insert into trashed_groups (group_uuid, trash_at)
- select target_uuid as group_uuid, trash_at from #{temptable} where trash_at is not NULL
-on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
-},
- "Group.update_trash.insert"
+ temptable = "group_subtree_#{rand(2**64).to_s(10)}"
+ ActiveRecord::Base.connection.exec_query(
+ "create temporary table #{temptable} on commit drop " +
+ "as select * from project_subtree_with_trash_at($1, LEAST($2, $3)::timestamp)",
+ "Group.update_trash.select",
+ [[nil, self.uuid],
+ [nil, TrashedGroup.find_by_group_uuid(self.owner_uuid).andand.trash_at],
+ [nil, self.trash_at]])
+ frozen_descendants = ActiveRecord::Base.connection.exec_query(
+ "select uuid from frozen_groups, #{temptable} where uuid = target_uuid",
+ "Group.update_trash.check_frozen")
+ if frozen_descendants.any?
+ raise ArgumentError.new("cannot trash project containing frozen project #{frozen_descendants[0]["uuid"]}")
end
+ ActiveRecord::Base.connection.exec_delete(
+ "delete from trashed_groups where group_uuid in (select target_uuid from #{temptable} where trash_at is NULL)",
+ "Group.update_trash.delete")
+ ActiveRecord::Base.connection.exec_query(
+ "insert into trashed_groups (group_uuid, trash_at) "+
+ "select target_uuid as group_uuid, trash_at from #{temptable} where trash_at is not NULL " +
+ "on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at",
+ "Group.update_trash.insert")
end
def update_frozen
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 1a37eb058..2c084cc88 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -364,6 +364,21 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
assert_match /can only be set if properties\[frobity\] value is non-empty/, err.inspect
proj.reload
+ # Cannot set frozen_by_uuid while project or its parent is
+ # trashed
+ [parent, proj].each do |trashed|
+ trashed.update_attributes!(trash_at: db_current_time)
+ err = assert_raises do
+ proj.update_attributes!(
+ frozen_by_uuid: users(:active).uuid,
+ description: 'ready to freeze',
+ properties: {'frobity' => 'bar baz'})
+ end
+ assert_match /cannot be set on a trashed project/, err.inspect
+ proj.reload
+ trashed.update_attributes!(trash_at: nil)
+ end
+
# Can set frozen_by_uuid if all conditions are met
ok = proj.update_attributes(
frozen_by_uuid: users(:active).uuid,
commit 8648b0114ebb2263953ce92b2ae93cd3a5e0df59
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 8 14:16:06 2022 -0500
18691: include_trash does not return trash in frozen projects.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index a979338e4..ffae867c1 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -297,26 +297,32 @@ class ArvadosModel < ApplicationRecord
user_uuids = users_list.map { |u| u.uuid }
all_user_uuids = []
+ admin = users_list.select { |u| u.is_admin }.any?
+
# For details on how the trashed_groups table is constructed, see
# see db/migrate/20200501150153_permission_table.rb
- exclude_trashed_records = ""
- if !include_trash and (sql_table == "groups" or sql_table == "collections") then
- # Only include records that are not trashed
- exclude_trashed_records = "AND (#{sql_table}.trash_at is NULL or #{sql_table}.trash_at > statement_timestamp())"
- end
+ if (admin && include_trash) || sql_table == "api_client_authorizations"
+ excluded_trash = "false"
+ else
+ excluded_trash = "(#{sql_table}.owner_uuid IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " +
+ "WHERE trash_at <= statement_timestamp()))"
+ if sql_table == "groups" || sql_table == "collections"
+ excluded_trash = "(#{excluded_trash} OR #{sql_table}.trash_at <= statement_timestamp() IS TRUE)"
+ end
- trashed_check = ""
- if !include_trash && sql_table != "api_client_authorizations"
- trashed_check = "#{sql_table}.owner_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " +
- "where trash_at <= statement_timestamp()) #{exclude_trashed_records}"
+ if include_trash
+ # Exclude trash inside frozen projects
+ excluded_trash = "(#{excluded_trash} AND #{sql_table}.owner_uuid IN (SELECT uuid FROM #{FROZEN_GROUPS}))"
+ end
end
- if users_list.select { |u| u.is_admin }.any?
- # Admin skips most permission checks, but still want to filter on trashed items.
+ if admin
+ # Admin skips most permission checks, but still want to filter
+ # on trashed items.
if !include_trash && sql_table != "api_client_authorizations"
# Only include records where the owner is not trashed
- sql_conds = trashed_check
+ sql_conds = "NOT #{excluded_trash}"
end
else
# The core of the permission check is a join against the
@@ -416,7 +422,7 @@ class ArvadosModel < ApplicationRecord
" WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 3))) "
end
- sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) #{trashed_check.empty? ? "" : "AND"} #{trashed_check}"
+ sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) AND NOT #{excluded_trash}"
end
commit 2e62663846f7cf0f40fc96c740a0208c347d52ae
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 8 14:16:04 2022 -0500
18691: Test hiding of trashed items inside a frozen project.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 0819c2306..fcdce0e60 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -920,4 +920,24 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
assert_response 422
end
+
+ test "include_trash does not return trash inside frozen project" do
+ authorize_with :active
+ trashtime = Time.now - 1.second
+ outerproj = Group.create!(group_class: 'project')
+ innerproj = Group.create!(group_class: 'project', owner_uuid: outerproj.uuid)
+ innercoll = Collection.create!(name: 'inner-not-trashed', owner_uuid: innerproj.uuid)
+ innertrash = Collection.create!(name: 'inner-trashed', owner_uuid: innerproj.uuid, trash_at: trashtime)
+ innertrashproj = Group.create!(group_class: 'project', name: 'inner-trashed-proj', owner_uuid: innerproj.uuid, trash_at: trashtime)
+ outertrash = Collection.create!(name: 'outer-trashed', owner_uuid: outerproj.uuid, trash_at: trashtime)
+ innerproj.update_attributes!(frozen_by_uuid: users(:active).uuid)
+ get :contents, params: {id: outerproj.uuid, include_trash: true, recursive: true}
+ assert_response :success
+ uuids = json_response['items'].collect { |item| item['uuid'] }
+ assert_includes uuids, outertrash.uuid
+ assert_includes uuids, innerproj.uuid
+ assert_includes uuids, innercoll.uuid
+ refute_includes uuids, innertrash.uuid
+ refute_includes uuids, innertrashproj.uuid
+ end
end
commit c11604747c8a58993d8839eb23859dd1dc6ebc4e
Author: Tom Clegg <tom at curii.com>
Date: Tue Mar 8 10:51:52 2022 -0500
18691: Test preventing trashing parent of frozen project.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index bbb0f1957..1a37eb058 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -317,7 +317,8 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
test "freeze project" do
act_as_user users(:active) do
Rails.configuration.API.UnfreezeProjectRequiresAdmin = false
- proj = Group.create!(group_class: 'project', name: 'freeze-test', owner_uuid: users(:active).uuid)
+ parent = Group.create!(group_class: 'project', name: 'freeze-test-parent', owner_uuid: users(:active).uuid)
+ proj = Group.create!(group_class: 'project', name: 'freeze-test', owner_uuid: parent.uuid)
proj_inner = Group.create!(group_class: 'project', name: 'freeze-test-inner', owner_uuid: proj.uuid)
coll = Collection.create!(name: 'foo', manifest_text: '', owner_uuid: proj_inner.uuid)
@@ -431,6 +432,10 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
frozen.update_attributes!(trash_at: db_current_time)
end
frozen.reload
+ assert_raises(StandardError, "should reject setting delete_at of #{frozen.uuid}") do
+ frozen.update_attributes!(delete_at: db_current_time)
+ end
+ frozen.reload
assert_raises(StandardError, "should reject delete of #{frozen.uuid}") do
frozen.destroy
end
@@ -457,6 +462,16 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
assert_match /can only be changed by an admin user, once set/, err.inspect
proj.reload
+ # Cannot trash or delete a frozen project's ancestor
+ assert_raises(StandardError, "should not be able to set trash_at on parent of frozen project") do
+ parent.update_attributes!(trash_at: db_current_time)
+ end
+ parent.reload
+ assert_raises(StandardError, "should not be able to set delete_at on parent of frozen project") do
+ parent.update_attributes!(delete_at: db_current_time)
+ end
+ parent.reload
+
act_as_user users(:admin) do
# Even admin cannot change frozen_by_uuid to someone else's UUID.
err = assert_raises do
commit f7d78f308d8ae5b89d5634069da892bf31bab459
Author: Tom Clegg <tom at curii.com>
Date: Fri Mar 4 12:45:44 2022 -0500
18691: Return null for empty frozen_by_uuid in controller response.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index 6f5b4bbb8..cf4c346fa 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -34,7 +34,7 @@ table(table table-bordered table-condensed).
|trash_at|datetime|If @trash_at@ is non-null and in the past, this group and all objects directly or indirectly owned by the group will be hidden from API calls. May be untrashed.||
|delete_at|datetime|If @delete_at@ is non-null and in the past, the group and all objects directly or indirectly owned by the group may be permanently deleted.||
|is_trashed|datetime|True if @trash_at@ is in the past, false if not.||
-|frozen_by_uuid|string|For a frozen project, indicates the user who froze the project. Empty in all other cases. When a project is frozen, no further changes can be made to the project or its contents.||
+|frozen_by_uuid|string|For a frozen project, indicates the user who froze the project; null in all other cases. When a project is frozen, no further changes can be made to the project or its contents.||
h3. Frozen projects
diff --git a/lib/controller/handler_test.go b/lib/controller/handler_test.go
index 723e1011f..817cff796 100644
--- a/lib/controller/handler_test.go
+++ b/lib/controller/handler_test.go
@@ -367,16 +367,14 @@ func (s *HandlerSuite) CheckObjectType(c *check.C, url string, token string, ski
for k := range direct {
if _, ok := skippedFields[k]; ok {
continue
- } else if val, ok := proxied[k]; ok {
- if direct["kind"] == "arvados#collection" && k == "manifest_text" {
- // Tokens differ from request to request
- c.Check(strings.Split(val.(string), "+A")[0], check.Equals, strings.Split(direct[k].(string), "+A")[0])
- } else {
- c.Check(val, check.DeepEquals, direct[k],
- check.Commentf("RailsAPI %s key %q's value %q differs from controller's %q.", direct["kind"], k, direct[k], val))
- }
- } else {
+ } else if val, ok := proxied[k]; !ok {
c.Errorf("%s's key %q missing on controller's response.", direct["kind"], k)
+ } else if direct["kind"] == "arvados#collection" && k == "manifest_text" {
+ // Tokens differ from request to request
+ c.Check(strings.Split(val.(string), "+A")[0], check.Equals, strings.Split(direct[k].(string), "+A")[0])
+ } else {
+ c.Check(val, check.DeepEquals, direct[k],
+ check.Commentf("RailsAPI %s key %q's value %q differs from controller's %q.", direct["kind"], k, direct[k], val))
}
}
}
diff --git a/lib/controller/router/response.go b/lib/controller/router/response.go
index c0c599be8..42b343559 100644
--- a/lib/controller/router/response.go
+++ b/lib/controller/router/response.go
@@ -208,7 +208,7 @@ func (rtr *router) mungeItemFields(tmp map[string]interface{}) {
// they appear in responses as null, rather than a
// zero value.
switch k {
- case "output_uuid", "output_name", "log_uuid", "description", "requesting_container_uuid", "container_uuid":
+ case "output_uuid", "output_name", "log_uuid", "description", "requesting_container_uuid", "container_uuid", "modified_by_client_uuid", "frozen_by_uuid":
if v == "" {
tmp[k] = nil
}
commit e4ea68ad3b9ad92ec3f14e396cec3fd9dec96f63
Author: Tom Clegg <tom at curii.com>
Date: Thu Mar 3 16:28:04 2022 -0500
18691: Update database indexes for frozen_by_uuid.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index e3df46029..216397a1b 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -250,4 +250,8 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
return true
end
end
+
+ def self.full_text_searchable_columns
+ super - ["frozen_by_uuid"]
+ end
end
diff --git a/services/api/db/migrate/20220303204419_add_frozen_by_uuid_to_group_search_index.rb b/services/api/db/migrate/20220303204419_add_frozen_by_uuid_to_group_search_index.rb
new file mode 100644
index 000000000..52fb16b2a
--- /dev/null
+++ b/services/api/db/migrate/20220303204419_add_frozen_by_uuid_to_group_search_index.rb
@@ -0,0 +1,17 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class AddFrozenByUuidToGroupSearchIndex < ActiveRecord::Migration[5.0]
+ disable_ddl_transaction!
+
+ def up
+ remove_index :groups, :name => 'groups_search_index'
+ add_index :groups, ["uuid", "owner_uuid", "modified_by_client_uuid", "modified_by_user_uuid", "name", "group_class", "frozen_by_uuid"], name: 'groups_search_index', algorithm: :concurrently
+ end
+
+ def down
+ remove_index :groups, :name => 'groups_search_index'
+ add_index :groups, ["uuid", "owner_uuid", "modified_by_client_uuid", "modified_by_user_uuid", "name", "group_class"], name: 'groups_search_index', algorithm: :concurrently
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index cf92bcad9..cfe21f7c9 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -1803,7 +1803,7 @@ CREATE INDEX group_index_on_properties ON public.groups USING gin (properties);
-- Name: groups_search_index; Type: INDEX; Schema: public; Owner: -
--
-CREATE INDEX groups_search_index ON public.groups USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, name, group_class);
+CREATE INDEX groups_search_index ON public.groups USING btree (uuid, owner_uuid, modified_by_client_uuid, modified_by_user_uuid, name, group_class, frozen_by_uuid);
--
@@ -3184,6 +3184,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20210816191509'),
('20211027154300'),
('20220224203102'),
-('20220301155729');
+('20220301155729'),
+('20220303204419');
commit 5369edc0bb6be97f530f5f088d20faf401d6a05f
Author: Tom Clegg <tom at curii.com>
Date: Thu Mar 3 15:23:06 2022 -0500
18691: Fix wrong column name in clear_permissions.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 93bafc45a..e3df46029 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -194,11 +194,14 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
def clear_permissions_trash_frozen
MaterializedPermission.where(target_uuid: uuid).delete_all
- ['trashed_groups', 'frozen_groups'].each do |table|
- ActiveRecord::Base.connection.exec_delete %{
- delete from #{table} where group_uuid=$1
- }, "Group.clear_permissions_trash_frozen", [[nil, self.uuid]]
- end
+ ActiveRecord::Base.connection.exec_delete(
+ "delete from trashed_groups where group_uuid=$1",
+ "Group.clear_permissions_trash_frozen",
+ [[nil, self.uuid]])
+ ActiveRecord::Base.connection.exec_delete(
+ "delete from frozen_groups where uuid=$1",
+ "Group.clear_permissions_trash_frozen",
+ [[nil, self.uuid]])
end
def assign_name
commit 84f7c31b11b90bbb7f41d3863a68560fc19c4fa4
Author: Tom Clegg <tom at curii.com>
Date: Thu Mar 3 13:30:37 2022 -0500
18691: Document groups.frozen_by_uuid attribute.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/doc/api/methods/groups.html.textile.liquid b/doc/api/methods/groups.html.textile.liquid
index e4b8594dd..6f5b4bbb8 100644
--- a/doc/api/methods/groups.html.textile.liquid
+++ b/doc/api/methods/groups.html.textile.liquid
@@ -34,6 +34,17 @@ table(table table-bordered table-condensed).
|trash_at|datetime|If @trash_at@ is non-null and in the past, this group and all objects directly or indirectly owned by the group will be hidden from API calls. May be untrashed.||
|delete_at|datetime|If @delete_at@ is non-null and in the past, the group and all objects directly or indirectly owned by the group may be permanently deleted.||
|is_trashed|datetime|True if @trash_at@ is in the past, false if not.||
+|frozen_by_uuid|string|For a frozen project, indicates the user who froze the project. Empty in all other cases. When a project is frozen, no further changes can be made to the project or its contents.||
+
+h3. Frozen projects
+
+A user with @manage@ permission can set the @frozen_by_uuid@ attribute of a @project@ group to their own user UUID. Once this is done, no further changes can be made to the project or its contents, including subprojects.
+
+The @frozen_by_uuid@ attribute can be cleared by an admin user. It can also be cleared by a user with @manage@ permission, unless the @API.UnfreezeProjectRequiresAdmin@ configuration setting is active.
+
+The optional @API.FreezeProjectRequiresDescription@ and @API.FreezeProjectRequiresProperties@ configuration settings can be used to prevent users from freezing projects that have empty @description@ and/or specified @properties@ entries.
+
+h3. Filter groups
@filter@ groups are virtual groups; they can not own other objects. Filter groups have a special @properties@ field named @filters@, which must be an array of filter conditions. See "list method filters":{{site.baseurl}}/api/methods.html#filters for details on the syntax of valid filters, but keep in mind that the attributes must include the object type (@collections@, @container_requests@, @groups@, @workflows@), separated with a dot from the field to be filtered on.
commit c28cc42b6c3ff5674ac8c893005435c3f077aee5
Author: Tom Clegg <tom at curii.com>
Date: Thu Mar 3 13:30:18 2022 -0500
18691: Export new frozen-project configs.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/config/export.go b/lib/config/export.go
index 4e903a8b3..db413b97b 100644
--- a/lib/config/export.go
+++ b/lib/config/export.go
@@ -62,6 +62,8 @@ var whitelist = map[string]bool{
"API": true,
"API.AsyncPermissionsUpdateInterval": false,
"API.DisabledAPIs": false,
+ "API.FreezeProjectRequiresDescription": true,
+ "API.FreezeProjectRequiresProperties": true,
"API.KeepServiceRequestTimeout": false,
"API.MaxConcurrentRequests": false,
"API.MaxIndexDatabaseRead": false,
@@ -72,6 +74,7 @@ var whitelist = map[string]bool{
"API.MaxTokenLifetime": false,
"API.RequestTimeout": true,
"API.SendTimeout": true,
+ "API.UnfreezeProjectRequiresAdmin": true,
"API.VocabularyPath": false,
"API.WebsocketClientEventQueue": false,
"API.WebsocketServerEventQueue": false,
commit 99b2925ae7d88e3fbd1d6449de994552e468b150
Author: Tom Clegg <tom at curii.com>
Date: Thu Mar 3 13:09:57 2022 -0500
18691: Add frozen_by_uuid field to Go SDK.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/sdk/go/arvados/group.go b/sdk/go/arvados/group.go
index d5243dc17..ad7ac1ee2 100644
--- a/sdk/go/arvados/group.go
+++ b/sdk/go/arvados/group.go
@@ -26,6 +26,7 @@ type Group struct {
Properties map[string]interface{} `json:"properties"`
WritableBy []string `json:"writable_by,omitempty"`
Description string `json:"description"`
+ FrozenByUUID string `json:"frozen_by_uuid"`
}
// GroupList is an arvados#groupList resource.
commit ca31106245ce5a5d65ca82142a2f819cdfc87252
Author: Tom Clegg <tom at curii.com>
Date: Thu Mar 3 13:09:34 2022 -0500
18691: Test moving items into/out of frozen project.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index ff079e1b5..bbb0f1957 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -404,19 +404,29 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
cr.destroy
end
- # Once project is frozen, cannot change name/contents, trash, or
- # delete the project or anything beneath it
+ # Once project is frozen, cannot change name/contents, move,
+ # trash, or delete the project or anything beneath it
[proj, proj_inner, coll].each do |frozen|
assert_raises(StandardError, "should reject rename of #{frozen.uuid} with parent #{frozen.owner_uuid}") do
frozen.update_attributes!(name: 'foo2')
end
frozen.reload
+
if frozen.is_a?(Collection)
assert_raises(StandardError, "should reject manifest change of #{frozen.uuid}") do
frozen.update_attributes!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo\n")
end
- frozen.reload
+ else
+ assert_raises(StandardError, "should reject moving a project into #{frozen.uuid}") do
+ groups(:private).update_attributes!(owner_uuid: frozen.uuid)
+ end
+ end
+ frozen.reload
+
+ assert_raises(StandardError, "should reject moving #{frozen.uuid} to a different parent project") do
+ frozen.update_attributes!(owner_uuid: groups(:private).uuid)
end
+ frozen.reload
assert_raises(StandardError, "should reject setting trash_at of #{frozen.uuid}") do
frozen.update_attributes!(trash_at: db_current_time)
end
commit 3cb6fe609aa9e0512d282e21501304eda3fbfe31
Author: Tom Clegg <tom at curii.com>
Date: Thu Mar 3 11:29:25 2022 -0500
18691: Return empty writable_by for items inside frozen projects.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 029034d8b..a979338e4 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -246,6 +246,15 @@ class ArvadosModel < ApplicationRecord
# If current user cannot write this object, just return
# [self.owner_uuid].
def writable_by
+ # Return [] if this is a frozen project and the current user can't
+ # unfreeze
+ return [] if respond_to?(:frozen_by_uuid) && frozen_by_uuid &&
+ !(Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
+ current_user.andand.is_admin :
+ current_user.can?(manage: uuid))
+ # Return [] if nobody can write because this object is inside a
+ # frozen project
+ return [] if FrozenGroup.where(uuid: owner_uuid).any?
return [owner_uuid] if not current_user
unless (owner_uuid == current_user.uuid or
current_user.is_admin or
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 513c1dae6..ff079e1b5 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -425,6 +425,9 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
frozen.destroy
end
frozen.reload
+ if frozen != proj
+ assert_equal [], frozen.writable_by
+ end
end
# User with manage permission can unfreeze, then create items
commit c67c1c80023e26b937cf61c497fc2c244b49fc5d
Author: Tom Clegg <tom at curii.com>
Date: Thu Mar 3 11:26:07 2022 -0500
18691: Test freeze requires manage permission.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index d71a0b5f9..513c1dae6 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -327,6 +327,19 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
end
proj.reload
+ # Cannot set frozen_by_uuid without can_manage permission
+ act_as_system_user do
+ Link.create!(link_class: 'permission', name: 'can_write', tail_uuid: users(:spectator).uuid, head_uuid: proj.uuid)
+ end
+ act_as_user users(:spectator) do
+ # First confirm we have write permission
+ assert Collection.create(name: 'bar', owner_uuid: proj.uuid)
+ assert_raises(ArvadosModel::PermissionDeniedError) do
+ proj.update_attributes!(frozen_by_uuid: users(:spectator).uuid)
+ end
+ end
+ proj.reload
+
# Cannot set frozen_by_uuid without description (if so configured)
Rails.configuration.API.FreezeProjectRequiresDescription = true
err = assert_raises do
commit a65b70aa6e255b5c347b7d84ae7b83646d547c01
Author: Tom Clegg <tom at curii.com>
Date: Wed Mar 2 14:32:42 2022 -0500
18691: Prevent changes in frozen projects.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 1c842c48d..029034d8b 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -643,7 +643,14 @@ class ArvadosModel < ApplicationRecord
end
def permission_to_create
- current_user.andand.is_active
+ if !current_user.andand.is_active
+ return false
+ end
+ if self.respond_to?(:owner_uuid) && FrozenGroup.where(uuid: owner_uuid).any?
+ errors.add :owner_uuid, "#{owner_uuid} is frozen"
+ return false
+ end
+ return true
end
def permission_to_update
@@ -660,6 +667,13 @@ class ArvadosModel < ApplicationRecord
logger.warn "User #{current_user.uuid} tried to change uuid of #{self.class.to_s} #{self.uuid_was} to #{self.uuid}"
return false
end
+ if self.respond_to?(:owner_uuid)
+ frozen = FrozenGroup.where(uuid: [owner_uuid, owner_uuid_in_database]).to_a
+ if frozen.any?
+ errors.add :uuid, "#{uuid} cannot be modified in frozen project #{frozen[0]}"
+ return false
+ end
+ end
return true
end
diff --git a/services/api/app/models/frozen_group.rb b/services/api/app/models/frozen_group.rb
new file mode 100644
index 000000000..bf4ee5d0b
--- /dev/null
+++ b/services/api/app/models/frozen_group.rb
@@ -0,0 +1,6 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class FrozenGroup < ApplicationRecord
+end
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index d6f698432..93bafc45a 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -23,6 +23,7 @@ class Group < ArvadosModel
before_create :assign_name
after_create :after_ownership_change
after_create :update_trash
+ after_create :update_frozen
before_update :before_ownership_change
after_update :after_ownership_change
@@ -30,7 +31,8 @@ class Group < ArvadosModel
after_create :add_role_manage_link
after_update :update_trash
- before_destroy :clear_permissions_and_trash
+ after_update :update_frozen
+ before_destroy :clear_permissions_trash_frozen
api_accessible :user, extend: :common do |t|
t.add :name
@@ -161,6 +163,22 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
end
end
+ def update_frozen
+ return unless saved_change_to_frozen_by_uuid? || saved_change_to_owner_uuid?
+ temptable = "group_subtree_#{rand(2**64).to_s(10)}"
+ ActiveRecord::Base.connection.exec_query(
+ "create temporary table #{temptable} on commit drop as select * from project_subtree_with_is_frozen($1,$2)",
+ "Group.update_frozen.select",
+ [[nil, self.uuid],
+ [nil, !self.frozen_by_uuid.nil?]])
+ ActiveRecord::Base.connection.exec_delete(
+ "delete from frozen_groups where uuid in (select uuid from #{temptable} where not is_frozen)",
+ "Group.update_frozen.delete")
+ ActiveRecord::Base.connection.exec_query(
+ "insert into frozen_groups (uuid) select uuid from #{temptable} where is_frozen on conflict do nothing",
+ "Group.update_frozen.insert")
+ end
+
def before_ownership_change
if owner_uuid_changed? and !self.owner_uuid_was.nil?
MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
@@ -174,12 +192,13 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
end
end
- def clear_permissions_and_trash
+ def clear_permissions_trash_frozen
MaterializedPermission.where(target_uuid: uuid).delete_all
- ActiveRecord::Base.connection.exec_delete %{
-delete from trashed_groups where group_uuid=$1
-}, "Group.clear_permissions_and_trash", [[nil, self.uuid]]
-
+ ['trashed_groups', 'frozen_groups'].each do |table|
+ ActiveRecord::Base.connection.exec_delete %{
+ delete from #{table} where group_uuid=$1
+ }, "Group.clear_permissions_trash_frozen", [[nil, self.uuid]]
+ end
end
def assign_name
@@ -217,4 +236,15 @@ delete from trashed_groups where group_uuid=$1
end
end
end
+
+ def permission_to_update
+ if !super
+ return false
+ elsif frozen_by_uuid && frozen_by_uuid_in_database
+ errors.add :uuid, "#{uuid} is frozen and cannot be modified"
+ return false
+ else
+ return true
+ end
+ end
end
diff --git a/services/api/db/migrate/20220301155729_frozen_groups.rb b/services/api/db/migrate/20220301155729_frozen_groups.rb
new file mode 100644
index 000000000..730d051ce
--- /dev/null
+++ b/services/api/db/migrate/20220301155729_frozen_groups.rb
@@ -0,0 +1,39 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require '20200501150153_permission_table_constants'
+
+class FrozenGroups < ActiveRecord::Migration[5.0]
+ def up
+ create_table :frozen_groups, :id => false do |t|
+ t.string :uuid
+ end
+ add_index :frozen_groups, :uuid, :unique => true
+
+ ActiveRecord::Base.connection.execute %{
+create or replace function project_subtree_with_is_frozen (starting_uuid varchar(27), starting_is_frozen boolean)
+returns table (uuid varchar(27), is_frozen boolean)
+STABLE
+language SQL
+as $$
+WITH RECURSIVE
+ project_subtree(uuid, is_frozen) as (
+ values (starting_uuid, starting_is_frozen)
+ union
+ select groups.uuid, project_subtree.is_frozen or groups.frozen_by_uuid is not null
+ from groups join project_subtree on project_subtree.uuid = groups.owner_uuid
+ )
+ select uuid, is_frozen from project_subtree;
+$$;
+}
+
+ # Initialize the table. After this, it is updated incrementally.
+ # See app/models/group.rb#update_frozen_groups
+ refresh_frozen
+ end
+
+ def down
+ drop_table :frozen_groups
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 573a4375c..cf92bcad9 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -190,6 +190,24 @@ case (edges.edge_id = perm_edge_id)
$$;
+--
+-- Name: project_subtree_with_is_frozen(character varying, boolean); Type: FUNCTION; Schema: public; Owner: -
+--
+
+CREATE FUNCTION public.project_subtree_with_is_frozen(starting_uuid character varying, starting_is_frozen boolean) RETURNS TABLE(uuid character varying, is_frozen boolean)
+ LANGUAGE sql STABLE
+ AS $$
+WITH RECURSIVE
+ project_subtree(uuid, is_frozen) as (
+ values (starting_uuid, starting_is_frozen)
+ union
+ select groups.uuid, project_subtree.is_frozen or groups.frozen_by_uuid is not null
+ from groups join project_subtree on (groups.owner_uuid = project_subtree.uuid)
+ )
+ select uuid, is_frozen from project_subtree;
+$$;
+
+
--
-- Name: project_subtree_with_trash_at(character varying, timestamp without time zone); Type: FUNCTION; Schema: public; Owner: -
--
@@ -548,6 +566,15 @@ CREATE SEQUENCE public.containers_id_seq
ALTER SEQUENCE public.containers_id_seq OWNED BY public.containers.id;
+--
+-- Name: frozen_groups; Type: TABLE; Schema: public; Owner: -
+--
+
+CREATE TABLE public.frozen_groups (
+ uuid character varying
+);
+
+
--
-- Name: groups; Type: TABLE; Schema: public; Owner: -
--
@@ -2059,6 +2086,13 @@ CREATE INDEX index_containers_on_secret_mounts_md5 ON public.containers USING bt
CREATE UNIQUE INDEX index_containers_on_uuid ON public.containers USING btree (uuid);
+--
+-- Name: index_frozen_groups_on_uuid; Type: INDEX; Schema: public; Owner: -
+--
+
+CREATE UNIQUE INDEX index_frozen_groups_on_uuid ON public.frozen_groups USING btree (uuid);
+
+
--
-- Name: index_groups_on_created_at; Type: INDEX; Schema: public; Owner: -
--
@@ -3149,6 +3183,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20210621204455'),
('20210816191509'),
('20211027154300'),
-('20220224203102');
+('20220224203102'),
+('20220301155729');
diff --git a/services/api/lib/20200501150153_permission_table_constants.rb b/services/api/lib/20200501150153_permission_table_constants.rb
index 74c15bc2e..7ee503936 100644
--- a/services/api/lib/20200501150153_permission_table_constants.rb
+++ b/services/api/lib/20200501150153_permission_table_constants.rb
@@ -15,8 +15,8 @@
# update_permissions reference that file instead.
PERMISSION_VIEW = "materialized_permissions"
-
TRASHED_GROUPS = "trashed_groups"
+FROZEN_GROUPS = "frozen_groups"
# We need to use this parameterized query in a few different places,
# including as a subquery in a larger query.
@@ -83,3 +83,21 @@ INSERT INTO materialized_permissions
}, "refresh_permission_view.do"
end
end
+
+def refresh_frozen
+ ActiveRecord::Base.transaction do
+ ActiveRecord::Base.connection.execute("LOCK TABLE #{FROZEN_GROUPS}")
+ ActiveRecord::Base.connection.execute("DELETE FROM #{FROZEN_GROUPS}")
+
+ # Compute entire frozen_groups table, starting with top-level
+ # projects (i.e., project groups owned by a user).
+ ActiveRecord::Base.connection.execute(%{
+INSERT INTO #{FROZEN_GROUPS}
+select ps.uuid from groups,
+ lateral project_subtree_with_is_frozen(groups.uuid, groups.frozen_by_uuid is not null) ps
+ where groups.owner_uuid like '_____-tpzed-_______________'
+ and group_class = 'project'
+ and ps.is_frozen
+})
+ end
+end
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index f3367118c..d71a0b5f9 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -319,7 +319,7 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
Rails.configuration.API.UnfreezeProjectRequiresAdmin = false
proj = Group.create!(group_class: 'project', name: 'freeze-test', owner_uuid: users(:active).uuid)
proj_inner = Group.create!(group_class: 'project', name: 'freeze-test-inner', owner_uuid: proj.uuid)
- coll = Collection.create!(name: 'foo', manifest_text: '')
+ coll = Collection.create!(name: 'foo', manifest_text: '', owner_uuid: proj_inner.uuid)
# Cannot set frozen_by_uuid to a different user
assert_raises do
@@ -357,35 +357,19 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
properties: {'frobity' => 'bar baz'})
assert ok, proj.errors.messages.inspect
- # Once project is frozen, cannot change name/contents, trash, or
- # delete the project or anything beneath it
- [proj, proj_inner, coll].each do |frozen|
- assert_raises do
- frozen.update_attributes!(name: 'foo2')
- end
- if frozen.is_a?(Collection)
- assert_raises do
- frozen.update_attributes!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo\n")
- end
- end
- assert_raises do
- frozen.update_attributes!(trash_at: db_current_time)
- end
- assert_raises do
- frozen.destroy
- end
- end
-
# Once project is frozen, cannot create new items inside it or
# its descendants
- [proj, proj_inner].each do |parent|
+ [proj, proj_inner].each do |frozen|
assert_raises do
- Collection.create!(owner_uuid: parent.uuid, name: 'inside-frozen-project')
+ collections(:collection_owned_by_active).update_attributes!(owner_uuid: frozen.uuid)
end
assert_raises do
- Group.create!(owner_uuid: parent.uuid, group_class: 'project', name: 'inside-frozen-project')
+ Collection.create!(owner_uuid: frozen.uuid, name: 'inside-frozen-project')
end
- cr = ContainerRequest.create(
+ assert_raises do
+ Group.create!(owner_uuid: frozen.uuid, group_class: 'project', name: 'inside-frozen-project')
+ end
+ cr = ContainerRequest.new(
command: ["echo", "foo"],
container_image: links(:docker_image_collection_tag).name,
cwd: "/tmp",
@@ -395,12 +379,39 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
runtime_constraints: {"vcpus" => 1, "ram" => 2},
name: "foo",
description: "bar",
- owner_uuid: parent.uuid,
+ owner_uuid: frozen.uuid,
)
- assert_raises do
- cr.save!
+ assert_raises ArvadosModel::PermissionDeniedError do
+ cr.save
+ end
+ assert_match /frozen/, cr.errors.inspect
+ # Check the frozen-parent condition is the only reason save failed.
+ cr.owner_uuid = users(:active).uuid
+ assert cr.save
+ cr.destroy
+ end
+
+ # Once project is frozen, cannot change name/contents, trash, or
+ # delete the project or anything beneath it
+ [proj, proj_inner, coll].each do |frozen|
+ assert_raises(StandardError, "should reject rename of #{frozen.uuid} with parent #{frozen.owner_uuid}") do
+ frozen.update_attributes!(name: 'foo2')
+ end
+ frozen.reload
+ if frozen.is_a?(Collection)
+ assert_raises(StandardError, "should reject manifest change of #{frozen.uuid}") do
+ frozen.update_attributes!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo\n")
+ end
+ frozen.reload
+ end
+ assert_raises(StandardError, "should reject setting trash_at of #{frozen.uuid}") do
+ frozen.update_attributes!(trash_at: db_current_time)
+ end
+ frozen.reload
+ assert_raises(StandardError, "should reject delete of #{frozen.uuid}") do
+ frozen.destroy
end
- assert_match cr.errors.inspect, /frozen/
+ frozen.reload
end
# User with manage permission can unfreeze, then create items
commit 50b2c892fe8acb04cde5df8b10033bdbdefc616a
Author: Tom Clegg <tom at curii.com>
Date: Mon Feb 28 11:32:39 2022 -0500
18691: Add groups.frozen_by_uuid attribute.
Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom at curii.com>
diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index 9800be704..656385cc1 100644
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -240,6 +240,18 @@ Clusters:
# https://doc.arvados.org/admin/metadata-vocabulary.html
VocabularyPath: ""
+ # If true, a project must have a non-empty description field in
+ # order to be frozen.
+ FreezeProjectRequiresDescription: false
+
+ # Project properties that must have non-empty values in order to
+ # freeze a project. Example: {"property_name": true}
+ FreezeProjectRequiresProperties: {}
+
+ # If true, only an admin user can un-freeze a project. If false,
+ # any user with "manage" permission can un-freeze.
+ UnfreezeProjectRequiresAdmin: false
+
Users:
# Config parameters to automatically setup new users. If enabled,
# this users will be able to self-activate. Enable this if you want
diff --git a/sdk/go/arvados/config.go b/sdk/go/arvados/config.go
index b8c8269f1..e0750bd8c 100644
--- a/sdk/go/arvados/config.go
+++ b/sdk/go/arvados/config.go
@@ -94,21 +94,24 @@ type Cluster struct {
PostgreSQL PostgreSQL
API struct {
- AsyncPermissionsUpdateInterval Duration
- DisabledAPIs StringSet
- MaxIndexDatabaseRead int
- MaxItemsPerResponse int
- MaxConcurrentRequests int
- MaxKeepBlobBuffers int
- MaxRequestAmplification int
- MaxRequestSize int
- MaxTokenLifetime Duration
- RequestTimeout Duration
- SendTimeout Duration
- WebsocketClientEventQueue int
- WebsocketServerEventQueue int
- KeepServiceRequestTimeout Duration
- VocabularyPath string
+ AsyncPermissionsUpdateInterval Duration
+ DisabledAPIs StringSet
+ MaxIndexDatabaseRead int
+ MaxItemsPerResponse int
+ MaxConcurrentRequests int
+ MaxKeepBlobBuffers int
+ MaxRequestAmplification int
+ MaxRequestSize int
+ MaxTokenLifetime Duration
+ RequestTimeout Duration
+ SendTimeout Duration
+ WebsocketClientEventQueue int
+ WebsocketServerEventQueue int
+ KeepServiceRequestTimeout Duration
+ VocabularyPath string
+ FreezeProjectRequiresDescription bool
+ FreezeProjectRequiresProperties StringSet
+ UnfreezeProjectRequiresAdmin bool
}
AuditLogs struct {
MaxAge Duration
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 8565b2a41..d6f698432 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -19,6 +19,7 @@ class Group < ArvadosModel
validate :ensure_filesystem_compatible_name
validate :check_group_class
validate :check_filter_group_filters
+ validate :check_frozen_state_change_allowed
before_create :assign_name
after_create :after_ownership_change
after_create :update_trash
@@ -40,6 +41,7 @@ class Group < ArvadosModel
t.add :trash_at
t.add :is_trashed
t.add :properties
+ t.add :frozen_by_uuid
end
def ensure_filesystem_compatible_name
@@ -92,6 +94,40 @@ class Group < ArvadosModel
end
end
+ def check_frozen_state_change_allowed
+ if frozen_by_uuid == ""
+ self.frozen_by_uuid = nil
+ end
+ if frozen_by_uuid_changed? || (new_record? && frozen_by_uuid)
+ if group_class != "project"
+ errors.add(:frozen_by_uuid, "cannot be modified on a non-project group")
+ return
+ end
+ if frozen_by_uuid_was && Rails.configuration.API.UnfreezeProjectRequiresAdmin && !current_user.is_admin
+ errors.add(:frozen_by_uuid, "can only be changed by an admin user, once set")
+ return
+ end
+ if frozen_by_uuid && frozen_by_uuid != current_user.uuid
+ errors.add(:frozen_by_uuid, "can only be set to the current user's UUID")
+ return
+ end
+ if !new_record? && !current_user.can?(manage: uuid)
+ raise PermissionDeniedError
+ end
+ if frozen_by_uuid_was.nil?
+ if Rails.configuration.API.FreezeProjectRequiresDescription && !attribute_present?(:description)
+ errors.add(:frozen_by_uuid, "can only be set if description is non-empty")
+ end
+ Rails.configuration.API.FreezeProjectRequiresProperties.andand.each do |key, _|
+ key = key.to_s
+ if !properties[key] || properties[key] == ""
+ errors.add(:frozen_by_uuid, "can only be set if properties[#{key}] value is non-empty")
+ end
+ end
+ end
+ end
+ end
+
def update_trash
if saved_change_to_trash_at? or saved_change_to_owner_uuid?
# The group was added or removed from the trash.
diff --git a/services/api/db/migrate/20220224203102_add_frozen_by_uuid_to_groups.rb b/services/api/db/migrate/20220224203102_add_frozen_by_uuid_to_groups.rb
new file mode 100644
index 000000000..d730b2535
--- /dev/null
+++ b/services/api/db/migrate/20220224203102_add_frozen_by_uuid_to_groups.rb
@@ -0,0 +1,9 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+class AddFrozenByUuidToGroups < ActiveRecord::Migration[5.2]
+ def change
+ add_column :groups, :frozen_by_uuid, :string
+ end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index da9959593..573a4375c 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -567,7 +567,8 @@ CREATE TABLE public.groups (
trash_at timestamp without time zone,
is_trashed boolean DEFAULT false NOT NULL,
delete_at timestamp without time zone,
- properties jsonb DEFAULT '{}'::jsonb
+ properties jsonb DEFAULT '{}'::jsonb,
+ frozen_by_uuid character varying
);
@@ -3147,6 +3148,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20210126183521'),
('20210621204455'),
('20210816191509'),
-('20211027154300');
+('20211027154300'),
+('20220224203102');
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 10932e116..f3367118c 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -313,4 +313,124 @@ update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
assert Link.where(tail_uuid: g3, head_uuid: g4, link_class: "permission", name: "can_manage").any?
assert !Link.where(link_class: 'permission', name: 'can_manage', tail_uuid: g5, head_uuid: g4).any?
end
+
+ test "freeze project" do
+ act_as_user users(:active) do
+ Rails.configuration.API.UnfreezeProjectRequiresAdmin = false
+ proj = Group.create!(group_class: 'project', name: 'freeze-test', owner_uuid: users(:active).uuid)
+ proj_inner = Group.create!(group_class: 'project', name: 'freeze-test-inner', owner_uuid: proj.uuid)
+ coll = Collection.create!(name: 'foo', manifest_text: '')
+
+ # Cannot set frozen_by_uuid to a different user
+ assert_raises do
+ proj.update_attributes!(frozen_by_uuid: users(:spectator).uuid)
+ end
+ proj.reload
+
+ # Cannot set frozen_by_uuid without description (if so configured)
+ Rails.configuration.API.FreezeProjectRequiresDescription = true
+ err = assert_raises do
+ proj.update_attributes!(frozen_by_uuid: users(:active).uuid)
+ end
+ assert_match /can only be set if description is non-empty/, err.inspect
+ proj.reload
+ err = assert_raises do
+ proj.update_attributes!(frozen_by_uuid: users(:active).uuid, description: '')
+ end
+ assert_match /can only be set if description is non-empty/, err.inspect
+ proj.reload
+
+ # Cannot set frozen_by_uuid without properties (if so configured)
+ Rails.configuration.API.FreezeProjectRequiresProperties['frobity'] = true
+ err = assert_raises do
+ proj.update_attributes!(
+ frozen_by_uuid: users(:active).uuid,
+ description: 'ready to freeze')
+ end
+ assert_match /can only be set if properties\[frobity\] value is non-empty/, err.inspect
+ proj.reload
+
+ # Can set frozen_by_uuid if all conditions are met
+ ok = proj.update_attributes(
+ frozen_by_uuid: users(:active).uuid,
+ description: 'ready to freeze',
+ properties: {'frobity' => 'bar baz'})
+ assert ok, proj.errors.messages.inspect
+
+ # Once project is frozen, cannot change name/contents, trash, or
+ # delete the project or anything beneath it
+ [proj, proj_inner, coll].each do |frozen|
+ assert_raises do
+ frozen.update_attributes!(name: 'foo2')
+ end
+ if frozen.is_a?(Collection)
+ assert_raises do
+ frozen.update_attributes!(manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:foo\n")
+ end
+ end
+ assert_raises do
+ frozen.update_attributes!(trash_at: db_current_time)
+ end
+ assert_raises do
+ frozen.destroy
+ end
+ end
+
+ # Once project is frozen, cannot create new items inside it or
+ # its descendants
+ [proj, proj_inner].each do |parent|
+ assert_raises do
+ Collection.create!(owner_uuid: parent.uuid, name: 'inside-frozen-project')
+ end
+ assert_raises do
+ Group.create!(owner_uuid: parent.uuid, group_class: 'project', name: 'inside-frozen-project')
+ end
+ cr = ContainerRequest.create(
+ command: ["echo", "foo"],
+ container_image: links(:docker_image_collection_tag).name,
+ cwd: "/tmp",
+ environment: {},
+ mounts: {"/out" => {"kind" => "tmp", "capacity" => 1000000}},
+ output_path: "/out",
+ runtime_constraints: {"vcpus" => 1, "ram" => 2},
+ name: "foo",
+ description: "bar",
+ owner_uuid: parent.uuid,
+ )
+ assert_raises do
+ cr.save!
+ end
+ assert_match cr.errors.inspect, /frozen/
+ end
+
+ # User with manage permission can unfreeze, then create items
+ # inside it and its children
+ assert proj.update_attributes(frozen_by_uuid: nil)
+ assert Collection.create!(owner_uuid: proj.uuid, name: 'inside-unfrozen-project')
+ assert Collection.create!(owner_uuid: proj_inner.uuid, name: 'inside-inner-unfrozen-project')
+
+ # Re-freeze, and reconfigure so only admins can unfreeze.
+ assert proj.update_attributes(frozen_by_uuid: users(:active).uuid)
+ Rails.configuration.API.UnfreezeProjectRequiresAdmin = true
+
+ # Owner cannot unfreeze, because not admin.
+ err = assert_raises do
+ proj.update_attributes!(frozen_by_uuid: nil)
+ end
+ assert_match /can only be changed by an admin user, once set/, err.inspect
+ proj.reload
+
+ act_as_user users(:admin) do
+ # Even admin cannot change frozen_by_uuid to someone else's UUID.
+ err = assert_raises do
+ proj.update_attributes!(frozen_by_uuid: users(:project_viewer).uuid)
+ end
+ assert_match /can only be set to the current user's UUID/, err.inspect
+ proj.reload
+
+ # Admin can unfreeze.
+ assert proj.update_attributes(frozen_by_uuid: nil), proj.errors.messages
+ end
+ end
+ end
end
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list