[misc] Update CONTRIBUTING.md

CONTRIBUTING.md

@@ -8,7 +8,7 @@ There are many ways that you can contribute:
 - Sharing use cases in slack or Reddit
 - Bug fix or feature enhancement
 
-If you haven't already, join our slack workspace [here](https://join.slack.com/t/netbirdio/shared_invite/zt-vrahf41g-ik1v7fV8du6t0RwxSrJ96A), we would love to discuss topics that need community contribution and enhancements to existing features.
+If you haven't already, join our slack workspace [here](https://docs.netbird.io/slack-url), we would love to discuss topics that need community contribution and enhancements to existing features.
 
 ## Contents
 

idp/dex/connector.go

@@ -89,21 +89,33 @@ func (p *Provider) ListConnectors(ctx context.Context) ([]*ConnectorConfig, erro
 }
 
 // UpdateConnector updates an existing connector in Dex storage.
-// It merges incoming updates with existing values to prevent data loss on partial updates.
+// It overlays user-mutable config fields (issuer, clientID, clientSecret,
+// redirectURI) onto the stored connector config, and updates the connector name
+// when cfg.Name is set. Empty fields on cfg leave stored values unchanged, so
+// partial updates preserve create-time defaults such as scopes, claimMapping,
+// and userIDKey.
 func (p *Provider) UpdateConnector(ctx context.Context, cfg *ConnectorConfig) error {
 	if err := p.storage.UpdateConnector(ctx, cfg.ID, func(old storage.Connector) (storage.Connector, error) {
-		oldCfg, err := p.parseStorageConnector(old)
-		if err != nil {
-			return storage.Connector{}, fmt.Errorf("failed to parse existing connector: %w", err)
+		if cfg.Type != "" && cfg.Type != inferIdentityProviderType(old.Type, cfg.ID, nil) {
+			return storage.Connector{}, errors.New("connector type change not allowed")
 		}
 
-		mergeConnectorConfig(cfg, oldCfg)
-
-		storageConn, err := p.buildStorageConnector(cfg)
+		configData, err := overlayConnectorConfig(old.Config, cfg)
 		if err != nil {
-			return storage.Connector{}, fmt.Errorf("failed to build connector: %w", err)
+			return storage.Connector{}, fmt.Errorf("failed to overlay connector config: %w", err)
 		}
-		return storageConn, nil
+
+		name := cfg.Name
+		if name == "" {
+			name = old.Name
+		}
+
+		return storage.Connector{
+			ID:     cfg.ID,
+			Type:   old.Type,
+			Name:   name,
+			Config: configData,
+		}, nil
 	}); err != nil {
 		return fmt.Errorf("failed to update connector: %w", err)
 	}
@@ -112,23 +124,27 @@ func (p *Provider) UpdateConnector(ctx context.Context, cfg *ConnectorConfig) er
 	return nil
 }
 
-// mergeConnectorConfig preserves existing values for empty fields in the update.
-func mergeConnectorConfig(cfg, oldCfg *ConnectorConfig) {
-	if cfg.ClientSecret == "" {
-		cfg.ClientSecret = oldCfg.ClientSecret
+// overlayConnectorConfig writes only the user-mutable fields onto the existing
+// stored config, preserving every other field (scopes, claimMapping, userIDKey,
+// insecure flags, etc.). Empty fields on cfg leave the existing value alone.
+func overlayConnectorConfig(oldConfig []byte, cfg *ConnectorConfig) ([]byte, error) {
+	var m map[string]any
+	if err := decodeConnectorConfig(oldConfig, &m); err != nil {
+		return nil, err
 	}
-	if cfg.RedirectURI == "" {
-		cfg.RedirectURI = oldCfg.RedirectURI
+	if cfg.Issuer != "" {
+		m["issuer"] = cfg.Issuer
 	}
-	if cfg.Issuer == "" && cfg.Type == oldCfg.Type {
-		cfg.Issuer = oldCfg.Issuer
+	if cfg.ClientID != "" {
+		m["clientID"] = cfg.ClientID
 	}
-	if cfg.ClientID == "" {
-		cfg.ClientID = oldCfg.ClientID
+	if cfg.ClientSecret != "" {
+		m["clientSecret"] = cfg.ClientSecret
 	}
-	if cfg.Name == "" {
-		cfg.Name = oldCfg.Name
+	if cfg.RedirectURI != "" {
+		m["redirectURI"] = cfg.RedirectURI
 	}
+	return encodeConnectorConfig(m)
 }
 
 // DeleteConnector removes a connector from Dex storage.
@@ -216,6 +232,10 @@ func buildOIDCConnectorConfig(cfg *ConnectorConfig, redirectURI string) ([]byte,
 		oidcConfig["getUserInfo"] = true
 	case "entra":
 		oidcConfig["claimMapping"] = map[string]string{"email": "preferred_username"}
+		// Use the Entra Object ID (oid) instead of the default OIDC sub claim.
+		// Entra issues sub as a per-app pairwise identifier that does not match
+		// the stable Object ID.
+		oidcConfig["userIDKey"] = "oid"
 	case "okta":
 		oidcConfig["scopes"] = []string{"openid", "profile", "email", "groups"}
 	case "pocketid":

idp/dex/connector_test.go

@@ -0,0 +1,205 @@
+package dex
+
+import (
+	"context"
+	"encoding/json"
+	"log/slog"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/dexidp/dex/storage"
+	"github.com/dexidp/dex/storage/sql"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func newTestProvider(t *testing.T) (*Provider, func()) {
+	t.Helper()
+	tmpDir, err := os.MkdirTemp("", "dex-connector-test-*")
+	require.NoError(t, err)
+
+	logger := slog.New(slog.NewTextHandler(os.Stderr, nil))
+	s, err := (&sql.SQLite3{File: filepath.Join(tmpDir, "dex.db")}).Open(logger)
+	require.NoError(t, err)
+
+	return &Provider{storage: s, logger: logger}, func() {
+		_ = s.Close()
+		_ = os.RemoveAll(tmpDir)
+	}
+}
+
+func TestBuildOIDCConnectorConfig_EntraSetsUserIDKey(t *testing.T) {
+	cfg := &ConnectorConfig{
+		ID:           "entra-test",
+		Name:         "Entra",
+		Type:         "entra",
+		Issuer:       "https://login.microsoftonline.com/tid/v2.0",
+		ClientID:     "client-id",
+		ClientSecret: "client-secret",
+	}
+	data, err := buildOIDCConnectorConfig(cfg, "https://example.com/oauth2/callback")
+	require.NoError(t, err)
+
+	var m map[string]any
+	require.NoError(t, json.Unmarshal(data, &m))
+
+	assert.Equal(t, "oid", m["userIDKey"], "entra connectors must default userIDKey to oid")
+	assert.Equal(t, map[string]any{"email": "preferred_username"}, m["claimMapping"])
+}
+
+func TestBuildOIDCConnectorConfig_NonEntraDoesNotSetUserIDKey(t *testing.T) {
+	// ensures the Entra userIDKey override does not leak into other OIDC providers,
+	// which already use a stable sub claim.
+	for _, typ := range []string{"oidc", "zitadel", "okta", "pocketid", "authentik", "keycloak", "adfs"} {
+		t.Run(typ, func(t *testing.T) {
+			data, err := buildOIDCConnectorConfig(&ConnectorConfig{Type: typ}, "https://example.com/oauth2/callback")
+			require.NoError(t, err)
+			var m map[string]any
+			require.NoError(t, json.Unmarshal(data, &m))
+			_, ok := m["userIDKey"]
+			assert.False(t, ok, "%s connectors must not have userIDKey set", typ)
+		})
+	}
+}
+
+func TestUpdateConnector_PreservesCreateTimeDefaults(t *testing.T) {
+	ctx := context.Background()
+	p, cleanup := newTestProvider(t)
+	defer cleanup()
+
+	created, err := p.CreateConnector(ctx, &ConnectorConfig{
+		ID:           "entra-test",
+		Name:         "Entra",
+		Type:         "entra",
+		Issuer:       "https://login.microsoftonline.com/tid/v2.0",
+		ClientID:     "client-id",
+		ClientSecret: "old-secret",
+		RedirectURI:  "https://example.com/oauth2/callback",
+	})
+	require.NoError(t, err)
+	require.Equal(t, "entra-test", created.ID)
+
+	// Rotate only the client secret.
+	err = p.UpdateConnector(ctx, &ConnectorConfig{
+		ID:           "entra-test",
+		Type:         "entra",
+		ClientSecret: "new-secret",
+	})
+	require.NoError(t, err)
+
+	conn, err := p.storage.GetConnector(ctx, "entra-test")
+	require.NoError(t, err)
+	var m map[string]any
+	require.NoError(t, json.Unmarshal(conn.Config, &m))
+
+	assert.Equal(t, "new-secret", m["clientSecret"], "clientSecret should be rotated")
+	assert.Equal(t, "client-id", m["clientID"], "clientID must survive (overlay should leave it alone)")
+	assert.Equal(t, "https://login.microsoftonline.com/tid/v2.0", m["issuer"])
+	assert.Equal(t, "oid", m["userIDKey"], "userIDKey must survive update")
+	assert.Equal(t, map[string]any{"email": "preferred_username"}, m["claimMapping"], "claimMapping must survive update")
+}
+
+func TestUpdateConnector_DoesNotAddUserIDKeyToExistingConnector(t *testing.T) {
+	ctx := context.Background()
+	p, cleanup := newTestProvider(t)
+	defer cleanup()
+
+	// Seed a connector directly into storage without userIDKey
+	preFixConfig, err := json.Marshal(map[string]any{
+		"issuer":       "https://login.microsoftonline.com/tid/v2.0",
+		"clientID":     "client-id",
+		"clientSecret": "old-secret",
+		"redirectURI":  "https://example.com/oauth2/callback",
+		"scopes":       []string{"openid", "profile", "email"},
+		"claimMapping": map[string]string{"email": "preferred_username"},
+	})
+	require.NoError(t, err)
+
+	require.NoError(t, p.storage.CreateConnector(ctx, storage.Connector{
+		ID:     "entra-prefix",
+		Type:   "oidc",
+		Name:   "Entra",
+		Config: preFixConfig,
+	}))
+
+	// Rotate client secret via UpdateConnector.
+	err = p.UpdateConnector(ctx, &ConnectorConfig{
+		ID:           "entra-prefix",
+		Type:         "entra",
+		ClientSecret: "new-secret",
+	})
+	require.NoError(t, err)
+
+	conn, err := p.storage.GetConnector(ctx, "entra-prefix")
+	require.NoError(t, err)
+	var m map[string]any
+	require.NoError(t, json.Unmarshal(conn.Config, &m))
+
+	assert.Equal(t, "new-secret", m["clientSecret"])
+	_, has := m["userIDKey"]
+	assert.False(t, has, "userIDKey must not be auto-added to a connector that did not have it before")
+}
+
+func TestUpdateConnector_RejectsTypeChange(t *testing.T) {
+	ctx := context.Background()
+	p, cleanup := newTestProvider(t)
+	defer cleanup()
+
+	_, err := p.CreateConnector(ctx, &ConnectorConfig{
+		ID:           "entra-test",
+		Name:         "Entra",
+		Type:         "entra",
+		Issuer:       "https://login.microsoftonline.com/tid/v2.0",
+		ClientID:     "client-id",
+		ClientSecret: "secret",
+		RedirectURI:  "https://example.com/oauth2/callback",
+	})
+	require.NoError(t, err)
+
+	// Attempt to switch the connector to okta.
+	err = p.UpdateConnector(ctx, &ConnectorConfig{
+		ID:   "entra-test",
+		Type: "okta",
+	})
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "connector type change not allowed")
+
+	// stored connector type/config unchanged after the rejected update.
+	conn, err := p.storage.GetConnector(ctx, "entra-test")
+	require.NoError(t, err)
+	assert.Equal(t, "oidc", conn.Type)
+	var m map[string]any
+	require.NoError(t, json.Unmarshal(conn.Config, &m))
+	assert.Equal(t, "oid", m["userIDKey"])
+}
+
+func TestUpdateConnector_AllowsSameTypeUpdate(t *testing.T) {
+	ctx := context.Background()
+	p, cleanup := newTestProvider(t)
+	defer cleanup()
+
+	_, err := p.CreateConnector(ctx, &ConnectorConfig{
+		ID:           "entra-test",
+		Name:         "Entra",
+		Type:         "entra",
+		Issuer:       "https://login.microsoftonline.com/old/v2.0",
+		ClientID:     "client-id",
+		ClientSecret: "secret",
+		RedirectURI:  "https://example.com/oauth2/callback",
+	})
+	require.NoError(t, err)
+
+	err = p.UpdateConnector(ctx, &ConnectorConfig{
+		ID:     "entra-test",
+		Type:   "entra",
+		Issuer: "https://login.microsoftonline.com/new/v2.0",
+	})
+	require.NoError(t, err)
+
+	conn, err := p.storage.GetConnector(ctx, "entra-test")
+	require.NoError(t, err)
+	var m map[string]any
+	require.NoError(t, json.Unmarshal(conn.Config, &m))
+	assert.Equal(t, "https://login.microsoftonline.com/new/v2.0", m["issuer"])
+}