[ARVADOS] updated: 2.0.3-17-g95594ca88
Git user
git at public.arvados.org
Fri Aug 14 18:25:19 UTC 2020
Summary of changes:
.../app/views/application/_show_sharing.html.erb | 2 +
lib/cloud/azure/azure.go | 8 ++-
lib/config/config.default.yml | 18 ++++-
lib/config/generated_config.go | 18 ++++-
lib/controller/federation/generated.go | 3 +
lib/controller/federation/list.go | 5 +-
sdk/go/arvados/api.go | 1 +
sdk/python/arvados/commands/federation_migrate.py | 81 +++++++++++++---------
services/api/app/models/arvados_model.rb | 25 ++++---
services/api/app/models/link.rb | 27 ++++++++
services/api/test/unit/link_test.rb | 8 +++
11 files changed, 149 insertions(+), 47 deletions(-)
via 95594ca8821d6c8f7e13fa788b4dc160fc3bbb53 (commit)
via 35f0b6522e32e4f7601760efaca3e900ca918072 (commit)
via 0cc5bd9af9f936c2eeffb31c4f417b0f43eed036 (commit)
via 93480ec377d8d78352cb900e9d894593daa4b3ef (commit)
via 65803831b766cf0abf8286937a78a8fe05419d3c (commit)
via 00964013f61975cd03ce2aed810c0543402ca707 (commit)
from 6d644fd3475576fe3648315c62b5003365add9da (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit 95594ca8821d6c8f7e13fa788b4dc160fc3bbb53
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Thu Aug 13 15:22:49 2020 -0400
16683: Check that remote cluster id is presumed valid, add test
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index c580d63b0..bf107f575 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -47,6 +47,7 @@ class Link < ArvadosModel
!attr_value.nil? &&
self.link_class == 'permission' &&
attr_value[0..4] != Rails.configuration.ClusterID &&
+ ApiClientAuthorization.remote_host(uuid_prefix: attr_value[0..4]) &&
ArvadosModel::resource_class_for_uuid(attr_value) == User
# Permission link tail is a remote user (the user permissions
# are being granted to), so bypass the standard check that a
diff --git a/services/api/test/unit/link_test.rb b/services/api/test/unit/link_test.rb
index 00f3cc291..c7d21bdc4 100644
--- a/services/api/test/unit/link_test.rb
+++ b/services/api/test/unit/link_test.rb
@@ -58,6 +58,14 @@ class LinkTest < ActiveSupport::TestCase
users(:active).uuid.sub(/-\w+$/, "-#{'z' * 15}"))
end
+ test "link granting permission to remote user is valid" do
+ refute new_active_link_valid?(tail_uuid:
+ users(:active).uuid.sub(/^\w+-/, "foooo-"))
+ Rails.configuration.RemoteClusters = Rails.configuration.RemoteClusters.merge({foooo: ActiveSupport::InheritableOptions.new({Host: "bar.com"})})
+ assert new_active_link_valid?(tail_uuid:
+ users(:active).uuid.sub(/^\w+-/, "foooo-"))
+ end
+
test "link granting non-project permission to unreadable user is invalid" do
refute new_active_link_valid?(tail_uuid: users(:admin).uuid,
head_uuid: collections(:bar_file).uuid)
commit 35f0b6522e32e4f7601760efaca3e900ca918072
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Wed Aug 12 17:32:06 2020 -0400
16683: Permit granting permissions to remote users
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/sdk/go/arvados/api.go b/sdk/go/arvados/api.go
index ec04cecec..4695a9504 100644
--- a/sdk/go/arvados/api.go
+++ b/sdk/go/arvados/api.go
@@ -85,7 +85,7 @@ type ListOptions struct {
IncludeTrash bool `json:"include_trash"`
IncludeOldVersions bool `json:"include_old_versions"`
BypassFederation bool `json:"bypass_federation"`
- ForwardedFor string `json:"forwarded_for,omitempty"`
+ ForwardedFor string `json:"forwarded_for,omitempty"`
}
type CreateOptions struct {
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 816dbf475..7f6ab462d 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -716,6 +716,20 @@ class ArvadosModel < ApplicationRecord
%r/[a-z0-9]{5}-#{uuid_prefix}-[a-z0-9]{15}/
end
+ def check_readable_uuid attr, attr_value
+ return if attr_value.nil?
+ if (r = ArvadosModel::resource_class_for_uuid attr_value)
+ unless skip_uuid_read_permission_check.include? attr
+ r = r.readable_by(current_user)
+ end
+ if r.where(uuid: attr_value).count == 0
+ errors.add(attr, "'#{attr_value}' not found")
+ end
+ else
+ # Not a valid uuid or PDH, but that (currently) is not an error.
+ end
+ end
+
def ensure_valid_uuids
specials = [system_user_uuid]
@@ -724,16 +738,7 @@ class ArvadosModel < ApplicationRecord
next if skip_uuid_existence_check.include? attr
attr_value = send attr
next if specials.include? attr_value
- if attr_value
- if (r = ArvadosModel::resource_class_for_uuid attr_value)
- unless skip_uuid_read_permission_check.include? attr
- r = r.readable_by(current_user)
- end
- if r.where(uuid: attr_value).count == 0
- errors.add(attr, "'#{attr_value}' not found")
- end
- end
- end
+ check_readable_uuid attr, attr_value
end
end
end
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index ad7800fe6..c580d63b0 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -42,6 +42,27 @@ class Link < ArvadosModel
protected
+ def check_readable_uuid attr, attr_value
+ if attr == 'tail_uuid' &&
+ !attr_value.nil? &&
+ self.link_class == 'permission' &&
+ attr_value[0..4] != Rails.configuration.ClusterID &&
+ ArvadosModel::resource_class_for_uuid(attr_value) == User
+ # Permission link tail is a remote user (the user permissions
+ # are being granted to), so bypass the standard check that a
+ # referenced object uuid is readable by current user.
+ #
+ # We could do a call to the remote cluster to check if the user
+ # in tail_uuid exists. This would detect copy-and-paste errors,
+ # but add another way for the request to fail, and I don't think
+ # it would improve security. It doesn't seem to be worth the
+ # complexity tradeoff.
+ true
+ else
+ super
+ end
+ end
+
def permission_to_attach_to_objects
# Anonymous users cannot write links
return false if !current_user
@@ -54,6 +75,11 @@ class Link < ArvadosModel
head_obj = ArvadosModel.find_by_uuid(head_uuid)
+ if head_obj.nil?
+ errors.add(:head_uuid, "does not exist")
+ return false
+ end
+
# No permission links can be pointed to past collection versions
return false if head_obj.is_a?(Collection) && head_obj.current_version_uuid != head_uuid
commit 0cc5bd9af9f936c2eeffb31c4f417b0f43eed036
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Wed Aug 12 16:20:19 2020 -0400
16683: Sharing dialog query is compatible with federated user list
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/apps/workbench/app/views/application/_show_sharing.html.erb b/apps/workbench/app/views/application/_show_sharing.html.erb
index 7877e60d3..75773ab90 100644
--- a/apps/workbench/app/views/application/_show_sharing.html.erb
+++ b/apps/workbench/app/views/application/_show_sharing.html.erb
@@ -8,6 +8,8 @@ SPDX-License-Identifier: AGPL-3.0 %>
[User, Group].each do |type|
type
.filter([['uuid','in', at share_links.collect(&:tail_uuid)]])
+ .with_count("none")
+ .fetch_multiple_pages(false)
.each do |o|
uuid_map[o.uuid] = o
end
commit 93480ec377d8d78352cb900e9d894593daa4b3ef
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Wed Aug 12 16:10:32 2020 -0400
16683: Set and check ForwardedFor on federated list requests
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/lib/controller/federation/generated.go b/lib/controller/federation/generated.go
index 20edd90b9..8745f3b97 100755
--- a/lib/controller/federation/generated.go
+++ b/lib/controller/federation/generated.go
@@ -23,6 +23,7 @@ func (conn *Conn) generated_ContainerList(ctx context.Context, options arvados.L
var needSort atomic.Value
needSort.Store(false)
err := conn.splitListRequest(ctx, options, func(ctx context.Context, _ string, backend arvados.API, options arvados.ListOptions) ([]string, error) {
+ options.ForwardedFor = conn.cluster.ClusterID + "-" + options.ForwardedFor
cl, err := backend.ContainerList(ctx, options)
if err != nil {
return nil, err
@@ -63,6 +64,7 @@ func (conn *Conn) generated_SpecimenList(ctx context.Context, options arvados.Li
var needSort atomic.Value
needSort.Store(false)
err := conn.splitListRequest(ctx, options, func(ctx context.Context, _ string, backend arvados.API, options arvados.ListOptions) ([]string, error) {
+ options.ForwardedFor = conn.cluster.ClusterID + "-" + options.ForwardedFor
cl, err := backend.SpecimenList(ctx, options)
if err != nil {
return nil, err
@@ -103,6 +105,7 @@ func (conn *Conn) generated_UserList(ctx context.Context, options arvados.ListOp
var needSort atomic.Value
needSort.Store(false)
err := conn.splitListRequest(ctx, options, func(ctx context.Context, _ string, backend arvados.API, options arvados.ListOptions) ([]string, error) {
+ options.ForwardedFor = conn.cluster.ClusterID + "-" + options.ForwardedFor
cl, err := backend.UserList(ctx, options)
if err != nil {
return nil, err
diff --git a/lib/controller/federation/list.go b/lib/controller/federation/list.go
index 0a596eb9c..bc6d3e00a 100644
--- a/lib/controller/federation/list.go
+++ b/lib/controller/federation/list.go
@@ -27,6 +27,7 @@ func (conn *Conn) generated_CollectionList(ctx context.Context, options arvados.
var needSort atomic.Value
needSort.Store(false)
err := conn.splitListRequest(ctx, options, func(ctx context.Context, _ string, backend arvados.API, options arvados.ListOptions) ([]string, error) {
+ options.ForwardedFor = conn.cluster.ClusterID + "-" + options.ForwardedFor
cl, err := backend.CollectionList(ctx, options)
if err != nil {
return nil, err
@@ -107,7 +108,7 @@ func (conn *Conn) generated_CollectionList(ctx context.Context, options arvados.
// backend.
func (conn *Conn) splitListRequest(ctx context.Context, opts arvados.ListOptions, fn func(context.Context, string, arvados.API, arvados.ListOptions) ([]string, error)) error {
- if opts.BypassFederation {
+ if opts.BypassFederation || opts.ForwardedFor != "" {
// Client requested no federation. Pass through.
_, err := fn(ctx, conn.cluster.ClusterID, conn.local, opts)
return err
@@ -249,7 +250,7 @@ func (conn *Conn) splitListRequest(ctx context.Context, opts arvados.ListOptions
done, err := fn(ctx, clusterID, backend, remoteOpts)
if err != nil {
- errs <- httpErrorf(http.StatusBadGateway, err.Error())
+ errs <- httpErrorf(http.StatusBadGateway, "%s", err.Error())
return
}
progress := false
diff --git a/sdk/go/arvados/api.go b/sdk/go/arvados/api.go
index e60108335..ec04cecec 100644
--- a/sdk/go/arvados/api.go
+++ b/sdk/go/arvados/api.go
@@ -85,6 +85,7 @@ type ListOptions struct {
IncludeTrash bool `json:"include_trash"`
IncludeOldVersions bool `json:"include_old_versions"`
BypassFederation bool `json:"bypass_federation"`
+ ForwardedFor string `json:"forwarded_for,omitempty"`
}
type CreateOptions struct {
commit 65803831b766cf0abf8286937a78a8fe05419d3c
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Thu Aug 13 17:21:35 2020 -0400
Merge federation-migrate fixes refs #16589
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/sdk/python/arvados/commands/federation_migrate.py b/sdk/python/arvados/commands/federation_migrate.py
index 445775cce..5c1bb29e7 100755
--- a/sdk/python/arvados/commands/federation_migrate.py
+++ b/sdk/python/arvados/commands/federation_migrate.py
@@ -22,6 +22,7 @@ import hmac
import urllib.parse
import os
import hashlib
+import re
from arvados._version import __version__
EMAIL=0
@@ -169,19 +170,20 @@ def read_migrations(args, by_email, by_username):
def update_username(args, email, user_uuid, username, migratecluster, migratearv):
print("(%s) Updating username of %s to '%s' on %s" % (email, user_uuid, username, migratecluster))
- if not args.dry_run:
- try:
- conflicts = migratearv.users().list(filters=[["username", "=", username]], bypass_federation=True).execute()
- if conflicts["items"]:
- # There's already a user with the username, move the old user out of the way
- migratearv.users().update(uuid=conflicts["items"][0]["uuid"],
- bypass_federation=True,
- body={"user": {"username": username+"migrate"}}).execute()
- migratearv.users().update(uuid=user_uuid,
- bypass_federation=True,
- body={"user": {"username": username}}).execute()
- except arvados.errors.ApiError as e:
- print("(%s) Error updating username of %s to '%s' on %s: %s" % (email, user_uuid, username, migratecluster, e))
+ if args.dry_run:
+ return
+ try:
+ conflicts = migratearv.users().list(filters=[["username", "=", username]], bypass_federation=True).execute()
+ if conflicts["items"]:
+ # There's already a user with the username, move the old user out of the way
+ migratearv.users().update(uuid=conflicts["items"][0]["uuid"],
+ bypass_federation=True,
+ body={"user": {"username": username+"migrate"}}).execute()
+ migratearv.users().update(uuid=user_uuid,
+ bypass_federation=True,
+ body={"user": {"username": username}}).execute()
+ except arvados.errors.ApiError as e:
+ print("(%s) Error updating username of %s to '%s' on %s: %s" % (email, user_uuid, username, migratecluster, e))
def choose_new_user(args, by_email, email, userhome, username, old_user_uuid, clusters):
@@ -212,11 +214,17 @@ def choose_new_user(args, by_email, email, userhome, username, old_user_uuid, cl
conflicts = homearv.users().list(filters=[["username", "=", username]],
bypass_federation=True).execute()
if conflicts["items"]:
- homearv.users().update(uuid=conflicts["items"][0]["uuid"],
- bypass_federation=True,
- body={"user": {"username": username+"migrate"}}).execute()
- user = homearv.users().create(body={"user": {"email": email, "username": username,
- "is_active": olduser["is_active"]}}).execute()
+ homearv.users().update(
+ uuid=conflicts["items"][0]["uuid"],
+ bypass_federation=True,
+ body={"user": {"username": username+"migrate"}}).execute()
+ user = homearv.users().create(
+ body={"user": {
+ "email": email,
+ "first_name": olduser["first_name"],
+ "last_name": olduser["last_name"],
+ "username": username,
+ "is_active": olduser["is_active"]}}).execute()
except arvados.errors.ApiError as e:
print("(%s) Could not create user: %s" % (email, str(e)))
return None
@@ -271,7 +279,7 @@ def activate_remote_user(args, email, homearv, migratearv, old_user_uuid, new_us
newuser = arvados.api(host=ru.netloc, token=salted,
insecure=os.environ.get("ARVADOS_API_HOST_INSECURE")).users().current().execute()
else:
- newuser = {"is_active": True, "username": username}
+ newuser = {"is_active": True, "username": email.split('@')[0], "is_admin": False}
except arvados.errors.ApiError as e:
print("(%s) Error getting user info for %s from %s: %s" % (email, new_user_uuid, migratecluster, e))
return None
@@ -287,39 +295,48 @@ def activate_remote_user(args, email, homearv, migratearv, old_user_uuid, new_us
return None
if olduser["is_admin"] and not newuser["is_admin"]:
- print("(%s) Not migrating %s because user is admin but target user %s is not admin on %s" % (email, old_user_uuid, new_user_uuid, migratecluster))
+ print("(%s) Not migrating %s because user is admin but target user %s is not admin on %s. Please ensure the user admin status is the same on both clusters. Note that a federated admin account has admin privileges on the entire federation." % (email, old_user_uuid, new_user_uuid, migratecluster))
return None
return newuser
def migrate_user(args, migratearv, email, new_user_uuid, old_user_uuid):
+ if args.dry_run:
+ return
try:
- if not args.dry_run:
+ new_owner_uuid = new_user_uuid
+ if args.data_into_subproject:
grp = migratearv.groups().create(body={
"owner_uuid": new_user_uuid,
"name": "Migrated from %s (%s)" % (email, old_user_uuid),
"group_class": "project"
}, ensure_unique_name=True).execute()
- migratearv.users().merge(old_user_uuid=old_user_uuid,
- new_user_uuid=new_user_uuid,
- new_owner_uuid=grp["uuid"],
- redirect_to_new_user=True).execute()
+ new_owner_uuid = grp["uuid"]
+ migratearv.users().merge(old_user_uuid=old_user_uuid,
+ new_user_uuid=new_user_uuid,
+ new_owner_uuid=new_owner_uuid,
+ redirect_to_new_user=True).execute()
except arvados.errors.ApiError as e:
- print("(%s) Error migrating user: %s" % (email, e))
+ name_collision = re.search(r'Key \(owner_uuid, name\)=\((.*?), (.*?)\) already exists\.\n.*UPDATE "(.*?)"', e._get_reason())
+ if name_collision:
+ target_owner, rsc_name, rsc_type = name_collision.groups()
+ print("(%s) Cannot migrate to %s because both origin and target users have a %s named '%s'. Please rename the conflicting items or use --data-into-subproject to migrate all users' data into a special subproject." % (email, target_owner, rsc_type[:-1], rsc_name))
+ else:
+ print("(%s) Skipping user migration because of error: %s" % (email, e))
def main():
-
parser = argparse.ArgumentParser(description='Migrate users to federated identity, see https://doc.arvados.org/admin/merge-remote-account.html')
parser.add_argument(
'--version', action='version', version="%s %s" % (sys.argv[0], __version__),
help='Print version and exit.')
- parser.add_argument('--tokens', type=str, required=False)
+ parser.add_argument('--tokens', type=str, metavar='FILE', required=False, help="Read tokens from FILE. Not needed when using LoginCluster.")
+ parser.add_argument('--data-into-subproject', action="store_true", help="Migrate user's data into a separate subproject. This can be used to avoid name collisions from within an account.")
group = parser.add_mutually_exclusive_group(required=True)
- group.add_argument('--report', type=str, help="Generate report .csv file listing users by email address and their associated Arvados accounts")
- group.add_argument('--migrate', type=str, help="Consume report .csv and migrate users to designated Arvados accounts")
- group.add_argument('--dry-run', type=str, help="Consume report .csv and report how user would be migrated to designated Arvados accounts")
- group.add_argument('--check', action="store_true", help="Check that tokens are usable and the federation is well connected")
+ group.add_argument('--report', type=str, metavar='FILE', help="Generate report .csv file listing users by email address and their associated Arvados accounts.")
+ group.add_argument('--migrate', type=str, metavar='FILE', help="Consume report .csv and migrate users to designated Arvados accounts.")
+ group.add_argument('--dry-run', type=str, metavar='FILE', help="Consume report .csv and report how user would be migrated to designated Arvados accounts.")
+ group.add_argument('--check', action="store_true", help="Check that tokens are usable and the federation is well connected.")
args = parser.parse_args()
clusters, errors, loginCluster = connect_clusters(args)
commit 00964013f61975cd03ce2aed810c0543402ca707
Author: Peter Amstutz <peter.amstutz at curii.com>
Date: Wed Jul 22 14:58:59 2020 -0400
16623: Add NetworkResourceGroup option for Azure
refs #16623
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>
diff --git a/lib/cloud/azure/azure.go b/lib/cloud/azure/azure.go
index 752de152d..6de367aa2 100644
--- a/lib/cloud/azure/azure.go
+++ b/lib/cloud/azure/azure.go
@@ -43,6 +43,7 @@ type azureInstanceSetConfig struct {
ResourceGroup string
Location string
Network string
+ NetworkResourceGroup string
Subnet string
StorageAccount string
BlobContainer string
@@ -356,6 +357,11 @@ func (az *azureInstanceSet) Create(
}
tags["created-at"] = to.StringPtr(time.Now().Format(time.RFC3339Nano))
+ networkResourceGroup := az.azconfig.NetworkResourceGroup
+ if networkResourceGroup == "" {
+ networkResourceGroup = az.azconfig.ResourceGroup
+ }
+
nicParameters := network.Interface{
Location: &az.azconfig.Location,
Tags: tags,
@@ -368,7 +374,7 @@ func (az *azureInstanceSet) Create(
ID: to.StringPtr(fmt.Sprintf("/subscriptions/%s/resourceGroups/%s/providers"+
"/Microsoft.Network/virtualnetworks/%s/subnets/%s",
az.azconfig.SubscriptionID,
- az.azconfig.ResourceGroup,
+ networkResourceGroup,
az.azconfig.Network,
az.azconfig.Subnet)),
},
diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index a0def71f7..08df3acb4 100644
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -869,13 +869,29 @@ Clusters:
# (azure) Instance configuration.
CloudEnvironment: AzurePublicCloud
- ResourceGroup: ""
Location: centralus
+
+ # (azure) The resource group where the VM and virtual NIC will be
+ # created.
+ ResourceGroup: ""
+
+ # (azure) The resource group of the Network to use for the virtual
+ # NIC (if different from ResourceGroup)
+ NetworkResourceGroup: ""
Network: ""
Subnet: ""
+
+ # (azure) Where to store the VM VHD blobs
StorageAccount: ""
BlobContainer: ""
+
+ # (azure) How long to wait before deleting VHD and NIC
+ # objects that are no longer being used.
DeleteDanglingResourcesAfter: 20s
+
+ # Account (that already exists in the VM image) that will be
+ # set up with an ssh authorized key to allow the compute
+ # dispatcher to connect.
AdminUsername: arvados
InstanceTypes:
diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go
index 7b7c3c20d..c99a23b58 100644
--- a/lib/config/generated_config.go
+++ b/lib/config/generated_config.go
@@ -875,13 +875,29 @@ Clusters:
# (azure) Instance configuration.
CloudEnvironment: AzurePublicCloud
- ResourceGroup: ""
Location: centralus
+
+ # (azure) The resource group where the VM and virtual NIC will be
+ # created.
+ ResourceGroup: ""
+
+ # (azure) The resource group of the Network to use for the virtual
+ # NIC (if different from ResourceGroup)
+ NetworkResourceGroup: ""
Network: ""
Subnet: ""
+
+ # (azure) Where to store the VM VHD blobs
StorageAccount: ""
BlobContainer: ""
+
+ # (azure) How long to wait before deleting VHD and NIC
+ # objects that are no longer being used.
DeleteDanglingResourcesAfter: 20s
+
+ # Account (that already exists in the VM image) that will be
+ # set up with an ssh authorized key to allow the compute
+ # dispatcher to connect.
AdminUsername: arvados
InstanceTypes:
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list