From 85c03a97fbe54830e306706cca4174bff1f379bc Mon Sep 17 00:00:00 2001 From: "Babak K. Shandiz" Date: Wed, 25 Sep 2024 12:43:16 +0100 Subject: [PATCH] Ignore auth for `/swagger.json` endpoint (#1377) * Ignore authentication for `/swagger.json` in ReBAC admin API Signed-off-by: Babak K. Shandiz * Add test case to verify skipping auth for `/swagger.json` Signed-off-by: Babak K. Shandiz * Fix ReBAC Admin test Signed-off-by: Babak K. Shandiz * Use a map to lookup for endpoints Signed-off-by: Babak K. Shandiz * Upgrade `rebac-admin-ui-handlers` to `v0.1.2` Signed-off-by: Babak K. Shandiz * Use zero-length struct in map/set declaration Signed-off-by: Babak K. Shandiz --------- Signed-off-by: Babak K. Shandiz --- cmd/jimmsrv/service/service.go | 2 +- cmd/jimmsrv/service/service_test.go | 5 ++- go.mod | 2 +- go.sum | 2 + internal/middleware/auth.go | 27 +++++++++--- internal/middleware/auth_test.go | 65 +++++++++++++++++++++++------ 6 files changed, 82 insertions(+), 21 deletions(-) diff --git a/cmd/jimmsrv/service/service.go b/cmd/jimmsrv/service/service.go index ecfadf702..a1544792f 100644 --- a/cmd/jimmsrv/service/service.go +++ b/cmd/jimmsrv/service/service.go @@ -408,7 +408,7 @@ func NewService(ctx context.Context, p Params) (*Service, error) { s.mux.Mount("/metrics", promhttp.Handler()) - s.mux.Mount("/rebac", middleware.AuthenticateRebac(rebacBackend.Handler(""), &s.jimm)) + s.mux.Mount("/rebac", middleware.AuthenticateRebac("/rebac", rebacBackend.Handler(""), &s.jimm)) mountHandler( "/debug", diff --git a/cmd/jimmsrv/service/service_test.go b/cmd/jimmsrv/service/service_test.go index 33014065d..f82fa3627 100644 --- a/cmd/jimmsrv/service/service_test.go +++ b/cmd/jimmsrv/service/service_test.go @@ -294,7 +294,10 @@ func TestRebacAdminApi(t *testing.T) { response, err := srv.Client().Get(srv.URL + "/rebac/v1/swagger.json") c.Assert(err, qt.IsNil) defer response.Body.Close() - c.Assert(response.StatusCode, qt.Equals, 401) + + // The `/swagger.json` endpoint doesn't require authentication, so the returned + // status code should be 200. + c.Assert(response.StatusCode, qt.Equals, 200) } func TestThirdPartyCaveatDischarge(t *testing.T) { diff --git a/go.mod b/go.mod index e2dbc9fac..f2a79b38b 100644 --- a/go.mod +++ b/go.mod @@ -45,7 +45,7 @@ require ( require ( github.com/antonlindstrom/pgstore v0.0.0-20220421113606-e3a6e3fed12a github.com/canonical/ofga v0.10.0 - github.com/canonical/rebac-admin-ui-handlers v0.1.1 + github.com/canonical/rebac-admin-ui-handlers v0.1.2 github.com/coreos/go-oidc/v3 v3.9.0 github.com/dustinkirkland/golang-petname v0.0.0-20231002161417-6a283f1aaaf2 github.com/go-chi/chi/v5 v5.0.12 diff --git a/go.sum b/go.sum index 8d8a65842..6c4c8b310 100644 --- a/go.sum +++ b/go.sum @@ -158,6 +158,8 @@ github.com/canonical/rebac-admin-ui-handlers v0.1.0 h1:Bef1N/RgQine8hHX4ZMksQz/1 github.com/canonical/rebac-admin-ui-handlers v0.1.0/go.mod h1:EIdBoaTHWYPkzNeUeXUBueJkglN9nQz5HLIvaOT7o1k= github.com/canonical/rebac-admin-ui-handlers v0.1.1 h1:rjsb45diShhwD/uUFpai6gmhFUzT+jTdsnEWcOvcKx4= github.com/canonical/rebac-admin-ui-handlers v0.1.1/go.mod h1:EIdBoaTHWYPkzNeUeXUBueJkglN9nQz5HLIvaOT7o1k= +github.com/canonical/rebac-admin-ui-handlers v0.1.2 h1:tQU/NWQVD9IEgy3ct8JWZXpLK/dedWjKcy9E8W4glpo= +github.com/canonical/rebac-admin-ui-handlers v0.1.2/go.mod h1:EIdBoaTHWYPkzNeUeXUBueJkglN9nQz5HLIvaOT7o1k= github.com/cenkalti/backoff/v3 v3.0.0/go.mod h1:cIeZDE3IrqwwJl6VUwCN6trj1oXrTS4rc0ij+ULvLYs= github.com/cenkalti/backoff/v3 v3.2.2 h1:cfUAAO3yvKMYKPrvhDuHSwQnhZNk/RMHKdZqKTxfm6M= github.com/cenkalti/backoff/v3 v3.2.2/go.mod h1:cIeZDE3IrqwwJl6VUwCN6trj1oXrTS4rc0ij+ULvLYs= diff --git a/internal/middleware/auth.go b/internal/middleware/auth.go index 44120ffbf..5bf0c75cf 100644 --- a/internal/middleware/auth.go +++ b/internal/middleware/auth.go @@ -4,6 +4,7 @@ package middleware import ( "net/http" + "strings" rebac_handlers "github.com/canonical/rebac-admin-ui-handlers/v1" "github.com/juju/zaputil/zapctx" @@ -27,11 +28,18 @@ func AuthenticateViaCookie(next http.Handler, jimm jujuapi.JIMM) http.Handler { }) } -// AuthenticateRebac is a layer on top of AuthenticateViaCookie -// It places the OpenFGA user for the session identity inside the request's context -// and verifies that the user is a JIMM admin. -func AuthenticateRebac(next http.Handler, jimm jujuapi.JIMM) http.Handler { - return AuthenticateViaCookie(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { +// Set of ReBAC Admin API endpoints that do not require authentication. +var unauthenticatedEndpoints = map[string]struct{}{ + "/v1/swagger.json": {}, +} + +// AuthenticateRebac is a layer on top of AuthenticateViaCookie. It places the +// OpenFGA user for the session identity inside the request's context and +// verifies that the user is a JIMM admin. Note that the method needs the base +// URL to decide if the request does not require authentication; this is to +// safeguard against conflicting/similar endpoint names in the future. +func AuthenticateRebac(baseURL string, next http.Handler, jimm jujuapi.JIMM) http.Handler { + cookieAuthenticator := AuthenticateViaCookie(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() identity := auth.SessionIdentityFromContext(ctx) @@ -56,4 +64,13 @@ func AuthenticateRebac(next http.Handler, jimm jujuapi.JIMM) http.Handler { ctx = rebac_handlers.ContextWithIdentity(ctx, user) next.ServeHTTP(w, r.WithContext(ctx)) }), jimm) + + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + relativePath, _ := strings.CutPrefix(r.URL.Path, baseURL) + if _, found := unauthenticatedEndpoints[relativePath]; found { + next.ServeHTTP(w, r) + return + } + cookieAuthenticator.ServeHTTP(w, r) + }) } diff --git a/internal/middleware/auth_test.go b/internal/middleware/auth_test.go index a622f542b..185226d85 100644 --- a/internal/middleware/auth_test.go +++ b/internal/middleware/auth_test.go @@ -5,6 +5,7 @@ package middleware_test import ( "context" "errors" + "io" "net/http" "net/http/httptest" "testing" @@ -22,12 +23,18 @@ import ( // Checks if the authenticator responsible for access control to rebac admin handlers works correctly. func TestAuthenticateRebac(t *testing.T) { + c := qt.New(t) + baseURL := "/rebac" + testUser := "test-user@canonical.com" tests := []struct { name string + setupRequest func() *http.Request + setupHandler func() http.Handler mockAuthBrowserSession func(ctx context.Context, w http.ResponseWriter, req *http.Request) (context.Context, error) jimmAdmin bool expectedStatus int + expectedBody string }{ { name: "success", @@ -59,12 +66,26 @@ func TestAuthenticateRebac(t *testing.T) { jimmAdmin: false, expectedStatus: http.StatusUnauthorized, }, + { + name: "should skip auth for /swagger.json", + setupRequest: func() *http.Request { + req, _ := http.NewRequest(http.MethodGet, baseURL+"/v1/swagger.json", nil) + return req + }, + setupHandler: func() http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte("the-body")) + }) + }, + jimmAdmin: false, + expectedStatus: http.StatusOK, + expectedBody: "the-body", + }, } for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - c := qt.New(t) - + c.Run(tt.name, func(c *qt.C) { j := jimmtest.JIMM{ LoginService: mocks.LoginService{ AuthenticateBrowserSession_: func(ctx context.Context, w http.ResponseWriter, req *http.Request) (context.Context, error) { @@ -76,23 +97,41 @@ func TestAuthenticateRebac(t *testing.T) { return &openfga.User{Identity: &user, JimmAdmin: tt.jimmAdmin}, nil }, } - req := httptest.NewRequest(http.MethodGet, "/", nil) + + var req *http.Request + if tt.setupRequest != nil { + req = tt.setupRequest() + } else { + req = httptest.NewRequest(http.MethodGet, baseURL, nil) + } + w := httptest.NewRecorder() - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - identity, err := rebac_handlers.GetIdentityFromContext(r.Context()) - c.Assert(err, qt.IsNil) + var handler http.Handler + if tt.setupHandler != nil { + handler = tt.setupHandler() + } else { + handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + identity, err := rebac_handlers.GetIdentityFromContext(r.Context()) + c.Assert(err, qt.IsNil) - user, ok := identity.(*openfga.User) - c.Assert(ok, qt.IsTrue) - c.Assert(user.Name, qt.Equals, testUser) + user, ok := identity.(*openfga.User) + c.Assert(ok, qt.IsTrue) + c.Assert(user.Name, qt.Equals, testUser) - w.WriteHeader(http.StatusOK) - }) - middleware := middleware.AuthenticateRebac(handler, &j) + w.WriteHeader(http.StatusOK) + }) + } + + middleware := middleware.AuthenticateRebac(baseURL, handler, &j) middleware.ServeHTTP(w, req) c.Assert(w.Code, qt.Equals, tt.expectedStatus) + if tt.expectedBody != "" { + body, err := io.ReadAll(w.Body) + c.Assert(err, qt.IsNil) + c.Assert(string(body), qt.Equals, tt.expectedBody) + } }) } }