From 6cfdd8cbe16237c74e90b7708b175392a0c4f4af Mon Sep 17 00:00:00 2001 From: Fabrice Matrat Date: Mon, 23 Jul 2018 15:11:25 +0200 Subject: [PATCH 1/2] internal/v5: Support symlink for files in archive. --- internal/charmstore/archive.go | 18 +++++++ .../all-hooks/hooks/foo-relation-departed | 3 +- internal/v5/archive.go | 12 +++++ internal/v5/archive_test.go | 49 +++++++++++++------ 4 files changed, 65 insertions(+), 17 deletions(-) mode change 100644 => 120000 internal/storetesting/charm-repo/quantal/all-hooks/hooks/foo-relation-departed diff --git a/internal/charmstore/archive.go b/internal/charmstore/archive.go index 6d7e8880b..61e329f4c 100644 --- a/internal/charmstore/archive.go +++ b/internal/charmstore/archive.go @@ -6,7 +6,9 @@ package charmstore // import "gopkg.in/juju/charmstore.v5/internal/charmstore" import ( "archive/zip" "bytes" + "fmt" "io" + "io/ioutil" "os" "path" "strings" @@ -135,11 +137,27 @@ func (s *Store) OpenBlobFile(blob *Blob, filePath string) (io.ReadCloser, int64, if err != nil { return nil, 0, errgo.Notef(err, "unable to read file %q", filePath) } + if file.Mode()&os.ModeSymlink != 0 { + defer content.Close() + url, err := ioutil.ReadAll(content) + if err != nil { + return nil, 0, errgo.Notef(err, "cannot read archive data for symlink %s", file.Name) + } + return nil, 0, ErrRedirect{URL: string(url)} + } return content, fileInfo.Size(), nil } return nil, 0, errgo.WithCausef(nil, params.ErrNotFound, "file %q not found in the archive", filePath) } +type ErrRedirect struct { + URL string +} + +func (e ErrRedirect) Error() string { + return fmt.Sprintf("redirect to %v", e.URL) +} + // OpenCachedBlobFile opens a file from the given entity's archive blob. // The file is identified by the provided fileId. If the file has not // previously been opened on this entity, the isFile function will be diff --git a/internal/storetesting/charm-repo/quantal/all-hooks/hooks/foo-relation-departed b/internal/storetesting/charm-repo/quantal/all-hooks/hooks/foo-relation-departed deleted file mode 100644 index e99481016..000000000 --- a/internal/storetesting/charm-repo/quantal/all-hooks/hooks/foo-relation-departed +++ /dev/null @@ -1,2 +0,0 @@ -#!/bin/sh -echo $0 diff --git a/internal/storetesting/charm-repo/quantal/all-hooks/hooks/foo-relation-departed b/internal/storetesting/charm-repo/quantal/all-hooks/hooks/foo-relation-departed new file mode 120000 index 000000000..f7ffc47af --- /dev/null +++ b/internal/storetesting/charm-repo/quantal/all-hooks/hooks/foo-relation-departed @@ -0,0 +1 @@ +install \ No newline at end of file diff --git a/internal/v5/archive.go b/internal/v5/archive.go index 52f1fdc79..466f04e91 100644 --- a/internal/v5/archive.go +++ b/internal/v5/archive.go @@ -10,6 +10,7 @@ import ( "io/ioutil" "mime" "net/http" + "path" "path/filepath" "strconv" "time" @@ -334,6 +335,17 @@ func (h *ReqHandler) serveArchiveFile(id *router.ResolvedURL, w http.ResponseWri 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 err != nil { + if redirect, ok := err.(charmstore.ErrRedirect); ok { + p, err := router.RelativeURLPath(req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.URL)) + 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.URL)) + } + // 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) + return nil + } return errgo.Mask(err, errgo.Is(params.ErrNotFound), errgo.Is(params.ErrForbidden)) } defer r.Close() diff --git a/internal/v5/archive_test.go b/internal/v5/archive_test.go index a78a01c92..7663085a7 100644 --- a/internal/v5/archive_test.go +++ b/internal/v5/archive_test.go @@ -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) { + 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,7 +1207,7 @@ 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. @@ -1215,6 +1215,25 @@ func (s *ArchiveSuite) assertArchiveFileContents(c *gc.C, zipFile *zip.ReadClose assertCacheControl(c, rec.Header(), true) } +func retrieveExpectedBytes(c *gc.C, zipFile *zip.ReadCloser, filePath string) (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) + 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)) From 6a001cfe07cd2157b09af3329c3dbdc40bfe6a17 Mon Sep 17 00:00:00 2001 From: Fabrice Matrat Date: Tue, 24 Jul 2018 09:36:27 +0200 Subject: [PATCH 2/2] Reviews. --- internal/charmstore/archive.go | 12 +++++++++--- internal/v5/archive.go | 24 ++++++++++++------------ 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/internal/charmstore/archive.go b/internal/charmstore/archive.go index 61e329f4c..5cde40043 100644 --- a/internal/charmstore/archive.go +++ b/internal/charmstore/archive.go @@ -115,6 +115,9 @@ func (r *multiReadSeekCloser) Close() error { // If no such file was found, it returns an error // with a params.ErrNotFound cause. // +// If the file is a symlink, it returns an error ErrRedirect that holds the path +// that the symbolic link refers to. +// // If the file is actually a directory in the blob, it returns // an error with a params.ErrForbidden cause. func (s *Store) OpenBlobFile(blob *Blob, filePath string) (io.ReadCloser, int64, error) { @@ -143,19 +146,22 @@ func (s *Store) OpenBlobFile(blob *Blob, filePath string) (io.ReadCloser, int64, if err != nil { return nil, 0, errgo.Notef(err, "cannot read archive data for symlink %s", file.Name) } - return nil, 0, ErrRedirect{URL: string(url)} + return nil, 0, &ErrRedirect{Path: string(url)} } return content, fileInfo.Size(), nil } return nil, 0, errgo.WithCausef(nil, params.ErrNotFound, "file %q not found in the archive", filePath) } +// ErrRedirect holds the error returned by OpenBlobFile when a symbolic link +// is opened. The Path field holds the name of the file that the symbolic link +// refers to. type ErrRedirect struct { - URL string + Path string } func (e ErrRedirect) Error() string { - return fmt.Sprintf("redirect to %v", e.URL) + return fmt.Sprintf("file is a symbolic link to %q", e.Path) } // OpenCachedBlobFile opens a file from the given entity's archive blob. diff --git a/internal/v5/archive.go b/internal/v5/archive.go index 466f04e91..7312efbd5 100644 --- a/internal/v5/archive.go +++ b/internal/v5/archive.go @@ -10,7 +10,6 @@ import ( "io/ioutil" "mime" "net/http" - "path" "path/filepath" "strconv" "time" @@ -24,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" ) // GET id/archive @@ -334,18 +334,18 @@ 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 err != nil { - if redirect, ok := err.(charmstore.ErrRedirect); ok { - p, err := router.RelativeURLPath(req.URL.Path, path.Join(path.Dir(req.URL.Path), redirect.URL)) - 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.URL)) - } - // 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) - return nil + 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) + return nil + } + if err != nil { return errgo.Mask(err, errgo.Is(params.ErrNotFound), errgo.Is(params.ErrForbidden)) } defer r.Close()