Skip to content

Commit

Permalink
Remove additional features and add a feature flag instead (#3663)
Browse files Browse the repository at this point in the history
Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
  • Loading branch information
nabokihms authored Aug 1, 2024
1 parent 2256607 commit 81af488
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 93 deletions.
30 changes: 0 additions & 30 deletions cmd/dex/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"net/http"
"net/netip"
"os"
"slices"
"strings"

"golang.org/x/crypto/bcrypt"
Expand Down Expand Up @@ -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
Expand All @@ -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"},
Expand All @@ -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 {
Expand Down
18 changes: 0 additions & 18 deletions cmd/dex/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,19 +37,15 @@ 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)
}
}

func TestInvalidConfiguration(t *testing.T) {
configuration := Config{}
configuration.Parse()
err := configuration.Validate()
if err == nil {
t.Fatal("this configuration should be invalid")
Expand Down Expand Up @@ -232,16 +228,13 @@ additionalFeatures: [
Level: slog.LevelDebug,
Format: "json",
},
AdditionalFeatures: server.ValidAdditionalFeatures,
}

var c Config
if err := yaml.Unmarshal(rawConfig, &c); err != nil {
t.Fatalf("failed to decode config: %v", err)
}

c.Parse()

if diff := pretty.Compare(c, want); diff != "" {
t.Errorf("got!=want: %s", diff)
}
Expand Down Expand Up @@ -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")
}
}
3 changes: 1 addition & 2 deletions cmd/dex/serve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down
5 changes: 0 additions & 5 deletions examples/config-dev.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
3 changes: 3 additions & 0 deletions pkg/featureflags/set.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
)
29 changes: 13 additions & 16 deletions server/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand All @@ -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,
}
}

Expand All @@ -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) {
Expand Down Expand Up @@ -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 == "" {
Expand Down Expand Up @@ -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 == "" {
Expand Down Expand Up @@ -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 == "" {
Expand All @@ -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()
Expand Down
37 changes: 25 additions & 12 deletions server/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"io"
"log/slog"
"net"
"os"
"strings"
"testing"
"time"
Expand All @@ -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
Expand All @@ -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()
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()

Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
10 changes: 0 additions & 10 deletions server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down

0 comments on commit 81af488

Please sign in to comment.