From 44bf972da6eb89a8725e01fe7f7f33057fbf3182 Mon Sep 17 00:00:00 2001 From: Gavin Date: Wed, 8 May 2024 19:32:39 -0700 Subject: [PATCH] Add query param to delete after file stream This is useful for temporary nodes where once the user has the data, the node is no longer needed, and the link to the data is expected to work once. --- README.md | 6 +++- RELEASE_NOTES.md | 17 +++++---- service/errortypes.go | 16 +++++++++ service/integration_test.go | 72 ++++++++++++++++++++++++++++++++++++- service/server.go | 31 ++++++++++++++++ 5 files changed, 134 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 8033038..cebf585 100644 --- a/README.md +++ b/README.md @@ -171,7 +171,7 @@ RETURNS: an ACL. ## Download a file from a node ``` AUTHORIZATION OPTIONAL -GET /node/?download[_raw][&seek=#][&length=#] +GET /node/?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 diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md index 43c2f8c..fb7efea 100644 --- a/RELEASE_NOTES.md +++ b/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 diff --git a/service/errortypes.go b/service/errortypes.go index b08e72e..8c7ebe4 100644 --- a/service/errortypes.go +++ b/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() diff --git a/service/integration_test.go b/service/integration_test.go index ae4a07b..4877f94 100644 --- a/service/integration_test.go +++ b/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() { diff --git a/service/server.go b/service/server.go index e331aef..b8f33ef 100644 --- a/service/server.go +++ b/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 {