rawlinp commented on code in PR #6761:
URL: https://github.com/apache/trafficcontrol/pull/6761#discussion_r856561064
##########
traffic_ops/app/db/migrations/2022031622271100_add_user_sharing_cdn_locks.down.sql:
##########
@@ -0,0 +1,18 @@
+/*
Review Comment:
The dates in the names of these DB migration files need to be updated to be
later than ones that already exist in master
##########
traffic_ops/testing/api/v4/cdn_locks_test.go:
##########
@@ -57,6 +57,26 @@ func TestCDNLocks(t *testing.T) {
Expectations:
utils.CkRequest(utils.NoError(), utils.HasStatus(http.StatusCreated),
validateCreateResponseFields(map[string]interface{}{"username": "admin", "cdn":
"cdn3", "message": "snapping cdn", "soft": true})),
},
+ "CREATED when INVALID shared username": {
Review Comment:
should this say "NOT CREATED"?
##########
traffic_ops/app/db/migrations/2022031622271100_add_user_sharing_cdn_locks.up.sql:
##########
@@ -0,0 +1,18 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with this
+ * work for additional information regarding copyright ownership. The ASF
+ * licenses this file to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
under
+ * the License.
+ */
+
+ALTER TABLE public.cdn_lock ADD COLUMN shared_usernames text[] DEFAULT NULL;
Review Comment:
Generally we should try to avoid using array-based columns -- we should add
a new table like `cdn_lock_user` which contains the username of the lock owner,
the username of the shared owner, and cdn name (with the combination of all
three being the primary key). The username of the lock owner and CDN name
should be FK references to `cdn_lock.username` and `cdn_lock.cdn` (with `ON
DELETE CASCADE`), and the username of the shared owner should be a FK reference
to a row in the `tm_user.username` table.
##########
traffic_ops/traffic_ops_golang/cdn_lock/cdn_lock.go:
##########
@@ -32,12 +32,32 @@ import (
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/auth"
"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/dbhelpers"
+
+ "github.com/lib/pq"
)
-const readQuery = `SELECT username, cdn, message, soft, last_updated FROM
cdn_lock`
-const insertQuery = `INSERT INTO cdn_lock (username, cdn, message, soft)
VALUES (:username, :cdn, :message, :soft) RETURNING username, cdn, message,
soft, last_updated`
-const deleteQuery = `DELETE FROM cdn_lock WHERE cdn=$1 AND username=$2
RETURNING username, cdn, message, soft, last_updated`
-const deleteAdminQuery = `DELETE FROM cdn_lock WHERE cdn=$1 RETURNING
username, cdn, message, soft, last_updated`
+const readQuery = `SELECT username, cdn, message, soft, shared_usernames,
last_updated FROM cdn_lock`
+const insertQuery = `INSERT INTO cdn_lock (username, cdn, message, soft,
shared_usernames) VALUES ($1, $2, $3, $4, $5) RETURNING username, cdn, message,
soft, shared_usernames, last_updated`
+const deleteQuery = `DELETE FROM cdn_lock WHERE cdn=$1 AND username=$2
RETURNING username, cdn, message, soft, shared_usernames, last_updated`
+const deleteAdminQuery = `DELETE FROM cdn_lock WHERE cdn=$1 RETURNING
username, cdn, message, soft, shared_usernames, last_updated`
+const checkSharedUsersValidityQuery = `select count(*) from tm_user u join
role r on r.id = u.role join role_capability rc on rc.role_id = r.id where
u.tenant_id in (WITH RECURSIVE
+user_tenant_id as (select (select tenant_id from tm_user where
username=$1)::bigint as v),
Review Comment:
Do we really need to check tenancy here? If someone has the ability to lock
the CDN, shouldn't we be able to share the lock with them even if they're in a
different or ancestral tenant?
##########
traffic_portal/app/src/common/modules/dialog/select/lock/dialog.select.lock.tpl.html:
##########
@@ -39,6 +39,17 @@ <h4 class="modal-title">Create CDN Lock</h4>
</div>
</div>
</div>
+ <div class="row">
+ <div class="col-sm-12 col-md-12">
+ <div class="form-group">
+ <label class="control-label"
for="sharedUserNames">Usernames to share lock with</label>
+ <input id="sharedUserNames" name="sharedUserNames"
type="text" class="form-control" ng-list ng-model="lock.sharedUserNames"
maxlength="256">
+ <div class="helptext">
+ A comma separated list of usernames that you want your
lock to be shared with.
Review Comment:
Comma-separated lists are kind of clunky -- it would be nice to mirror the
list functionality of adding Consistent Hash Query Params in the DS form, where
you can add/remove to the list by using buttons. We could even get the list of
active users from the TO API and pre-populate the list of valid usernames to
choose from.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
