Merge pull request kbase#147 from kbase/dev-kill_DIE

Add query param to delete after file stream

README.md

@@ -171,7 +171,7 @@ RETURNS: an ACL.
 ## Download a file from a node
 ```
 AUTHORIZATION OPTIONAL
-GET /node/<id>?download[_raw][&seek=#][&length=#]
+GET /node/<id>?download[_raw][&seek=#][&length=#][&del]
 
 RETURNS: the file content.
 ```
@@ -186,6 +186,10 @@ to the file size is an error. Defaults to 0.
 `length` may be greater than the remaining file length. Defaults to 0, which indicates that the
 remainder of the file should be returned.
 
+`del` causes the node to be deleted once the file contents have been streamed. The user must
+be the node owner or a service administrator. Note this is playing very fast and loose with the
+semantics of an HTTP GET.
+
 ## Set a node to be publicly readable
 ```
 AUTHORIZATION REQUIRED

RELEASE_NOTES.md

@@ -1,17 +1,22 @@
+# 0.1.4
+
+* Added the `del` param when downloading the file from a node.
+
 # 0.1.3
 
-- Added GHA workflows and removed Travis CI
-- MongoController is now compatible with Mongo versions 2 through 7
-- Updated test config file to specify the auth2 shadow jar path vs. the jars repo path
+* Added GHA workflows and removed Travis CI
+* MongoController is now compatible with Mongo versions 2 through 7
+* Updated test config file to specify the auth2 shadow jar path vs. the jars repo path
 
 # 0.1.2
 
-- Support for disabling SSL verification of remote S3 certificates (default false) with the s3-disable-ssl-verify option in the configuration file.
+* Support for disabling SSL verification of remote S3 certificates (default false)
+  with the s3-disable-ssl-verify option in the configuration file.
 
 # 0.1.1
 
-- Added seek & length parameters to file download requests
+* Added seek & length parameters to file download requests
 
 # 0.1.0
 
-- Initial release
+* Initial release

service/errortypes.go

@@ -14,6 +14,20 @@ const (
 	invalidAuthHeader = "Invalid authorization header or content"
 )
 
+// UnauthorizedCustomError denotes that an unauthorized operation was requested that needs
+// special explanation in the error string.
+type UnauthorizedCustomError string
+
+// UnauthorizedCustomError creates a new UnauthorizedCustomError.
+func NewUnauthorizedCustomError(err string) *UnauthorizedCustomError {
+	e := UnauthorizedCustomError(err)
+	return &e
+}
+
+func (e *UnauthorizedCustomError) Error() string {
+	return string(*e)
+}
+
 func translateError(err error) (code int, errstr string) {
 	// not sure about this approach. Alternative is to add some state to every error that
 	// can be mapped to a code, and I'm not super thrilled about that either.
@@ -30,6 +44,8 @@ func translateError(err error) (code int, errstr string) {
 		// Shock compatibility, really should be 403 forbidden
 		return http.StatusBadRequest, "Users that are not node owners can only delete " +
 			"themselves from ACLs."
+	case *UnauthorizedCustomError:
+		return http.StatusUnauthorized, t.Error()
 	case *auth.InvalidUserError:
 		// no equivalent shock error, it accepts any string as a username
 		return http.StatusBadRequest, t.Error()

service/integration_test.go

@@ -544,6 +544,23 @@ func (t *TestSuite) TestStoreAndGetNodeAsAdminWithFormatAndTrailingSlashAndSeekA
 	t.checkFile(t.url+path+dlrlen, path, &t.kBaseAdmin, 7, "", []byte("foobarb"))
 }
 
+func (t *TestSuite) TestGetFileWithDelete() {
+	t.testGetFileWithDelete(t.noRole)
+	t.testGetFileWithDelete(t.stdRole)
+}
+
+func (t *TestSuite) testGetFileWithDelete(deleter User) {
+	body := t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
+		"OAuth "+t.noRole.token, 374, 200)
+	id := (body["data"].(map[string]interface{}))["id"].(string)
+	t.loggerhook.Reset()
+
+	path := "/node/" + id
+	dl := "?download&del"
+	t.checkFile(t.url+path+dl, path, &deleter, 9, id, []byte("foobarbaz"))
+	t.checkNodeDeleted(id, t.noRole)
+}
+
 func (t *TestSuite) TestStoreMIMEMultipartFilenameFormat() {
 	partsuffix := ` filename="myfile.txt"`
 	format := "gasbomb"
@@ -1065,7 +1082,7 @@ func (t *TestSuite) TestGetNodeFailPerms() {
 	)
 }
 
-func (t *TestSuite) TestGetNodeFailSeekAndLength() {
+func (t *TestSuite) TestGetFileFailSeekAndLength() {
 	body := t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
 		"OAuth "+t.kBaseAdmin.token, 374, 200)
 	id := (body["data"].(map[string]interface{}))["id"].(string)
@@ -1104,6 +1121,49 @@ func (t *TestSuite) TestGetNodeFailSeekAndLength() {
 	)
 }
 
+func (t *TestSuite) TestGetFileFailDelete() {
+	// only tests differences between the standard path and the delete path
+	body := t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
+		"OAuth "+t.noRole.token, 374, 200)
+	id := (body["data"].(map[string]interface{}))["id"].(string)
+	t.loggerhook.Reset()
+
+	uid := uuid.New()
+	body2 := t.get(t.url+"/node/"+uid.String()+"?download&del", &t.noRole, 75, 404)
+	t.checkError(body2, 404, "Node not found")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/"+uid.String(), 404, &t.noRole.user,
+		"Node not found", mtmap(), false},
+	)
+	body3 := t.get(t.url+"/node/"+id+"?download&del", &t.noRole2, 78, 401)
+	t.checkError(body3, 401, "User Unauthorized")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, &t.noRole2.user,
+		"User Unauthorized", mtmap(), false},
+	)
+	body4 := t.get(t.url+"/node/"+id+"?download&del", nil, 78, 401)
+	t.checkError(body4, 401, "User Unauthorized")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, nil,
+		"User Unauthorized", mtmap(), false},
+	)
+	// add user 2 to read acl
+	t.req("PUT", t.url+"/node/"+id+"/acl/read?users="+t.noRole2.user, nil,
+		"Oauth "+t.noRole.token, 441, 200)
+	t.loggerhook.Reset()
+	body5 := t.get(t.url+"/node/"+id+"?download&del", &t.noRole2, 94, 401)
+	t.checkError(body5, 401, "Only node owners can delete nodes")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, &t.noRole2.user,
+		"Only node owners can delete nodes", mtmap(), false},
+	)
+	// make node public
+	t.req("PUT", t.url+"/node/"+id+"/acl/public_read", nil,
+		"Oauth "+t.noRole.token, 440, 200)
+	t.loggerhook.Reset()
+	body6 := t.get(t.url+"/node/"+id+"?download&del", nil, 94, 401)
+	t.checkError(body6, 401, "Only node owners can delete nodes")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 401, nil,
+		"Only node owners can delete nodes", mtmap(), false},
+	)
+}
+
 func (t *TestSuite) TestUnexpectedError() {
 	defer t.createTestBucket()
 	body := t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
@@ -1150,6 +1210,7 @@ func (t *TestSuite) TestDeleteNode() {
 	t.checkLogs(logEvent{logrus.InfoLevel, "DELETE", "/node/" + id, 200, &t.noRole.user,
 		"request complete", mtmap(), true},
 	)
+	t.checkNodeDeleted(id, t.noRole)
 
 	// test delete as admin and with trailing slash
 	body = t.req("POST", t.url+"/node", strings.NewReader("foobarbaz"),
@@ -1164,6 +1225,15 @@ func (t *TestSuite) TestDeleteNode() {
 	t.checkLogs(logEvent{logrus.InfoLevel, "DELETE", "/node/" + id + "/", 200, &t.kBaseAdmin.user,
 		"request complete", mtmap(), true},
 	)
+	t.checkNodeDeleted(id, t.noRole)
+}
+
+func (t *TestSuite) checkNodeDeleted(id string, user User) {
+	body := t.get(t.url+"/node/"+id, &user, 75, 404)
+	t.checkError(body, 404, "Node not found")
+	t.checkLogs(logEvent{logrus.ErrorLevel, "GET", "/node/" + id, 404, &user.user,
+		"Node not found", mtmap(), false},
+	)
 }
 
 func (t *TestSuite) TestDeleteNodeFail() {

service/server.go

@@ -514,6 +514,11 @@ func (s *Server) getNode(w http.ResponseWriter, r *http.Request) {
 	user := getUser(r)
 	download := download(r.URL)
 	if download != "" {
+		del, err := checkDel(s, user, id, r.URL)
+		if err != nil {
+			writeError(le, err, w)
+			return
+		}
 		seek, length, err := getSeekAndLengthFromQuery(r.URL)
 		if err != nil {
 			writeError(le, err, w)
@@ -534,6 +539,9 @@ func (s *Server) getNode(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("content-length", strconv.FormatInt(size, 10))
 		w.Header().Set("content-type", "application/octet-stream")
 		io.Copy(w, datareader)
+		if del {
+			s.store.DeleteNode(*user, *id) // ignore errors since file is written to output
+		}
 	} else {
 		node, err := s.store.Get(user, *id)
 		if err != nil {
@@ -544,6 +552,29 @@ func (s *Server) getNode(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
+func checkDel(s *Server, user *auth.User, id *uuid.UUID, u *url.URL) (del bool, err error) {
+	delete := false
+	if _, ok := u.Query()["del"]; ok {
+		delete = true
+		// If we want to move the deletion into the core logic it could go like this:
+		// * pass in deletion flag and a pre-write function that accepts a node
+		// * do the ACL checks, etc.
+		// * the pre-write function is called, which writes the headers to the writer
+		//   * the core logic should not know the writer is a http.ResponseWriter
+		// * stream the file
+		// * delete the node
+		// not worth the trouble for now
+		node, err := s.store.Get(user, *id)
+		if err != nil {
+			return false, err
+		}
+		if user == nil || (user.GetUserName() != node.Owner.AccountName && !user.IsAdmin()) {
+			return false, NewUnauthorizedCustomError("Only node owners can delete nodes")
+		}
+	}
+	return delete, nil
+}
+
 func getSeekAndLengthFromQuery(u *url.URL) (uint64, uint64, error) {
 	seek, err := getUint64FromQuery(u, "seek")
 	if err != nil {