Added more tests (#29)

- Key changes:
  - Minor improvements in docs;
  - Minor improvements in logging;
  - Added more tests;
  - `VictoriaMetrics/metricsql` bumped from `0.42.0` to `0.43.0`.

CHANGELOG.md

@@ -1,5 +1,13 @@
 # CHANGELOG
 
+## 0.12.2
+
+- Key changes:
+  - Minor improvements in docs;
+  - Minor improvements in logging;
+  - Added more tests;
+  - `VictoriaMetrics/metricsql` bumped from `0.42.0` to `0.43.0`.
+
 ## 0.12.1
 
 - Key changes:

README.md

@@ -27,12 +27,12 @@ Docker images are published on [ghcr.io/weisdd/lfgw](https://github.com/weisdd/l
 
 ## Configuration
 
-Example of `keycloak + grafana + lfgw` setup is described [here](./docs/oidc.md).
+Example of `keycloak + grafana + lfgw` setup is described [here](docs/oidc.md).
 
 ### Requirements for jwt-tokens
 
 * OIDC-roles must be present in `roles` claim;
-* Client ID specified via `OIDC_CLIENT_ID` must be present in `aud` claim (more details in [environment variables section](#Environment variables)), otherwise token verification will fail.
+* Client ID specified via `OIDC_CLIENT_ID` must be present in `aud` claim (more details in [environment variables section](#environment-variables)), otherwise token verification will fail.
 
 ### Environment variables
 
@@ -105,11 +105,10 @@ Note: a user is free to have multiple roles matching the contents of `acl.yaml`.
 
 ## Licensing
 
-lfgw code is licensed under MIT, though its dependencies might have other licenses. Please, inspect the modules listed in [go.mod](./go.mod) if needed.
+lfgw code is licensed under MIT, though its dependencies might have other licenses. Please, inspect the modules listed in [go.mod](go.mod) if needed.
 
 ## TODO
 
-* tests for handlers;
 * improve naming;
 * log slow requests;
 * metrics;

Taskfile.yml

@@ -18,3 +18,9 @@ tasks:
   tidy:
     cmds:
       - go mod tidy
+
+  cov:
+    cmds:
+      - mkdir -p temp
+      - go test -coverprofile=temp/coverage.out ./...
+      - go tool cover -html=temp/coverage.out

internal/lfgw/helpers.go

@@ -43,7 +43,17 @@ func (app *application) getRawAccessToken(r *http.Request) (string, error) {
 		}
 	}
 
-	return "", fmt.Errorf("no bearer token found")
+	var err error
+
+	isGrafanaRequest := strings.Contains(strings.ToLower(r.UserAgent()), "grafana")
+
+	if isGrafanaRequest {
+		err = fmt.Errorf("no bearer token found, possible causes: grafana data source is not configured with Forward Oauth Identity option; grafana user sessions are not tuned to live shorter than IDP sessions; malicious requests")
+	} else {
+		err = fmt.Errorf("no bearer token found")
+	}
+
+	return "", err
 }
 
 // isNotAPIRequest returns true if the requested path does not target API or federate endpoints.

internal/lfgw/logging.go

@@ -31,6 +31,7 @@ func (s stdErrorLogWrapper) Write(p []byte) (n int, err error) {
 	return len(p), nil
 }
 
+// configureLogging configures zerolog and sets the respective fields in the application struct
 func (app *application) configureLogging() {
 	zlog.Logger = zlog.Output(os.Stdout)
 	app.logger = &zlog.Logger

internal/lfgw/main.go

@@ -44,6 +44,18 @@ type application struct {
 	logger                  *zerolog.Logger
 }
 
+// Run is used as an entrypoint for cli
+func Run(c *cli.Context) error {
+	app, err := newApplication(c)
+	if err != nil {
+		return err
+	}
+
+	app.Run()
+
+	return nil
+}
+
 // newApplication returns application struct built from *cli.Context
 func newApplication(c *cli.Context) (application, error) {
 	upstreamURL, err := url.Parse(c.String("upstream-url"))
@@ -75,22 +87,13 @@ func newApplication(c *cli.Context) (application, error) {
 	return app, nil
 }
 
-// Run is used as an entrypoint for cli
-func Run(c *cli.Context) error {
-	app, err := newApplication(c)
-	if err != nil {
-		return err
-	}
-
-	app.Run()
-
-	return nil
-}
-
 // Run starts lfgw (main-like function)
 func (app *application) Run() {
 	app.configureLogging()
+	app.configureACLs()
+	app.configureOIDCVerifier()
 
+	// TODO: expose undo and move to another function?
 	if app.SetGomaxProcs {
 		undo, err := maxprocs.Set()
 		defer undo()
@@ -102,6 +105,20 @@ func (app *application) Run() {
 	app.logger.Info().Caller().
 		Msgf("Runtime settings: GOMAXPROCS = %d", runtime.GOMAXPROCS(0))
 
+	err := app.serve()
+	if err != nil {
+		app.logger.Fatal().Caller().
+			Err(err).Msg("")
+	}
+}
+
+// configureACLs logs assumed roles mode, verifies current ACLs settings (assumed roles, aclpath), loads the ACLs from a file and logs roles if needed
+func (app *application) configureACLs() {
+	// Just to make sure our logging calls are always safe
+	if app.logger == nil {
+		app.configureLogging()
+	}
+
 	if app.AssumedRolesEnabled {
 		app.logger.Info().Caller().
 			Msg("Assumed roles mode is on")
@@ -110,28 +127,38 @@ func (app *application) Run() {
 			Msg("Assumed roles mode is off")
 	}
 
-	var err error
-
-	if app.ACLPath != "" {
-		app.ACLs, err = querymodifier.NewACLsFromFile(app.ACLPath)
-		if err != nil {
-			app.logger.Fatal().Caller().
-				Err(err).Msgf("Failed to load ACL")
-		}
-
-		for role, acl := range app.ACLs {
-			app.logger.Info().Caller().
-				Msgf("Loaded role definition for %s: %q (converted to %s)", role, acl.RawACL, acl.LabelFilter.AppendString(nil))
-		}
-	} else {
+	if app.ACLPath == "" {
 		// NOTE: the condition should never happen as it's filtered out by "Before" functionality of cli, though left just in case
 		if !app.AssumedRolesEnabled {
 			app.logger.Fatal().Caller().
 				Msgf("The app cannot run without at least one source of configuration (Non-empty ACL_PATH and/or ASSUMED_ROLES set to true)")
 		}
 
 		app.logger.Info().Caller().
-			Msgf("ACL_PATH is empty, thus predefined roles are not loaded")
+			Msgf("ACL_PATH is empty, thus predefined roles will not be loaded")
+
+		return
+	}
+
+	var err error
+
+	app.ACLs, err = querymodifier.NewACLsFromFile(app.ACLPath)
+	if err != nil {
+		app.logger.Fatal().Caller().
+			Err(err).Msgf("Failed to load ACL")
+	}
+
+	for role, acl := range app.ACLs {
+		app.logger.Info().Caller().
+			Msgf("Loaded role definition for %s: %q (converted to %s)", role, acl.RawACL, acl.LabelFilter.AppendString(nil))
+	}
+}
+
+// configureOIDCVerifier sets up OIDC token verifier by using app.OIDCRealmURL and app.OIDCClientID
+func (app *application) configureOIDCVerifier() {
+	// Just to make sure our logging calls are always safe
+	if app.logger == nil {
+		app.configureLogging()
 	}
 
 	app.logger.Info().Caller().
@@ -149,10 +176,4 @@ func (app *application) Run() {
 		ClientID: app.OIDCClientID,
 	}
 	app.verifier = provider.Verifier(oidcConfig)
-
-	err = app.serve()
-	if err != nil {
-		app.logger.Fatal().Caller().
-			Err(err).Msg("")
-	}
 }

internal/lfgw/middleware.go

@@ -55,6 +55,10 @@ func (app *application) logMiddleware(next http.Handler) http.Handler {
 			// If any of those are empty, they won't get logged
 			app.enrichDebugLogContext(r, "get_params", app.unescapedURLQuery(r.URL.Query().Encode()))
 			app.enrichDebugLogContext(r, "post_params", app.unescapedURLQuery(postForm))
+
+			// Workaround to make further r.ParseForm() calls update r.Form and r.PostForm again, might be useful in case there's another middleware before rewriteRequestMiddleware
+			r.Form = nil
+			r.PostForm = nil
 		}
 
 		if app.LogRequests || app.Debug {
@@ -106,6 +110,10 @@ func (app *application) oidcModeMiddleware(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		rawAccessToken, err := app.getRawAccessToken(r)
 		if err != nil {
+			// Might produce plenty of error messages, though it will make it much easier to understand why requests are failing
+			hlog.FromRequest(r).Error().Caller().
+				Err(err).Msg("")
+
 			app.clientErrorMessage(w, http.StatusUnauthorized, err)
 			return
 		}
@@ -158,23 +166,29 @@ func (app *application) oidcModeMiddleware(next http.Handler) http.Handler {
 // rewriteRequestMiddleware rewrites a request before forwarding it to the upstream.
 func (app *application) rewriteRequestMiddleware(next http.Handler) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		// Rewrite request destination
-		r.Host = app.UpstreamURL.Host
-
-		if app.isNotAPIRequest(r.URL.Path) {
-			hlog.FromRequest(r).Debug().Caller().
-				Msg("Not an API request, request is not modified")
-			next.ServeHTTP(w, r)
+		// TODO: rewrite?
+		if app.UpstreamURL == nil {
+			app.serverError(w, r, fmt.Errorf("UpstreamURL is not initialized"))
 			return
 		}
 
+		// Rewrite request destination
+		r.Host = app.UpstreamURL.Host
+
 		acl, ok := r.Context().Value(contextKeyACL).(querymodifier.ACL)
 		if !ok {
 			// Should never happen. It means OIDC middleware hasn't done it's job
 			app.serverError(w, r, fmt.Errorf("ACL is not set in the context"))
 			return
 		}
 
+		if app.isNotAPIRequest(r.URL.Path) {
+			hlog.FromRequest(r).Debug().Caller().
+				Msg("Not an API request, request is not modified")
+			next.ServeHTTP(w, r)
+			return
+		}
+
 		if acl.Fullaccess {
 			hlog.FromRequest(r).Debug().Caller().
 				Msg("User has full access, request is not modified")
@@ -219,6 +233,10 @@ func (app *application) rewriteRequestMiddleware(next http.Handler) http.Handler
 		// TODO: the field name is slightly misleading, should, probably, be renamed
 		app.enrichDebugLogContext(r, "new_post_params", app.unescapedURLQuery(newPostParams))
 
+		// Workaround to make further r.ParseForm() calls update r.Form and r.PostForm again, might be useful in case there's another middleware before rewriteRequestMiddleware
+		r.Form = nil
+		r.PostForm = nil
+
 		next.ServeHTTP(w, r)
 	})
 }