[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4942: Add Traffic Ops If-Match and If-Unmodified-Since Support in Server and Clients

[GitBox <gi...@apache.org>]

rob05c commented on a change in pull request #4942:
URL: https://github.com/apache/trafficcontrol/pull/4942#discussion_r468289446



##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -383,6 +401,20 @@ func (req *deliveryServiceRequestAssignment) Update() (error, error, int) {
 	req.DeliveryServiceRequestNullable = current.DeliveryServiceRequestNullable
 	req.AssigneeID = assigneeID
 
+	err, existingLastUpdated := CheckIfExistsBeforeUpdate(req.ReqInfo.Tx, *req.ID)

Review comment:
       This whole block seems to be identical in a lot of places. Could it be abstracted into something like `func IsUnmodified(tx *sql.Tx, id int, tableName string, hdr http.Header) bool` ?

##########
File path: traffic_ops/traffic_ops_golang/user/user.go
##########
@@ -266,6 +286,20 @@ func (user *TOUser) Update() (error, error, int) {
 		}
 	}
 
+	err, existingLastUpdated := CheckIfExistsBeforeUpdate(user.ReqInfo.Tx, *user.ID)
+	if err != nil {
+		return errors.New("no server found with this id"), nil, http.StatusNotFound
+	}
+	_, iumsTime := rfc.GetETagOrIfUnmodifiedSinceTime(h)
+	existingEtag := rfc.ETag(existingLastUpdated.Time)
+
+	if h.Get(rfc.IfMatch) != "" && !strings.Contains(h.Get(rfc.IfMatch), existingEtag) {
+		return errors.New("header preconditions dont match"), nil, http.StatusPreconditionFailed
+	}
+	if iumsTime != nil && existingLastUpdated.UTC().After(iumsTime.UTC()) {

Review comment:
       If the header was an `If-Unmodified-Since`, the `HTTP-Date` only has second-precison. Which means, if we simply round or truncate when setting `Last-Modified`, `If-Modified-Since` will always be true, and `If-Unmodified-Since` will always be false.
   
   But ETag has as much precision as we want to give it, and we definitely want to keep that precision in the check.
   
   It could be changed in your code here, to add a second if it's exactly rounded to the second (and therefore an `IUS` not `IM`).
   
   But I think the better solution is to leave your code here as it is, and fix the place(s) that set `LastModified` to use `lastModified.Truncate(time.Second).Add(time.Second)`

##########
File path: traffic_ops/traffic_ops_golang/origin/origins.go
##########
@@ -309,6 +309,7 @@ func (origin *TOOrigin) Update() (error, error, int) {
 	}
 
 	log.Debugf("about to run exec query: %s with origin: %++v", updateQuery(), origin)
+	// ToDo : Srijeet change here

Review comment:
       I assume an Unmodified check needs to go here?

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -226,7 +244,7 @@ func (req *TODeliveryServiceRequest) Update() (error, error, int) {
 	userID := tc.IDNoMod(req.APIInfo().User.ID)
 	req.LastEditedByID = &userID
 
-	return api.GenericUpdate(req)
+	return api.GenericUpdate(nil, req)

Review comment:
       Why is this passing `nil`, and not the `http.Header`?

##########
File path: traffic_ops/traffic_ops_golang/cachegroup/cachegroups.go
##########
@@ -567,6 +582,22 @@ func (cg *TOCacheGroup) Update() (error, error, int) {
 	return nil, nil, http.StatusOK
 }
 
+func CheckIfExistsBeforeUpdate(tx *sqlx.Tx, cgID int) (error, *tc.TimeNoMod) {

Review comment:
       These functions look identical, except for the table in the select query.
   Could they be refactored to use a common func, maybe something like 
   ```
   // GetLastUpdated returns the last updated time of the given ID in the given table, whether the ID existed, and any error.
   func GetLastUpdated(tx *sqlx.Tx, dbTable string, id int) (time.Time, bool, error)
   ```` 
   
   In the `api` package?

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/safe.go
##########
@@ -23,11 +23,10 @@ import (
 	"database/sql"
 	"fmt"
 	"github.com/apache/trafficcontrol/lib/go-log"
-	"net/http"
-
 	"github.com/apache/trafficcontrol/lib/go-tc"
 	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/api"
 	"github.com/apache/trafficcontrol/traffic_ops/traffic_ops_golang/tenant"
+	"net/http"

Review comment:
       We try to separate imports with newlines for "standard library," "third party", and "our own," just to make it a little easier to read. Would you mind moving this up with the other standard library imports? Mind also moving `lib/go-log` down with the other TC ones? (that one wasn't yours, but while you're improving it, might as well do that one too).

##########
File path: traffic_ops/traffic_ops_golang/steeringtargets/steeringtargets.go
##########
@@ -244,6 +244,7 @@ func (st *TOSteeringTargetV11) Update() (error, error, int) {
 		return userErr, sysErr, errCode
 	}
 
+	// ToDo : Srijeet change here

Review comment:
       Need an Unmodified check here?

##########
File path: traffic_ops/traffic_ops_golang/server/servers_server_capability.go
##########
@@ -121,7 +121,7 @@ func (ssc TOServerServerCapability) Validate() error {
 	return util.JoinErrs(tovalidate.ToErrors(errs))
 }
 
-func (ssc *TOServerServerCapability) Update() (error, error, int) {
+func (ssc *TOServerServerCapability) Update(h http.Header) (error, error, int) {

Review comment:
       I know you didn't create this function, but it can be deleted.
   
   This is a legacy from when the "CRUDer" stuff required every func be implemented. This endpoint doesn't have a PUT method, and the "CRUDer" framework no longer requires all funcs, so it can just be deleted now.

##########
File path: traffic_ops/traffic_ops_golang/deliveryservice/request/requests.go
##########
@@ -383,6 +401,20 @@ func (req *deliveryServiceRequestAssignment) Update() (error, error, int) {
 	req.DeliveryServiceRequestNullable = current.DeliveryServiceRequestNullable
 	req.AssigneeID = assigneeID
 
+	err, existingLastUpdated := CheckIfExistsBeforeUpdate(req.ReqInfo.Tx, *req.ID)
+	if err != nil {
+		return errors.New("no delivery service request found with this id"), nil, http.StatusNotFound
+	}
+	_, iumsTime := rfc.GetETagOrIfUnmodifiedSinceTime(h)
+	existingEtag := rfc.ETag(existingLastUpdated.Time)
+
+	if h.Get(rfc.IfMatch) != "" && !strings.Contains(h.Get(rfc.IfMatch), existingEtag) {
+		return errors.New("header preconditions dont match"), nil, http.StatusPreconditionFailed

Review comment:
       The text here doesn't have to match the code, and since it's a user error, I think it's safe (from a security perspective) to let them know why they're getting a 412. Could we make the message something more informative like "resource was modified" ?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org