Code review changes

cmd/gitops/root/cmd.go

@@ -135,7 +135,7 @@ func RootCmd(client *resty.Client) *cobra.Command {
 	rootCmd.AddCommand(uninstall.Cmd)
 	rootCmd.AddCommand(version.Cmd)
 	rootCmd.AddCommand(flux.Cmd)
-	rootCmd.AddCommand(ui.Command())
+	rootCmd.AddCommand(ui.NewCommand())
 	rootCmd.AddCommand(get.GetCommand(&options.endpoint, client))
 	rootCmd.AddCommand(add.GetCommand(&options.endpoint, client))
 	rootCmd.AddCommand(delete.DeleteCommand(&options.endpoint, client))

cmd/gitops/ui/cmd.go

@@ -5,8 +5,8 @@ import (
 	"github.com/weaveworks/weave-gitops/cmd/gitops/ui/run"
 )
 
-// Command returns the `ui` command and its subcommands.
-func Command() *cobra.Command {
+// NewCommand returns the `ui` command and its subcommands.
+func NewCommand() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "ui",
 		Short: "Manages Gitops UI",
@@ -17,7 +17,7 @@ func Command() *cobra.Command {
 		Args: cobra.MinimumNArgs(1),
 	}
 
-	cmd.AddCommand(run.Command())
+	cmd.AddCommand(run.NewCommand())
 
 	return cmd
 }

cmd/gitops/ui/run/cmd.go

@@ -5,6 +5,7 @@ import (
 	"embed"
 	"fmt"
 	"io/fs"
+	"net"
 	"net/http"
 	"net/url"
 	"os"
@@ -41,13 +42,13 @@ type OIDCAuthenticationOptions struct {
 	ClientID       string
 	ClientSecret   string
 	RedirectURL    string
-	CookieDuration string
+	CookieDuration time.Duration
 }
 
 var options Options
 
-// Command returns the `ui run` command
-func Command() *cobra.Command {
+// NewCommand returns the `ui run` command
+func NewCommand() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "run [--log]",
 		Short: "Runs gitops ui",
@@ -67,7 +68,7 @@ func Command() *cobra.Command {
 		cmd.Flags().StringVar(&options.OIDC.ClientID, "oidc-client-id", "", "The client ID for the OpenID Connect client")
 		cmd.Flags().StringVar(&options.OIDC.ClientSecret, "oidc-client-secret", "", "The client secret to use with OpenID Connect issuer")
 		cmd.Flags().StringVar(&options.OIDC.RedirectURL, "oidc-redirect-url", "", "The OAuth2 redirect URL")
-		cmd.Flags().StringVar(&options.OIDC.CookieDuration, "oidc-cookie-duration", "1h", "The duration of the ID token cookie. It should be set in the format: number + time unit (s,m,h) e.g., 20m")
+		cmd.Flags().DurationVar(&options.OIDC.CookieDuration, "oidc-cookie-duration", time.Hour, "The duration of the ID token cookie. It should be set in the format: number + time unit (s,m,h) e.g., 20m")
 	}
 
 	return cmd
@@ -111,7 +112,7 @@ func runCmd(cmd *cobra.Command, args []string) error {
 
 	profilesConfig := server.NewProfilesConfig(rawClient, options.HelmRepoNamespace, options.HelmRepoName)
 
-	var authConfig *auth.AuthConfig
+	var authServer *auth.AuthServer
 
 	if server.AuthEnabled() {
 		_, err := url.Parse(options.OIDC.IssuerURL)
@@ -129,27 +130,31 @@ func runCmd(cmd *cobra.Command, args []string) error {
 			oidcIssueSecureCookies = true
 		}
 
-		oidcCookieDuration, err := time.ParseDuration(options.OIDC.CookieDuration)
+		srv, err := auth.NewAuthServer(cmd.Context(), appConfig.Logger, http.DefaultClient,
+			auth.AuthConfig{
+				OIDCConfig: auth.OIDCConfig{
+					IssuerURL:    options.OIDC.IssuerURL,
+					ClientID:     options.OIDC.ClientID,
+					ClientSecret: options.OIDC.ClientSecret,
+					RedirectURL:  options.OIDC.RedirectURL,
+				},
+				CookieConfig: auth.CookieConfig{
+					CookieDuration:     options.OIDC.CookieDuration,
+					IssueSecureCookies: oidcIssueSecureCookies,
+				},
+			},
+		)
 		if err != nil {
-			return fmt.Errorf("invalid cookie duration: %w", err)
+			return fmt.Errorf("could not create auth server: %w", err)
 		}
 
-		cfg, err := auth.NewAuthConfig(cmd.Context(), options.OIDC.IssuerURL,
-			options.OIDC.ClientID, options.OIDC.ClientSecret, options.OIDC.RedirectURL,
-			oidcIssueSecureCookies, oidcCookieDuration, http.DefaultClient,
-			appConfig.Logger)
-		if err != nil {
-			return fmt.Errorf("could not create auth config: %w", err)
-		}
-
-		cfg.Logger().Info("Registering callback route")
-		// Register /callback handler with mux
-		auth.RegisterAuthHandler(mux, "/oauth2", cfg)
+		appConfig.Logger.Info("Registering callback route")
+		auth.RegisterAuthServer(mux, "/oauth2", srv)
 
-		authConfig = cfg
+		authServer = srv
 	}
 
-	appAndProfilesHandlers, err := server.NewHandlers(context.Background(), &server.Config{AppConfig: appConfig, ProfilesConfig: profilesConfig, AuthConfig: authConfig})
+	appAndProfilesHandlers, err := server.NewHandlers(context.Background(), &server.Config{AppConfig: appConfig, ProfilesConfig: profilesConfig, AuthServer: authServer})
 	if err != nil {
 		return fmt.Errorf("could not create handler: %w", err)
 	}
@@ -164,7 +169,7 @@ func runCmd(cmd *cobra.Command, args []string) error {
 		// Redirect all non-file requests to index.html, where the JS routing will take over.
 		if extension == "" {
 			if server.AuthEnabled() {
-				auth.WithWebAuth(redirector, authConfig).ServeHTTP(w, req)
+				auth.WithWebAuth(redirector, authServer).ServeHTTP(w, req)
 			} else {
 				redirector(w, req)
 			}
@@ -173,7 +178,7 @@ func runCmd(cmd *cobra.Command, args []string) error {
 		assetHandler.ServeHTTP(w, req)
 	}))
 
-	addr := "0.0.0.0:" + options.Port
+	addr := net.JoinHostPort("0.0.0.0", options.Port)
 	srv := &http.Server{
 		Addr:    addr,
 		Handler: mux,

pkg/server/auth/auth.go

@@ -19,16 +19,16 @@ const (
 	// token.
 	RefreshTokenCookieName = "refresh_token"
 	// ScopeProfile is the "profile" scope
-	ScopeProfile = "profile"
+	scopeProfile = "profile"
 	// ScopeEmail is the "email" scope
-	ScopeEmail = "email"
+	scopeEmail = "email"
 )
 
-// RegisterAuthHandler registers the /callback route under a specified prefix.
+// RegisterAuthServer registers the /callback route under a specified prefix.
 // This route is called by the OIDC Provider in order to pass back state after
 // the authentication flow completes.
-func RegisterAuthHandler(mux *http.ServeMux, prefix string, cfg *AuthConfig) {
-	mux.Handle(prefix+"/callback", cfg.callback())
+func RegisterAuthServer(mux *http.ServeMux, prefix string, srv *AuthServer) {
+	mux.Handle(prefix+"/callback", srv)
 }
 
 type principalCtxKey struct{}
@@ -47,17 +47,19 @@ func WithPrincipal(ctx context.Context, p *UserPrincipal) context.Context {
 // WithAPIAuth middleware adds auth validation to API handlers.
 //
 // Unauthorized requests will be denied with a 401 status code.
-func WithAPIAuth(next http.Handler, cfg *AuthConfig) http.Handler {
-	cookieAuth := NewJWTCookiePrincipalGetter(cfg.logger,
-		cfg.verifier(), IDTokenCookieName)
-	headerAuth := NewJWTAuthorizationHeaderPrincipalGetter(cfg.logger, cfg.verifier())
+func WithAPIAuth(next http.Handler, srv *AuthServer) http.Handler {
+	cookieAuth := NewJWTCookiePrincipalGetter(srv.logger,
+		srv.verifier(), IDTokenCookieName)
+	headerAuth := NewJWTAuthorizationHeaderPrincipalGetter(srv.logger, srv.verifier())
+	multi := MultiAuthPrincipal{cookieAuth, headerAuth}
 
 	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-		principal, err := MultiAuthPrincipal{cookieAuth, headerAuth}.Principal(r)
-		if err != nil || principal == nil {
-			cfg.logger.Error(err, "failed to get principal")
+		principal, err := multi.Principal(r)
+		if err != nil {
+			srv.logger.Error(err, "failed to get principal")
+		}
 
-			rw.Header().Set("WWW-Authenticate", `Bearer realm="Weave GitOps"`)
+		if principal == nil || err != nil {
 			http.Error(rw, "Authentication required", http.StatusUnauthorized)
 			return
 		}
@@ -71,26 +73,29 @@ func WithAPIAuth(next http.Handler, cfg *AuthConfig) http.Handler {
 // Unauthorized requests will be redirected to the OIDC Provider.
 // It is meant to be used with routes that serve HTML content,
 // not API routes.
-func WithWebAuth(next http.Handler, cfg *AuthConfig) http.Handler {
-	cookieAuth := NewJWTCookiePrincipalGetter(cfg.logger,
-		cfg.verifier(), IDTokenCookieName)
-	headerAuth := NewJWTAuthorizationHeaderPrincipalGetter(cfg.logger, cfg.verifier())
+func WithWebAuth(next http.Handler, srv *AuthServer) http.Handler {
+	cookieAuth := NewJWTCookiePrincipalGetter(srv.logger,
+		srv.verifier(), IDTokenCookieName)
+	headerAuth := NewJWTAuthorizationHeaderPrincipalGetter(srv.logger, srv.verifier())
+	multi := MultiAuthPrincipal{cookieAuth, headerAuth}
 
 	return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
-		principal, err := MultiAuthPrincipal{cookieAuth, headerAuth}.Principal(r)
-		if err != nil || principal == nil {
-			cfg.logger.Error(err, "failed to get principal")
+		principal, err := multi.Principal(r)
+		if err != nil {
+			srv.logger.Error(err, "failed to get principal")
+		}
 
-			startAuthFlow(rw, r, cfg)
+		if principal == nil || err != nil {
+			startAuthFlow(rw, r, srv)
 			return
 		}
 
 		next.ServeHTTP(rw, r.Clone(WithPrincipal(r.Context(), principal)))
 	})
 }
 
-func startAuthFlow(rw http.ResponseWriter, r *http.Request, cfg *AuthConfig) {
-	nonce, err := generateNonce(32)
+func startAuthFlow(rw http.ResponseWriter, r *http.Request, srv *AuthServer) {
+	nonce, err := generateNonce()
 	if err != nil {
 		http.Error(rw, fmt.Sprintf("failed to generate nonce: %v", err), http.StatusInternalServerError)
 		return
@@ -109,17 +114,17 @@ func startAuthFlow(rw http.ResponseWriter, r *http.Request, cfg *AuthConfig) {
 
 	var scopes []string
 	// "openid", "offline_access" and "email" scopes added by default
-	scopes = append(scopes, ScopeProfile)
-	authCodeUrl := cfg.oauth2Config(scopes).AuthCodeURL(state)
+	scopes = append(scopes, scopeProfile)
+	authCodeUrl := srv.oauth2Config(scopes).AuthCodeURL(state)
 
 	// Issue state cookie
-	http.SetCookie(rw, cfg.createCookie(StateCookieName, state))
+	http.SetCookie(rw, srv.createCookie(StateCookieName, state))
 
 	http.Redirect(rw, r, authCodeUrl, http.StatusSeeOther)
 }
 
-func generateNonce(n int) (string, error) {
-	b := make([]byte, n)
+func generateNonce() (string, error) {
+	b := make([]byte, 32)
 
 	_, err := rand.Read(b)
 	if err != nil {

pkg/server/auth/auth_test.go

@@ -31,22 +31,34 @@ func TestWithAPIAuthReturns401ForUnauthenticatedRequests(t *testing.T) {
 	fake := m.Config()
 	mux := http.NewServeMux()
 
-	c, err := auth.NewAuthConfig(ctx, fake.Issuer, fake.ClientID, fake.ClientSecret, "", false, 20*time.Minute, http.DefaultClient, logr.Discard())
+	srv, err := auth.NewAuthServer(ctx, logr.Discard(), http.DefaultClient,
+		auth.AuthConfig{
+			auth.OIDCConfig{
+				IssuerURL:    fake.Issuer,
+				ClientID:     fake.ClientID,
+				ClientSecret: fake.ClientSecret,
+				RedirectURL:  "",
+			},
+			auth.CookieConfig{
+				CookieDuration:     20 * time.Minute,
+				IssueSecureCookies: false,
+			},
+		})
 	if err != nil {
 		t.Error("failed to create auth config")
 	}
 
-	auth.RegisterAuthHandler(mux, "/oauth2", c)
+	auth.RegisterAuthServer(mux, "/oauth2", srv)
 
 	s := httptest.NewServer(mux)
 	defer s.Close()
 
 	// Set the correct redirect URL now that we have a server running
-	c.SetRedirectURL(s.URL + "/oauth2/callback")
+	srv.SetRedirectURL(s.URL + "/oauth2/callback")
 
 	res := httptest.NewRecorder()
 	req := httptest.NewRequest(http.MethodGet, s.URL, nil)
-	auth.WithAPIAuth(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {}), c).ServeHTTP(res, req)
+	auth.WithAPIAuth(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {}), srv).ServeHTTP(res, req)
 
 	if res.Result().StatusCode != http.StatusUnauthorized {
 		t.Errorf("expected status of %d but got %d", http.StatusUnauthorized, res.Result().StatusCode)
@@ -68,29 +80,41 @@ func TestWithWebAuthRedirectsToOIDCIssuerForUnauthenticatedRequests(t *testing.T
 	fake := m.Config()
 	mux := http.NewServeMux()
 
-	c, err := auth.NewAuthConfig(ctx, fake.Issuer, fake.ClientID, fake.ClientSecret, "", false, 20*time.Minute, http.DefaultClient, logr.Discard())
+	srv, err := auth.NewAuthServer(ctx, logr.Discard(), http.DefaultClient,
+		auth.AuthConfig{
+			auth.OIDCConfig{
+				IssuerURL:    fake.Issuer,
+				ClientID:     fake.ClientID,
+				ClientSecret: fake.ClientSecret,
+				RedirectURL:  "",
+			},
+			auth.CookieConfig{
+				CookieDuration:     20 * time.Minute,
+				IssueSecureCookies: false,
+			},
+		})
 	if err != nil {
 		t.Error("failed to create auth config")
 	}
 
-	auth.RegisterAuthHandler(mux, "/oauth2", c)
+	auth.RegisterAuthServer(mux, "/oauth2", srv)
 
 	s := httptest.NewServer(mux)
 	defer s.Close()
 
 	// Set the correct redirect URL now that we have a server running
 	redirectURL := s.URL + "/oauth2/callback"
-	c.SetRedirectURL(redirectURL)
+	srv.SetRedirectURL(redirectURL)
 
 	res := httptest.NewRecorder()
 	req := httptest.NewRequest(http.MethodGet, s.URL, nil)
-	auth.WithWebAuth(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {}), c).ServeHTTP(res, req)
+	auth.WithWebAuth(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {}), srv).ServeHTTP(res, req)
 
 	if res.Result().StatusCode != http.StatusSeeOther {
 		t.Errorf("expected status of %d but got %d", http.StatusSeeOther, res.Result().StatusCode)
 	}
 
-	authCodeURL := fmt.Sprintf("%s?client_id=%s&redirect_uri=%s&response_type=code&scope=%s", m.AuthorizationEndpoint(), fake.ClientID, url.QueryEscape(redirectURL), strings.Join([]string{auth.ScopeProfile, oidc.ScopeOpenID, oidc.ScopeOfflineAccess, auth.ScopeEmail}, "+"))
+	authCodeURL := fmt.Sprintf("%s?client_id=%s&redirect_uri=%s&response_type=code&scope=%s", m.AuthorizationEndpoint(), fake.ClientID, url.QueryEscape(redirectURL), strings.Join([]string{"profile", oidc.ScopeOpenID, oidc.ScopeOfflineAccess, "email"}, "+"))
 	if !strings.HasPrefix(res.Result().Header.Get("Location"), authCodeURL) {
 		t.Errorf("expected Location header URL to include scopes %s but does not: %s", authCodeURL, res.Result().Header.Get("Location"))
 	}