Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add query param to delete after file stream #147

Merged
merged 1 commit into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
```
Expand All @@ -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
Expand Down
17 changes: 11 additions & 6 deletions RELEASE_NOTES.md
Original file line number Diff line number Diff line change
@@ -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
16 changes: 16 additions & 0 deletions service/errortypes.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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()
Expand Down
72 changes: 71 additions & 1 deletion service/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
Expand All @@ -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() {
Expand Down
31 changes: 31 additions & 0 deletions service/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand All @@ -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()) {
Copy link
Collaborator

@Xiangs18 Xiangs18 May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

L571, if any of condition in () in False, then the error will not be raised. This does not sound right, unless I am missing something.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parentheses say "If the user is not the node owner and the user is not an admin, raise an error", which is the behavior we want

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So either be the node owner or has admin right is good

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. You could do

if user == node.owner or user.isAdmin() {
    //pass
} else {
    throw NewError(...)

which would have equivalent logic, but that's just wasted space

Copy link
Collaborator

@Xiangs18 Xiangs18 May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is less restricted that I thought. But it makes sense.

yeah, it makes more sense. Admin can have access to everything, and I get access if I am the owner.

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 {
Expand Down
Loading