-
Notifications
You must be signed in to change notification settings - Fork 37
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
internal/v5: Support symlink for files in archive. #817
base: v5
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
install |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,6 +23,7 @@ import ( | |
"gopkg.in/juju/charmstore.v5/internal/charmstore" | ||
"gopkg.in/juju/charmstore.v5/internal/mongodoc" | ||
"gopkg.in/juju/charmstore.v5/internal/router" | ||
"path" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this seems in the wrong place |
||
) | ||
|
||
// GET id/archive | ||
|
@@ -333,6 +334,17 @@ func (h *ReqHandler) serveArchiveFile(id *router.ResolvedURL, w http.ResponseWri | |
// with the given id. | ||
func (h *ReqHandler) ServeBlobFile(w http.ResponseWriter, req *http.Request, id *router.ResolvedURL, blob *charmstore.Blob) error { | ||
r, size, err := h.Store.OpenBlobFile(blob, req.URL.Path) | ||
if redirect, ok := err.(*charmstore.ErrRedirect); ok { | ||
p, err := router.RelativeURLPath(req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.Path)) | ||
if err != nil { | ||
return errgo.Notef(err, "cannot make relative URL from %q and %q", req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.Path)) | ||
} | ||
// We can't use the http.redirect function because it tries to build an absolute url but the path in the | ||
// request URL has been changed. | ||
w.Header().Set("Location", p) | ||
w.WriteHeader(http.StatusMovedPermanently) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think StatusFound would be better. We cannot say it is moved perminently only until a new zip file is uploaded. |
||
return nil | ||
} | ||
if err != nil { | ||
return errgo.Mask(err, errgo.Is(params.ErrNotFound), errgo.Is(params.ErrForbidden)) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,7 @@ import ( | |
"net/http/httptest" | ||
"net/url" | ||
"os" | ||
"path" | ||
"sort" | ||
"strconv" | ||
"strings" | ||
|
@@ -1178,25 +1179,24 @@ func (s *ArchiveSuite) TestArchiveFileGet(c *gc.C) { | |
s.assertArchiveFileContents(c, zipFile, "~charmers/utopic/all-hooks-0/archive/hooks/install") | ||
} | ||
|
||
func (s *ArchiveSuite) TestSymLinkArchiveFileGet(c *gc.C) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd like to see a few more tests of this behaviour. For example, does it work when the symlink is absolute, contains multiple slashes, trailing slashes? Does it work when the URL path contains a trailing slash or isn't clean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say if a zipfile contains an absolute symlink then it is broken. |
||
ch := storetesting.Charms.CharmArchive(c.MkDir(), "all-hooks") | ||
id := newResolvedURL("cs:~charmers/utopic/all-hooks-0", 0) | ||
s.addPublicCharm(c, ch, id) | ||
zipFile, err := zip.OpenReader(ch.Path) | ||
c.Assert(err, gc.Equals, nil) | ||
defer zipFile.Close() | ||
|
||
// Check a file in a subdirectory. | ||
s.assertArchiveFileContents(c, zipFile, "~charmers/utopic/all-hooks-0/archive/hooks/foo-relation-departed") | ||
} | ||
|
||
// assertArchiveFileContents checks that the response returned by the | ||
// serveArchiveFile endpoint is correct for the given archive and URL path. | ||
func (s *ArchiveSuite) assertArchiveFileContents(c *gc.C, zipFile *zip.ReadCloser, path string) { | ||
// For example: trusty/django/archive/hooks/install -> hooks/install. | ||
filePath := strings.SplitN(path, "/archive/", 2)[1] | ||
|
||
// Retrieve the expected bytes. | ||
var expectBytes []byte | ||
for _, file := range zipFile.File { | ||
if file.Name == filePath { | ||
r, err := file.Open() | ||
c.Assert(err, gc.Equals, nil) | ||
defer r.Close() | ||
expectBytes, err = ioutil.ReadAll(r) | ||
c.Assert(err, gc.Equals, nil) | ||
break | ||
} | ||
} | ||
c.Assert(expectBytes, gc.Not(gc.HasLen), 0) | ||
expectBytes := retrieveExpectedBytes(c, zipFile, filePath) | ||
|
||
// Make the request. | ||
url := storeURL(path) | ||
|
@@ -1207,14 +1207,33 @@ func (s *ArchiveSuite) assertArchiveFileContents(c *gc.C, zipFile *zip.ReadClose | |
|
||
// Ensure the response is what we expect. | ||
c.Assert(rec.Code, gc.Equals, http.StatusOK) | ||
c.Assert(rec.Body.Bytes(), gc.DeepEquals, expectBytes) | ||
c.Assert(string(rec.Body.Bytes()), gc.DeepEquals, string(expectBytes)) | ||
headers := rec.Header() | ||
c.Assert(headers.Get("Content-Length"), gc.Equals, strconv.Itoa(len(expectBytes))) | ||
// We only have text files in the charm repository used for tests. | ||
c.Assert(headers.Get("Content-Type"), gc.Equals, "text/plain; charset=utf-8") | ||
assertCacheControl(c, rec.Header(), true) | ||
} | ||
|
||
func retrieveExpectedBytes(c *gc.C, zipFile *zip.ReadCloser, filePath string) (expectBytes []byte) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This might be better named readZipFilePath, but honestly I think we could just leave things as they were and just test the symlink behaviour directly in the symlink test rather than calling assertArchiveFileContents. |
||
for _, file := range zipFile.File { | ||
if file.Name == filePath { | ||
r, err := file.Open() | ||
c.Assert(err, gc.Equals, nil) | ||
defer r.Close() | ||
expectBytes, err = ioutil.ReadAll(r) | ||
c.Assert(err, gc.Equals, nil) | ||
if file.Mode()&os.ModeSymlink != 0 { | ||
newPath := path.Join(path.Dir(filePath), string(expectBytes)) | ||
return retrieveExpectedBytes(c, zipFile, newPath) | ||
} | ||
break | ||
} | ||
} | ||
c.Assert(expectBytes, gc.Not(gc.HasLen), 0) | ||
return expectBytes | ||
} | ||
|
||
func (s *ArchiveSuite) TestDelete(c *gc.C) { | ||
// Add a charm to the database (including the archive). | ||
id, _ := s.addPublicCharm(c, storetesting.NewCharm(nil), newResolvedURL("~charmers/utopic/mysql-42", -1)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please document this behaviour in the doc comment for OpenBlobFile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.