diff options
author | Jaime Martinez <jmartinez@gitlab.com> | 2020-05-28 11:07:03 +0300 |
---|---|---|
committer | Jaime Martinez <jmartinez@gitlab.com> | 2020-07-06 02:13:51 +0300 |
commit | 4f82a5659cfcec5e7782b80f0d551353cbdcc1dc (patch) | |
tree | 0b9ccdfa09a1cbf932dfd23d26506b3096ccd285 /internal | |
parent | fb2c26ff998b809baddeb9618aae52c49200bc8b (diff) |
Get namespace domain if auth fails for a private domain
Add acceptance test and some more domains for testing
Move namespace domain serving logic
Restore go.sum
Remove redundant return
Fix linter
Diffstat (limited to 'internal')
-rw-r--r-- | internal/auth/auth.go | 43 | ||||
-rw-r--r-- | internal/auth/auth_test.go | 24 | ||||
-rw-r--r-- | internal/domain/domain.go | 21 | ||||
-rw-r--r-- | internal/domain/domain_test.go | 76 | ||||
-rw-r--r-- | internal/source/disk/map_test.go | 1 |
5 files changed, 128 insertions, 37 deletions
diff --git a/internal/auth/auth.go b/internal/auth/auth.go index c12207ca..768e7f75 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -18,7 +18,6 @@ import ( log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/labkit/errortracking" - "gitlab.com/gitlab-org/gitlab-pages/internal/domain" "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" "gitlab.com/gitlab-org/gitlab-pages/internal/httptransport" "gitlab.com/gitlab-org/gitlab-pages/internal/request" @@ -437,10 +436,10 @@ func (a *Auth) IsAuthSupported() bool { return a != nil } -func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, projectID uint64) bool { +func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, projectID uint64) (contentServed, authFailed bool) { session := a.checkSessionIsValid(w, r) if session == nil { - return true + return true, false } // Access token exists, authorize request @@ -457,14 +456,14 @@ func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, proje errortracking.Capture(err, errortracking.WithRequest(req)) httperrors.Serve500(w) - return true + return true, false } req.Header.Add("Authorization", "Bearer "+session.Values["access_token"].(string)) resp, err := a.apiClient.Do(req) if err == nil && checkResponseForInvalidToken(resp, session, w, r) { - return true + return true, false } if err != nil || resp.StatusCode != 200 { @@ -472,17 +471,17 @@ func (a *Auth) checkAuthentication(w http.ResponseWriter, r *http.Request, proje logRequest(r).WithError(err).Error("Failed to retrieve info with token") } - return false + return false, true } - return false + return false, false } // CheckAuthenticationWithoutProject checks if user is authenticated and has a valid token -func (a *Auth) CheckAuthenticationWithoutProject(w http.ResponseWriter, r *http.Request) bool { +func (a *Auth) CheckAuthenticationWithoutProject(w http.ResponseWriter, r *http.Request) (contentServed, authFailed bool) { if a == nil { // No auth supported - return false + return false, false } return a.checkAuthentication(w, r, 0) @@ -512,7 +511,8 @@ func (a *Auth) RequireAuth(w http.ResponseWriter, r *http.Request) bool { } // CheckAuthentication checks if user is authenticated and has access to the project -func (a *Auth) CheckAuthentication(w http.ResponseWriter, r *http.Request, domain *domain.Domain) bool { +// will return contentServed = false when authFailed = true +func (a *Auth) CheckAuthentication(w http.ResponseWriter, r *http.Request, projectID uint64) (contentServed, authFailed bool) { logRequest(r).Debug("Authenticate request") if a == nil { @@ -520,29 +520,10 @@ func (a *Auth) CheckAuthentication(w http.ResponseWriter, r *http.Request, domai errortracking.Capture(errAuthNotConfigured, errortracking.WithRequest(r)) httperrors.Serve500(w) - return true - } - - if a.checkAuthentication(w, r, domain.GetProjectID(r)) { - // if auth fails, try to resolve parent namespace domain - r.URL.Path = "/" - parent, err := domain.Resolver.Resolve(r) - if err != nil { - httperrors.Serve404(w) - return true - } - - // for namespace domains that have no access control enabled - if parent.LookupPath.IsNamespaceProject && !parent.LookupPath.HasAccessControl { - parent.ServeNotFoundHTTP(w, r) - return true - } - - httperrors.Serve404(w) - return true + return true, false } - return false + return a.checkAuthentication(w, r, projectID) } // CheckResponseForInvalidToken checks response for invalid token and destroys session if it was invalid diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index fc8ddb44..711dea78 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -181,7 +181,10 @@ func TestCheckAuthenticationWhenAccess(t *testing.T) { session.Values["access_token"] = "abc" session.Save(r, result) - require.Equal(t, false, auth.CheckAuthentication(result, r, 1000)) + contentServed, authFailed := auth.CheckAuthentication(result, r, 1000) + require.False(t, contentServed) + require.False(t, authFailed) + // content wasn't served so the default response from CheckAuthentication should be 200 require.Equal(t, 200, result.Code) } @@ -219,8 +222,11 @@ func TestCheckAuthenticationWhenNoAccess(t *testing.T) { session.Values["access_token"] = "abc" session.Save(r, result) - require.Equal(t, true, auth.CheckAuthentication(result, r, 1000)) - require.Equal(t, 404, result.Code) + contentServed, authFailed := auth.CheckAuthentication(result, r, 1000) + require.False(t, contentServed) + require.True(t, authFailed) + // content wasn't served so the default response from CheckAuthentication should be 200 + require.Equal(t, 200, result.Code) } func TestCheckAuthenticationWhenInvalidToken(t *testing.T) { @@ -257,7 +263,9 @@ func TestCheckAuthenticationWhenInvalidToken(t *testing.T) { session.Values["access_token"] = "abc" session.Save(r, result) - require.Equal(t, true, auth.CheckAuthentication(result, r, 1000)) + contentServed, authFailed := auth.CheckAuthentication(result, r, 1000) + require.True(t, contentServed) + require.False(t, authFailed) require.Equal(t, 302, result.Code) } @@ -295,7 +303,9 @@ func TestCheckAuthenticationWithoutProject(t *testing.T) { session.Values["access_token"] = "abc" session.Save(r, result) - require.Equal(t, false, auth.CheckAuthenticationWithoutProject(result, r)) + contentServed, authFailed := auth.CheckAuthenticationWithoutProject(result, r) + require.False(t, contentServed) + require.False(t, authFailed) require.Equal(t, 200, result.Code) } @@ -332,7 +342,9 @@ func TestCheckAuthenticationWithoutProjectWhenInvalidToken(t *testing.T) { session.Values["access_token"] = "abc" session.Save(r, result) - require.Equal(t, true, auth.CheckAuthenticationWithoutProject(result, r)) + contentServed, authFailed := auth.CheckAuthenticationWithoutProject(result, r) + require.True(t, contentServed) + require.False(t, authFailed) require.Equal(t, 302, result.Code) } diff --git a/internal/domain/domain.go b/internal/domain/domain.go index 17a0e1d3..2a301121 100644 --- a/internal/domain/domain.go +++ b/internal/domain/domain.go @@ -168,3 +168,24 @@ func (d *Domain) ServeNotFoundHTTP(w http.ResponseWriter, r *http.Request) { request.ServeNotFoundHTTP(w, r) } + +// ServeNamespaceNotFound will try to find a parent namespace domain for a request +// that failed authentication so that we serve the custom namespace error page for +// public namespace domains +func (d *Domain) ServeNamespaceNotFound(w http.ResponseWriter, r *http.Request) { + // override the path nd try to resolve the domain name + r.URL.Path = "/" + namespaceDomain, err := d.Resolver.Resolve(r) + if err != nil { + httperrors.Serve404(w) + return + } + + // for namespace domains that have no access control enabled + if namespaceDomain.LookupPath.IsNamespaceProject && !namespaceDomain.LookupPath.HasAccessControl { + namespaceDomain.ServeNotFoundHTTP(w, r) + return + } + + httperrors.Serve404(w) +} diff --git a/internal/domain/domain_test.go b/internal/domain/domain_test.go index 49d46cb3..e152b031 100644 --- a/internal/domain/domain_test.go +++ b/internal/domain/domain_test.go @@ -1,7 +1,10 @@ package domain import ( + "fmt" + "io/ioutil" "net/http" + "net/http/httptest" "os" "testing" @@ -153,3 +156,76 @@ func chdirInPath(t require.TestingT, path string) func() { chdirSet = false } } + +func TestDomain_ServeNamespaceNotFound(t *testing.T) { + // defaultNotFound := "The page you're looking for could not be found." + // customNotFound := "Custom error page" + + tests := []struct { + name string + domain string + path string + resolver *stubbedResolver + expectedResponse string + }{ + { + name: "public_namespace_domain", + domain: "group.404.gitlab-example.com", + path: "/unknown", + resolver: &stubbedResolver{ + project: &serving.LookupPath{ + Path: "../../shared/pages/group.404/group.404.gitlab-example.com/public", + IsNamespaceProject: true, + }, + subpath: "/unknown", + }, + expectedResponse: "Custom 404 group page", + }, + { + name: "private_project_under_public_namespace_domain", + domain: "group.404.gitlab-example.com", + path: "/private_project/unknown", + resolver: &stubbedResolver{ + project: &serving.LookupPath{ + Path: "../../shared/pages/group.404/group.404.gitlab-example.com/public", + IsNamespaceProject: true, + HasAccessControl: false, + }, + subpath: "/", + }, + expectedResponse: "Custom 404 group page", + }, + { + name: "private_namespace_domain", + domain: "group.404.gitlab-example.com", + path: "/unknown", + resolver: &stubbedResolver{ + project: &serving.LookupPath{ + Path: "../../shared/pages/group.404/group.404.gitlab-example.com/public", + IsNamespaceProject: true, + HasAccessControl: true, + }, + subpath: "/", + }, + expectedResponse: "The page you're looking for could not be found.", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + d := &Domain{ + Name: tt.domain, + Resolver: tt.resolver, + } + w := httptest.NewRecorder() + r := httptest.NewRequest("GET", fmt.Sprintf("http://%s%s", tt.domain, tt.path), nil) + d.ServeNamespaceNotFound(w, r) + + resp := w.Result() + require.Equal(t, http.StatusNotFound, resp.StatusCode) + body, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + + require.Contains(t, string(body), tt.expectedResponse) + }) + } +} diff --git a/internal/source/disk/map_test.go b/internal/source/disk/map_test.go index d904e6aa..d2162883 100644 --- a/internal/source/disk/map_test.go +++ b/internal/source/disk/map_test.go @@ -49,6 +49,7 @@ func TestReadProjects(t *testing.T) { "group.acme.test.io", "withacmechallenge.domain.com", "capitalgroup.test.io", + "group.404.gitlab-example.com", } for _, expected := range domains { |