From 76b4724d15621caa651bcc878c6a73984cc59680 Mon Sep 17 00:00:00 2001 From: Jo Date: Tue, 3 Feb 2026 10:24:57 +0100 Subject: [PATCH] Auth: Remove ssoSettingsLDAP feature toggle (#117216) * remove ldap sso ft * add test cases for lbac disabled * remove legacy ldap pages * fix tests * fix: add ldap provider to TestService_List expected results --- .../feature-toggles/index.md | 1 - .../src/types/featureToggles.gen.ts | 5 --- pkg/services/authn/authnimpl/registration.go | 10 ++--- pkg/services/authn/clients/ldap.go | 9 ++++ pkg/services/authn/clients/ldap_test.go | 30 ++++++++++++- pkg/services/authn/clients/password.go | 4 ++ pkg/services/featuremgmt/registry.go | 8 ---- pkg/services/featuremgmt/toggles_gen.csv | 1 - pkg/services/featuremgmt/toggles_gen.go | 4 -- pkg/services/featuremgmt/toggles_gen.json | 3 +- pkg/services/ldap/service/fake.go | 15 ++++--- pkg/services/ldap/service/ldap.go | 42 +++++++------------ .../ssosettings/ssosettingsimpl/service.go | 8 +--- .../ssosettingsimpl/service_test.go | 22 ++++++++-- public/app/app.ts | 3 -- .../features/admin/ldap/LdapSettingsPage.tsx | 13 ------ public/app/features/auth-config/index.ts | 34 --------------- public/app/routes/routes.tsx | 9 ++-- public/locales/en-US/grafana.json | 2 - 19 files changed, 95 insertions(+), 128 deletions(-) diff --git a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md index e8e63e6ff4b..81087f68d62 100644 --- a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md +++ b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md @@ -42,7 +42,6 @@ Most [generally available](https://grafana.com/docs/release-life-cycle/#general- | `cloudWatchNewLabelParsing` | Updates CloudWatch label parsing to be more accurate | Yes | | `pluginProxyPreserveTrailingSlash` | Preserve plugin proxy trailing slash. | | | `azureMonitorPrometheusExemplars` | Allows configuration of Azure Monitor as a data source that can provide Prometheus exemplars | Yes | -| `ssoSettingsLDAP` | Use the new SSO Settings API to configure LDAP | Yes | | `cloudWatchRoundUpEndTime` | Round up end time for metric queries to the next minute to avoid missing data | Yes | | `newFiltersUI` | Enables new combobox style UI for the Ad hoc filters variable in scenes architecture | Yes | | `alertingQueryAndExpressionsStepMode` | Enables step mode for alerting queries and expressions | Yes | diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index 38ea2af3b47..2d831b3a0bb 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -639,11 +639,6 @@ export interface FeatureToggles { */ authZGRPCServer?: boolean; /** - * Use the new SSO Settings API to configure LDAP - * @default true - */ - ssoSettingsLDAP?: boolean; - /** * Use openFGA as authorization engine. * @default false */ diff --git a/pkg/services/authn/authnimpl/registration.go b/pkg/services/authn/authnimpl/registration.go index 19cbe411b78..852ad7b31f5 100644 --- a/pkg/services/authn/authnimpl/registration.go +++ b/pkg/services/authn/authnimpl/registration.go @@ -57,13 +57,9 @@ func ProvideRegistration( var proxyClients []authn.ProxyClient var passwordClients []authn.PasswordClient - // always register LDAP if LDAP is enabled in SSO settings - //nolint:staticcheck // not yet migrated to OpenFeature - if cfg.LDAPAuthEnabled || features.IsEnabledGlobally(featuremgmt.FlagSsoSettingsLDAP) { - ldap := clients.ProvideLDAP(cfg, ldapService, userService, authInfoService, tracer) - proxyClients = append(proxyClients, ldap) - passwordClients = append(passwordClients, ldap) - } + ldap := clients.ProvideLDAP(cfg, ldapService, userService, authInfoService, tracer) + proxyClients = append(proxyClients, ldap) + passwordClients = append(passwordClients, ldap) if !cfg.DisableLogin { grafana := clients.ProvideGrafana(cfg, userService, tracer) diff --git a/pkg/services/authn/clients/ldap.go b/pkg/services/authn/clients/ldap.go index 4f9cbccf066..d2d7cbfb7c1 100644 --- a/pkg/services/authn/clients/ldap.go +++ b/pkg/services/authn/clients/ldap.go @@ -20,6 +20,7 @@ var _ authn.PasswordClient = new(LDAP) type ldapService interface { Login(query *login.LoginUserQuery) (*login.ExternalUserInfo, error) User(username string) (*login.ExternalUserInfo, error) + Enabled() bool } func ProvideLDAP(cfg *setting.Cfg, ldapService ldapService, userService user.Service, authInfoService login.AuthInfoService, tracer trace.Tracer) *LDAP { @@ -40,6 +41,10 @@ func (c *LDAP) String() string { } func (c *LDAP) AuthenticateProxy(ctx context.Context, r *authn.Request, username string, _ map[string]string) (*authn.Identity, error) { + if !c.service.Enabled() { + return nil, nil + } + ctx, span := c.tracer.Start(ctx, "authn.ldap.AuthenticateProxy") defer span.End() info, err := c.service.User(username) @@ -55,6 +60,10 @@ func (c *LDAP) AuthenticateProxy(ctx context.Context, r *authn.Request, username } func (c *LDAP) AuthenticatePassword(ctx context.Context, r *authn.Request, username, password string) (*authn.Identity, error) { + if !c.service.Enabled() { + return nil, nil + } + ctx, span := c.tracer.Start(ctx, "authn.ldap.AuthenticatePassword") defer span.End() info, err := c.service.Login(&login.LoginUserQuery{ diff --git a/pkg/services/authn/clients/ldap_test.go b/pkg/services/authn/clients/ldap_test.go index 2d09ef5c933..f772d0fe4ba 100644 --- a/pkg/services/authn/clients/ldap_test.go +++ b/pkg/services/authn/clients/ldap_test.go @@ -37,6 +37,20 @@ type ldapTestCase struct { expectDisable bool } +func TestLDAP_AuthenticateProxy_Disabled(t *testing.T) { + c := ProvideLDAP( + setting.NewCfg(), + &service.LDAPFakeService{ExpectedEnabled: false}, + &usertest.FakeUserService{}, + &authinfotest.FakeService{}, + tracing.InitializeTracerForTest(), + ) + + identity, err := c.AuthenticateProxy(context.Background(), &authn.Request{OrgID: 1}, "test", nil) + assert.NoError(t, err) + assert.Nil(t, identity) +} + func TestLDAP_AuthenticateProxy(t *testing.T) { tests := []ldapTestCase{ { @@ -105,6 +119,20 @@ func TestLDAP_AuthenticateProxy(t *testing.T) { } } +func TestLDAP_AuthenticatePassword_Disabled(t *testing.T) { + c := ProvideLDAP( + setting.NewCfg(), + &service.LDAPFakeService{ExpectedEnabled: false}, + &usertest.FakeUserService{}, + &authinfotest.FakeService{}, + tracing.InitializeTracerForTest(), + ) + + identity, err := c.AuthenticatePassword(context.Background(), &authn.Request{OrgID: 1}, "test", "password") + assert.NoError(t, err) + assert.Nil(t, identity) +} + func TestLDAP_AuthenticatePassword(t *testing.T) { tests := []ldapTestCase{ { @@ -199,7 +227,7 @@ func setupLDAPTestCase(tt *ldapTestCase) *LDAP { c := ProvideLDAP( setting.NewCfg(), - &service.LDAPFakeService{ExpectedUser: tt.expectedLDAPInfo, ExpectedError: tt.expectedLDAPErr}, + &service.LDAPFakeService{ExpectedUser: tt.expectedLDAPInfo, ExpectedError: tt.expectedLDAPErr, ExpectedEnabled: true}, userService, authInfoService, tracing.InitializeTracerForTest(), diff --git a/pkg/services/authn/clients/password.go b/pkg/services/authn/clients/password.go index f9b2483d2c5..5cb1f6524b0 100644 --- a/pkg/services/authn/clients/password.go +++ b/pkg/services/authn/clients/password.go @@ -66,6 +66,10 @@ func (c *Password) AuthenticatePassword(ctx context.Context, r *authn.Request, u continue } + if identity == nil { + continue + } + return identity, nil } diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 9a1abc20545..50e803509a9 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -996,14 +996,6 @@ var ( HideFromDocs: true, Expression: "false", }, - { - Name: "ssoSettingsLDAP", - Description: "Use the new SSO Settings API to configure LDAP", - Stage: FeatureStageGeneralAvailability, - Owner: identityAccessTeam, - RequiresRestart: true, - Expression: "true", // enabled by default - }, { Name: "zanzana", Description: "Use openFGA as authorization engine.", diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index 79067fc1c67..27080cb7f70 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -123,7 +123,6 @@ Created,Name,Stage,Owner,requiresDevMode,RequiresRestart,FrontendOnly 2024-06-05,pluginProxyPreserveTrailingSlash,GA,@grafana/plugins-platform-backend,false,false,false 2024-06-06,azureMonitorPrometheusExemplars,GA,@grafana/partner-datasources,false,false,false 2024-06-13,authZGRPCServer,experimental,@grafana/identity-access-team,false,false,false -2024-06-18,ssoSettingsLDAP,GA,@grafana/identity-access-team,false,true,false 2024-06-19,zanzana,experimental,@grafana/identity-access-team,false,false,false 2025-10-21,zanzanaNoLegacyClient,experimental,@grafana/identity-access-team,false,false,false 2024-10-25,reloadDashboardsOnParamsChange,experimental,@grafana/dashboards-squad,false,false,false diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index 1284d0d0d5a..3f320d1e08b 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -379,10 +379,6 @@ const ( // Enables the gRPC server for authorization FlagAuthZGRPCServer = "authZGRPCServer" - // FlagSsoSettingsLDAP - // Use the new SSO Settings API to configure LDAP - FlagSsoSettingsLDAP = "ssoSettingsLDAP" - // FlagZanzana // Use openFGA as authorization engine. FlagZanzana = "zanzana" diff --git a/pkg/services/featuremgmt/toggles_gen.json b/pkg/services/featuremgmt/toggles_gen.json index 2cbd394c00c..192da137131 100644 --- a/pkg/services/featuremgmt/toggles_gen.json +++ b/pkg/services/featuremgmt/toggles_gen.json @@ -4262,7 +4262,8 @@ "metadata": { "name": "ssoSettingsLDAP", "resourceVersion": "1764664939750", - "creationTimestamp": "2024-06-18T11:31:27Z" + "creationTimestamp": "2024-06-18T11:31:27Z", + "deletionTimestamp": "2026-01-31T01:56:50Z" }, "spec": { "description": "Use the new SSO Settings API to configure LDAP", diff --git a/pkg/services/ldap/service/fake.go b/pkg/services/ldap/service/fake.go index c6aa168ae37..869d428b77e 100644 --- a/pkg/services/ldap/service/fake.go +++ b/pkg/services/ldap/service/fake.go @@ -7,11 +7,12 @@ import ( ) type LDAPFakeService struct { - ExpectedConfig *ldap.ServersConfig - ExpectedClient multildap.IMultiLDAP - ExpectedError error - ExpectedUser *login.ExternalUserInfo - UserCalled bool + ExpectedConfig *ldap.ServersConfig + ExpectedClient multildap.IMultiLDAP + ExpectedError error + ExpectedUser *login.ExternalUserInfo + ExpectedEnabled bool + UserCalled bool } func NewLDAPFakeService() *LDAPFakeService { @@ -22,6 +23,10 @@ func (s *LDAPFakeService) ReloadConfig() error { return s.ExpectedError } +func (s *LDAPFakeService) Enabled() bool { + return s.ExpectedEnabled +} + func (s *LDAPFakeService) Config() *ldap.ServersConfig { return s.ExpectedConfig } diff --git a/pkg/services/ldap/service/ldap.go b/pkg/services/ldap/service/ldap.go index a9598331096..d7d258f7c0a 100644 --- a/pkg/services/ldap/service/ldap.go +++ b/pkg/services/ldap/service/ldap.go @@ -28,6 +28,7 @@ var ( type LDAP interface { ReloadConfig() error Config() *ldap.ServersConfig + Enabled() bool Client() multildap.IMultiLDAP // Login authenticates the user against the LDAP server. @@ -56,36 +57,19 @@ func ProvideService(cfg *setting.Cfg, features featuremgmt.FeatureToggles, ssoSe ssoSettings: ssoSettings, } - //nolint:staticcheck // not yet migrated to OpenFeature - if s.features.IsEnabledGlobally(featuremgmt.FlagSsoSettingsLDAP) { - s.ssoSettings.RegisterReloadable(social.LDAPProviderName, s) + s.ssoSettings.RegisterReloadable(social.LDAPProviderName, s) - ldapSettings, err := s.ssoSettings.GetForProvider(context.Background(), social.LDAPProviderName) - if err != nil { - s.log.Error("Failed to retrieve LDAP settings from SSO settings service", "error", err) - return s - } - - err = s.Reload(context.Background(), *ldapSettings) - if err != nil { - s.log.Error("Failed to load LDAP settings", "error", err) - return s - } - } else { - s.cfg = ldap.GetLDAPConfig(cfg) - if !cfg.LDAPAuthEnabled { - return s - } - - ldapCfg, err := multildap.GetConfig(s.cfg) - if err != nil { - s.log.Error("Failed to get LDAP config", "error", err) - } else { - s.ldapCfg = ldapCfg - s.client = multildap.New(s.ldapCfg.Servers, s.cfg) - } + ldapSettings, err := s.ssoSettings.GetForProvider(context.Background(), social.LDAPProviderName) + if err != nil { + s.log.Error("Failed to retrieve LDAP settings from SSO settings service", "error", err) + return s } + err = s.Reload(context.Background(), *ldapSettings) + if err != nil { + s.log.Error("Failed to load LDAP settings", "error", err) + return s + } return s } @@ -138,6 +122,10 @@ func (s *LDAPImpl) Reload(ctx context.Context, settings models.SSOSettings) erro return nil } +func (s *LDAPImpl) Enabled() bool { + return s.cfg != nil && s.cfg.Enabled +} + func (s *LDAPImpl) Validate(ctx context.Context, settings models.SSOSettings, oldSettings models.SSOSettings, requester identity.Requester) error { ldapCfg, err := resolveServerConfig(settings.Settings["config"]) if err != nil { diff --git a/pkg/services/ssosettings/ssosettingsimpl/service.go b/pkg/services/ssosettings/ssosettingsimpl/service.go index 66a7e02eb69..e346fa36abe 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service.go @@ -64,12 +64,8 @@ func ProvideService(cfg *setting.Cfg, sqlStore db.DB, ac ac.AccessControl, } providersList := ssosettings.AllOAuthProviders - - //nolint:staticcheck // not yet migrated to OpenFeature - if features.IsEnabledGlobally(featuremgmt.FlagSsoSettingsLDAP) { - providersList = append(providersList, social.LDAPProviderName) - configurableProviders[social.LDAPProviderName] = true - } + providersList = append(providersList, social.LDAPProviderName) + configurableProviders[social.LDAPProviderName] = true if licensing.FeatureEnabled(social.SAMLProviderName) { fbStrategies = append(fbStrategies, strategies.NewSAMLStrategy(settingsProvider)) diff --git a/pkg/services/ssosettings/ssosettingsimpl/service_test.go b/pkg/services/ssosettings/ssosettingsimpl/service_test.go index d803b14fd6d..3d6c41b43a4 100644 --- a/pkg/services/ssosettings/ssosettingsimpl/service_test.go +++ b/pkg/services/ssosettings/ssosettingsimpl/service_test.go @@ -911,6 +911,11 @@ func TestService_List(t *testing.T) { Settings: map[string]any{"enabled": false}, Source: models.System, }, + { + Provider: "ldap", + Settings: map[string]any(nil), + Source: models.System, + }, }, wantErr: false, }, @@ -1093,6 +1098,11 @@ func TestService_ListWithRedactedSecrets(t *testing.T) { }, Source: models.System, }, + { + Provider: "ldap", + Settings: map[string]any{}, + Source: models.System, + }, }, wantErr: false, }, @@ -1213,6 +1223,11 @@ func TestService_ListWithRedactedSecrets(t *testing.T) { }, Source: models.System, }, + { + Provider: "ldap", + Settings: map[string]any{}, + Source: models.System, + }, }, wantErr: false, }, @@ -2104,6 +2119,7 @@ func Test_ProviderService(t *testing.T) { "grafana_com", "azuread", "okta", + "ldap", }, strategiesLength: 2, }, @@ -2118,6 +2134,7 @@ func Test_ProviderService(t *testing.T) { "grafana_com", "azuread", "okta", + "ldap", "saml", }, strategiesLength: 3, @@ -2136,7 +2153,7 @@ func Test_ProviderService(t *testing.T) { } } -func setupTestEnv(t *testing.T, isLicensingEnabled, keepFallbackStratergies bool, ldapEnabled bool) testEnv { +func setupTestEnv(t *testing.T, isLicensingEnabled, keepFallbackStratergies bool, _ bool) testEnv { t.Helper() store := ssosettingstests.NewFakeStore() @@ -2167,9 +2184,6 @@ func setupTestEnv(t *testing.T, isLicensingEnabled, keepFallbackStratergies bool licensing.On("FeatureEnabled", "saml").Return(isLicensingEnabled) features := make([]any, 0) - if ldapEnabled { - features = append(features, featuremgmt.FlagSsoSettingsLDAP) - } featureManager := featuremgmt.WithManager(features...) svc := ProvideService( diff --git a/public/app/app.ts b/public/app/app.ts index eb871882501..b70fa0d1928 100644 --- a/public/app/app.ts +++ b/public/app/app.ts @@ -83,7 +83,6 @@ import { initEchoSrv } from './core/services/echo/init'; import { KeybindingSrv } from './core/services/keybindingSrv'; import { startMeasure, stopMeasure } from './core/utils/metrics'; import { initAlerting } from './features/alerting/unified/initAlerting'; -import { initAuthConfig } from './features/auth-config'; import { getTimeSrv } from './features/dashboard/services/TimeSrv'; import { EmbeddedDashboardLazy } from './features/dashboard-scene/embedding/EmbeddedDashboardLazy'; import { DashboardLevelTimeMacro } from './features/dashboard-scene/scene/DashboardLevelTimeMacro'; @@ -191,8 +190,6 @@ export class GrafanaApp { initGrafanaLive(); setCurrentUser(contextSrv.user); - initAuthConfig(); - // Expose the app-wide eventbus setAppEvents(appEvents); diff --git a/public/app/features/admin/ldap/LdapSettingsPage.tsx b/public/app/features/admin/ldap/LdapSettingsPage.tsx index 3d597481e17..ce69ecb0880 100644 --- a/public/app/features/admin/ldap/LdapSettingsPage.tsx +++ b/public/app/features/admin/ldap/LdapSettingsPage.tsx @@ -150,19 +150,6 @@ export const LdapSettingsPage = () => { init(); }, [reset]); // eslint-disable-line react-hooks/exhaustive-deps - /** - * Display warning if the feature flag is disabled - */ - if (!config.featureToggles.ssoSettingsLDAP) { - return ( - - - This page is only accessible by enabling the ssoSettingsLDAP feature flag. - - - ); - } - /** * Fetches the settings from the backend * @returns Promise diff --git a/public/app/features/auth-config/index.ts b/public/app/features/auth-config/index.ts index 03a7146a983..7fb7fb6e185 100644 --- a/public/app/features/auth-config/index.ts +++ b/public/app/features/auth-config/index.ts @@ -1,7 +1,3 @@ -import config from 'app/core/config'; -import { getBackendSrv } from 'app/core/services/backend_srv'; -import { contextSrv } from 'app/core/services/context_srv'; -import { AccessControlAction } from 'app/types/accessControl'; import { Settings, SettingsSection } from 'app/types/settings'; import { AuthProviderInfo, GetStatusHook, AuthProviderStatus } from './types'; @@ -49,33 +45,3 @@ export async function getAuthProviderStatus(providerId: string): Promise { - if (contextSrv.hasPermission(AccessControlAction.SettingsRead)) { - const result = await getBackendSrv().get('/api/admin/settings'); - const ldapSettings = result!['auth.ldap'] || {}; - return { - configured: ldapSettings['enabled'] === 'true', - enabled: ldapSettings['enabled'] === 'true', - hide: ldapSettings['enabled'] !== 'true', - }; - } - - return { configured: false, enabled: false }; -} diff --git a/public/app/routes/routes.tsx b/public/app/routes/routes.tsx index 116f0c7613c..f4bef1d2180 100644 --- a/public/app/routes/routes.tsx +++ b/public/app/routes/routes.tsx @@ -6,7 +6,6 @@ import { NavLandingPage } from 'app/core/components/NavLandingPage/NavLandingPag import { PageNotFound } from 'app/core/components/PageNotFound/PageNotFound'; import config from 'app/core/config'; import { contextSrv } from 'app/core/services/context_srv'; -import LdapPage from 'app/features/admin/ldap/LdapPage'; import { getAlertingRoutes } from 'app/features/alerting/routes'; import { isAdmin, isLocalDevEnv, isOpenSourceEdition } from 'app/features/alerting/unified/utils/misc'; import { ConnectionsRedirectNotice } from 'app/features/connections/components/ConnectionsRedirectNotice/ConnectionsRedirectNotice'; @@ -329,11 +328,9 @@ export function getAppRoutes(): RouteDescriptor[] { }, { path: '/admin/authentication/ldap', - component: config.featureToggles.ssoSettingsLDAP - ? SafeDynamicImport( - () => import(/* webpackChunkName: "LdapSettingsPage" */ 'app/features/admin/ldap/LdapSettingsPage') - ) - : LdapPage, + component: SafeDynamicImport( + () => import(/* webpackChunkName: "LdapSettingsPage" */ 'app/features/admin/ldap/LdapSettingsPage') + ), }, { path: '/admin/authentication/:provider', diff --git a/public/locales/en-US/grafana.json b/public/locales/en-US/grafana.json index 4ea5f17a919..61568a01d86 100644 --- a/public/locales/en-US/grafana.json +++ b/public/locales/en-US/grafana.json @@ -162,7 +162,6 @@ }, "ldap-settings-page": { "label-reset-to-default-values": "Reset to default values", - "title-invalid-configuration": "Invalid configuration", "title-more-actions": "More actions", "tooltip-more-actions": "More actions" }, @@ -9793,7 +9792,6 @@ "error-fetching": "Error fetching LDAP settings", "error-saving": "Error saving LDAP settings", "error-validate-form": "Error validating LDAP settings", - "feature-flag-disabled": "This page is only accessible by enabling the ssoSettingsLDAP feature flag.", "saved": "LDAP settings saved" }, "bind-dn": {