From 81af48862bbd6df8415331b3d17d269ffbb6b187 Mon Sep 17 00:00:00 2001 From: Maksim Nabokikh Date: Thu, 1 Aug 2024 21:38:09 +0200 Subject: [PATCH] Remove additional features and add a feature flag instead (#3663) Signed-off-by: m.nabokikh --- cmd/dex/config.go | 30 ------------------------------ cmd/dex/config_test.go | 18 ------------------ cmd/dex/serve.go | 3 +-- examples/config-dev.yaml | 5 ----- pkg/featureflags/set.go | 3 +++ server/api.go | 29 +++++++++++++---------------- server/api_test.go | 37 +++++++++++++++++++++++++------------ server/server.go | 10 ---------- 8 files changed, 42 insertions(+), 93 deletions(-) diff --git a/cmd/dex/config.go b/cmd/dex/config.go index e4c3988ffc..dd6d2e2ab9 100644 --- a/cmd/dex/config.go +++ b/cmd/dex/config.go @@ -8,7 +8,6 @@ import ( "net/http" "net/netip" "os" - "slices" "strings" "golang.org/x/crypto/bcrypt" @@ -52,22 +51,10 @@ type Config struct { // querying the storage. Cannot be specified without enabling a passwords // database. StaticPasswords []password `json:"staticPasswords"` - - // AdditionalFeature allow the extension of Dex functionalities - AdditionalFeatures []server.AdditionalFeature `json:"additionalFeatures"` -} - -// Parse the configuration -func (c *Config) Parse() { - if c.AdditionalFeatures == nil { - c.AdditionalFeatures = []server.AdditionalFeature{} - } } // Validate the configuration func (c Config) Validate() error { - invalidFeatures := c.findInvalidAdditionalFeatures() - // Fast checks. Perform these first for a more responsive CLI. checks := []struct { bad bool @@ -86,7 +73,6 @@ func (c Config) Validate() error { {c.GRPC.TLSKey != "" && c.GRPC.Addr == "", "no address specified for gRPC"}, {(c.GRPC.TLSCert == "") != (c.GRPC.TLSKey == ""), "must specific both a gRPC TLS cert and key"}, {c.GRPC.TLSCert == "" && c.GRPC.TLSClientCA != "", "cannot specify gRPC TLS client CA without a gRPC TLS cert"}, - {len(invalidFeatures) > 0, fmt.Sprintf("invalid additionalFeatures supplied: %v. Valid entries: %s", invalidFeatures, server.ValidAdditionalFeatures)}, {c.GRPC.TLSMinVersion != "" && c.GRPC.TLSMinVersion != "1.2" && c.GRPC.TLSMinVersion != "1.3", "supported TLS versions are: 1.2, 1.3"}, {c.GRPC.TLSMaxVersion != "" && c.GRPC.TLSMaxVersion != "1.2" && c.GRPC.TLSMaxVersion != "1.3", "supported TLS versions are: 1.2, 1.3"}, {c.GRPC.TLSMaxVersion != "" && c.GRPC.TLSMinVersion != "" && c.GRPC.TLSMinVersion > c.GRPC.TLSMaxVersion, "TLSMinVersion greater than TLSMaxVersion"}, @@ -105,22 +91,6 @@ func (c Config) Validate() error { return nil } -// findInvalidAdditionalFeatures returns additional features that are not considered valid -func (c Config) findInvalidAdditionalFeatures() []server.AdditionalFeature { - if c.AdditionalFeatures == nil { - return []server.AdditionalFeature{} - } - - badFeatures := []server.AdditionalFeature{} - for _, feature := range c.AdditionalFeatures { - if !slices.Contains(server.ValidAdditionalFeatures, feature) { - badFeatures = append(badFeatures, feature) - } - } - - return badFeatures -} - type password storage.Password func (p *password) UnmarshalJSON(b []byte) error { diff --git a/cmd/dex/config_test.go b/cmd/dex/config_test.go index 8924931d36..c6d37cb03e 100644 --- a/cmd/dex/config_test.go +++ b/cmd/dex/config_test.go @@ -37,11 +37,8 @@ func TestValidConfiguration(t *testing.T) { Config: &mock.CallbackConfig{}, }, }, - AdditionalFeatures: server.ValidAdditionalFeatures, } - configuration.Parse() - if err := configuration.Validate(); err != nil { t.Fatalf("this configuration should have been valid: %v", err) } @@ -49,7 +46,6 @@ func TestValidConfiguration(t *testing.T) { func TestInvalidConfiguration(t *testing.T) { configuration := Config{} - configuration.Parse() err := configuration.Validate() if err == nil { t.Fatal("this configuration should be invalid") @@ -232,7 +228,6 @@ additionalFeatures: [ Level: slog.LevelDebug, Format: "json", }, - AdditionalFeatures: server.ValidAdditionalFeatures, } var c Config @@ -240,8 +235,6 @@ additionalFeatures: [ t.Fatalf("failed to decode config: %v", err) } - c.Parse() - if diff := pretty.Compare(c, want); diff != "" { t.Errorf("got!=want: %s", diff) } @@ -450,18 +443,7 @@ logger: t.Fatalf("failed to decode config: %v", err) } - c.Parse() - if diff := pretty.Compare(c, want); diff != "" { t.Errorf("got!=want: %s", diff) } } - -func TestParseConfig(t *testing.T) { - configuration := Config{} - configuration.Parse() - - if configuration.AdditionalFeatures == nil || len(configuration.AdditionalFeatures) != 0 { - t.Fatal("AdditionalFeatures should be an empty slice") - } -} diff --git a/cmd/dex/serve.go b/cmd/dex/serve.go index 572da8c97a..6fcca04da3 100644 --- a/cmd/dex/serve.go +++ b/cmd/dex/serve.go @@ -99,7 +99,6 @@ func runServe(options serveOptions) error { return fmt.Errorf("error parse config file %s: %v", configFile, err) } - c.Parse() applyConfigOverrides(options, &c) logger, err := newLogger(c.Logger.Level, c.Logger.Format) @@ -509,7 +508,7 @@ func runServe(options serveOptions) error { } grpcSrv := grpc.NewServer(grpcOptions...) - api.RegisterDexServer(grpcSrv, server.NewAPI(serverConfig.Storage, logger, version, c.AdditionalFeatures)) + api.RegisterDexServer(grpcSrv, server.NewAPI(serverConfig.Storage, logger, version)) grpcMetrics.InitializeMetrics(grpcSrv) if c.GRPC.Reflection { diff --git a/examples/config-dev.yaml b/examples/config-dev.yaml index 47adc04b75..147597a265 100644 --- a/examples/config-dev.yaml +++ b/examples/config-dev.yaml @@ -165,8 +165,3 @@ staticPasswords: hash: "$2a$10$2b2cU8CPhOTaGrs1HRQuAueS7JTT5ZHsHSzYiFPm1leZck7Mc8T4W" username: "admin" userID: "08a8684b-db88-4b73-90a9-3cd1661f5466" - -# A list of features that extend Dex functionalities -# additionalFeatures: -# # allows CRUD operations on connectors through the gRPC API -# - "ConnectorsCRUD" diff --git a/pkg/featureflags/set.go b/pkg/featureflags/set.go index 1a31040ae0..a86c4fa9b3 100644 --- a/pkg/featureflags/set.go +++ b/pkg/featureflags/set.go @@ -8,4 +8,7 @@ var ( // ExpandEnv can enable or disable env expansion in the config which can be useful in environments where, e.g., // $ sign is a part of the password for LDAP user. ExpandEnv = newFlag("expand_env", true) + + // APIConnectorsCRUD allows CRUD operations on connectors through the gRPC API + APIConnectorsCRUD = newFlag("api_connectors_crud", false) ) diff --git a/server/api.go b/server/api.go index 090b412cd4..f53bc60be5 100644 --- a/server/api.go +++ b/server/api.go @@ -6,11 +6,11 @@ import ( "errors" "fmt" "log/slog" - "slices" "golang.org/x/crypto/bcrypt" "github.com/dexidp/dex/api/v2" + "github.com/dexidp/dex/pkg/featureflags" "github.com/dexidp/dex/server/internal" "github.com/dexidp/dex/storage" ) @@ -31,12 +31,11 @@ const ( ) // NewAPI returns a server which implements the gRPC API interface. -func NewAPI(s storage.Storage, logger *slog.Logger, version string, additionalFeatures []AdditionalFeature) api.DexServer { +func NewAPI(s storage.Storage, logger *slog.Logger, version string) api.DexServer { return dexAPI{ - s: s, - logger: logger.With("component", "api"), - version: version, - additionalFeatures: additionalFeatures, + s: s, + logger: logger.With("component", "api"), + version: version, } } @@ -46,8 +45,6 @@ type dexAPI struct { s storage.Storage logger *slog.Logger version string - - additionalFeatures []AdditionalFeature } func (d dexAPI) GetClient(ctx context.Context, req *api.GetClientReq) (*api.GetClientResp, error) { @@ -392,8 +389,8 @@ func (d dexAPI) RevokeRefresh(ctx context.Context, req *api.RevokeRefreshReq) (* } func (d dexAPI) CreateConnector(ctx context.Context, req *api.CreateConnectorReq) (*api.CreateConnectorResp, error) { - if !slices.Contains(d.additionalFeatures, ConnectorsCRUD) { - return nil, fmt.Errorf("%v not provided in addtionalFeatures", ConnectorsCRUD) + if !featureflags.APIConnectorsCRUD.Enabled() { + return nil, fmt.Errorf("%s feature flag is not enabled", featureflags.APIConnectorsCRUD.Name) } if req.Connector.Id == "" { @@ -434,8 +431,8 @@ func (d dexAPI) CreateConnector(ctx context.Context, req *api.CreateConnectorReq } func (d dexAPI) UpdateConnector(ctx context.Context, req *api.UpdateConnectorReq) (*api.UpdateConnectorResp, error) { - if !slices.Contains(d.additionalFeatures, ConnectorsCRUD) { - return nil, fmt.Errorf("%v not provided in addtionalFeatures", ConnectorsCRUD) + if !featureflags.APIConnectorsCRUD.Enabled() { + return nil, fmt.Errorf("%s feature flag is not enabled", featureflags.APIConnectorsCRUD.Name) } if req.Id == "" { @@ -478,8 +475,8 @@ func (d dexAPI) UpdateConnector(ctx context.Context, req *api.UpdateConnectorReq } func (d dexAPI) DeleteConnector(ctx context.Context, req *api.DeleteConnectorReq) (*api.DeleteConnectorResp, error) { - if !slices.Contains(d.additionalFeatures, ConnectorsCRUD) { - return nil, fmt.Errorf("%v not provided in addtionalFeatures", ConnectorsCRUD) + if !featureflags.APIConnectorsCRUD.Enabled() { + return nil, fmt.Errorf("%s feature flag is not enabled", featureflags.APIConnectorsCRUD.Name) } if req.Id == "" { @@ -498,8 +495,8 @@ func (d dexAPI) DeleteConnector(ctx context.Context, req *api.DeleteConnectorReq } func (d dexAPI) ListConnectors(ctx context.Context, req *api.ListConnectorReq) (*api.ListConnectorResp, error) { - if !slices.Contains(d.additionalFeatures, ConnectorsCRUD) { - return nil, fmt.Errorf("%v not provided in addtionalFeatures", ConnectorsCRUD) + if !featureflags.APIConnectorsCRUD.Enabled() { + return nil, fmt.Errorf("%s feature flag is not enabled", featureflags.APIConnectorsCRUD.Name) } connectorList, err := d.s.ListConnectors() diff --git a/server/api_test.go b/server/api_test.go index 88601ec917..bcf240c192 100644 --- a/server/api_test.go +++ b/server/api_test.go @@ -5,6 +5,7 @@ import ( "io" "log/slog" "net" + "os" "strings" "testing" "time" @@ -30,14 +31,14 @@ type apiClient struct { } // newAPI constructs a gRCP client connected to a backing server. -func newAPI(s storage.Storage, logger *slog.Logger, t *testing.T, addtionalFeatures []AdditionalFeature) *apiClient { +func newAPI(s storage.Storage, logger *slog.Logger, t *testing.T) *apiClient { l, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatal(err) } serv := grpc.NewServer() - api.RegisterDexServer(serv, NewAPI(s, logger, "test", addtionalFeatures)) + api.RegisterDexServer(serv, NewAPI(s, logger, "test")) go serv.Serve(l) // NewClient will retry automatically if the serv.Serve() goroutine @@ -62,7 +63,7 @@ func TestPassword(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{}) + client := newAPI(s, logger, t) defer client.Close() ctx := context.Background() @@ -171,7 +172,7 @@ func TestCheckCost(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{}) + client := newAPI(s, logger, t) defer client.Close() tests := []struct { @@ -224,7 +225,7 @@ func TestRefreshToken(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{}) + client := newAPI(s, logger, t) defer client.Close() ctx := context.Background() @@ -333,7 +334,7 @@ func TestUpdateClient(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{}) + client := newAPI(s, logger, t) defer client.Close() ctx := context.Background() @@ -493,10 +494,13 @@ func find(item string, items []string) bool { } func TestCreateConnector(t *testing.T) { + os.Setenv("DEX_API_CONNECTORS_CRUD", "true") + defer os.Unsetenv("DEX_API_CONNECTORS_CRUD") + logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{ConnectorsCRUD}) + client := newAPI(s, logger, t) defer client.Close() ctx := context.Background() @@ -540,10 +544,13 @@ func TestCreateConnector(t *testing.T) { } func TestUpdateConnector(t *testing.T) { + os.Setenv("DEX_API_CONNECTORS_CRUD", "true") + defer os.Unsetenv("DEX_API_CONNECTORS_CRUD") + logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{ConnectorsCRUD}) + client := newAPI(s, logger, t) defer client.Close() ctx := context.Background() @@ -605,10 +612,13 @@ func TestUpdateConnector(t *testing.T) { } func TestDeleteConnector(t *testing.T) { + os.Setenv("DEX_API_CONNECTORS_CRUD", "true") + defer os.Unsetenv("DEX_API_CONNECTORS_CRUD") + logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{ConnectorsCRUD}) + client := newAPI(s, logger, t) defer client.Close() ctx := context.Background() @@ -646,10 +656,13 @@ func TestDeleteConnector(t *testing.T) { } func TestListConnectors(t *testing.T) { + os.Setenv("DEX_API_CONNECTORS_CRUD", "true") + defer os.Unsetenv("DEX_API_CONNECTORS_CRUD") + logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{ConnectorsCRUD}) + client := newAPI(s, logger, t) defer client.Close() ctx := context.Background() @@ -685,11 +698,11 @@ func TestListConnectors(t *testing.T) { } } -func TestMissingAdditionalFeature(t *testing.T) { +func TestMissingConnectorsCRUDFeatureFlag(t *testing.T) { logger := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{})) s := memory.New(logger) - client := newAPI(s, logger, t, []AdditionalFeature{}) + client := newAPI(s, logger, t) defer client.Close() ctx := context.Background() diff --git a/server/server.go b/server/server.go index b447fa3276..1cf71c5038 100644 --- a/server/server.go +++ b/server/server.go @@ -50,16 +50,6 @@ import ( "github.com/dexidp/dex/web" ) -// AdditionalFeature allows the extension of Dex server functionalities -type AdditionalFeature string - -// ConnectorsCRUD is an additional feature that allows CRUD operations on connectors -var ConnectorsCRUD AdditionalFeature = "ConnectorsCRUD" - -var ValidAdditionalFeatures []AdditionalFeature = []AdditionalFeature{ - ConnectorsCRUD, -} - // LocalConnector is the local passwordDB connector which is an internal // connector maintained by the server. const LocalConnector = "local"