From f7c6491f7317f3fbaf4f21b8beec967d76778a87 Mon Sep 17 00:00:00 2001 From: Ieva Date: Tue, 25 Jul 2023 13:27:02 +0100 Subject: [PATCH] Auth: add a feature toggle for locking user roles for users synced through GCom (#72202) * add a new feature toggle for locking down role sync for users managed by GCom * protect the frontend and the backend using the new feature toggle * fix merge --- .../feature-toggles/index.md | 1 + .../src/types/featureToggles.gen.ts | 1 + pkg/api/org_users.go | 7 ++++- pkg/api/org_users_test.go | 31 ++++++++++++++++--- pkg/services/featuremgmt/registry.go | 6 ++++ pkg/services/featuremgmt/toggles_gen.csv | 1 + pkg/services/featuremgmt/toggles_gen.go | 4 +++ public/app/features/users/UsersTable.tsx | 11 +++++-- 8 files changed, 54 insertions(+), 8 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 ce97d109cb3..6c421fe54d5 100644 --- a/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md +++ b/docs/sources/setup-grafana/configure-grafana/feature-toggles/index.md @@ -35,6 +35,7 @@ Some features are enabled by default. You can disable these feature by setting t | `disablePrometheusExemplarSampling` | Disable Prometheus exemplar sampling | | | `logsContextDatasourceUi` | Allow datasource to provide custom UI for context view | Yes | | `lokiQuerySplitting` | Split large interval queries into subqueries with smaller time intervals | Yes | +| `gcomOnlyExternalOrgRoleSync` | Prohibits a user from changing organization roles synced with Grafana Cloud auth provider | | | `prometheusMetricEncyclopedia` | Adds the metrics explorer component to the Prometheus query builder as an option in metric select | Yes | | `prometheusDataplane` | Changes responses to from Prometheus to be compliant with the dataplane specification. In particular it sets the numeric Field.Name from 'Value' to the value of the `__name__` label when present. | Yes | | `lokiMetricDataplane` | Changes metric responses from Loki to be compliant with the dataplane specification. | Yes | diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index fa6c5881053..570d7400ec5 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -64,6 +64,7 @@ export interface FeatureToggles { lokiQuerySplitting?: boolean; lokiQuerySplittingConfig?: boolean; individualCookiePreferences?: boolean; + gcomOnlyExternalOrgRoleSync?: boolean; prometheusMetricEncyclopedia?: boolean; timeSeriesTable?: boolean; prometheusResourceBrowserCache?: boolean; diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index 8baa033225f..20b1ba2d7f7 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -11,6 +11,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/services/accesscontrol" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" @@ -401,7 +402,11 @@ func (hs *HTTPServer) updateOrgUserHelper(c *contextmodel.ReqContext, cmd org.Up } } if authInfo != nil && authInfo.AuthModule != "" && login.IsExternallySynced(hs.Cfg, authInfo.AuthModule) { - return response.Err(org.ErrCannotChangeRoleForExternallySyncedUser.Errorf("Cannot change role for externally synced user")) + // A GCom specific feature toggle for role locking has been introduced, as the previous implementation had a bug with locking down external users synced through GCom (https://github.com/grafana/grafana/pull/72044) + // Remove this conditional once FlagGcomOnlyExternalOrgRoleSync feature toggle has been removed + if authInfo.AuthModule != login.GrafanaComAuthModule || hs.Features.IsEnabled(featuremgmt.FlagGcomOnlyExternalOrgRoleSync) { + return response.Err(org.ErrCannotChangeRoleForExternallySyncedUser.Errorf("Cannot change role for externally synced user")) + } } if err := hs.orgService.UpdateOrgUser(c.Req.Context(), &cmd); err != nil { diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index 2d368ecc72b..67c4eafc7d8 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -202,11 +202,12 @@ func TestOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { type testCase struct { - desc string - SkipOrgRoleSync bool - AuthEnabled bool - AuthModule string - expectedCode int + desc string + SkipOrgRoleSync bool + GcomOnlyExternalOrgRoleSync bool + AuthEnabled bool + AuthModule string + expectedCode int } permissions := []accesscontrol.Permission{ {Action: accesscontrol.ActionOrgUsersRead, Scope: "users:*"}, @@ -236,6 +237,22 @@ func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { AuthModule: login.GenericOAuthModule, expectedCode: http.StatusForbidden, }, + { + desc: "should be able to change basicRole for a user synced through GCom if GcomOnlyExternalOrgRoleSync flag is set to false", + SkipOrgRoleSync: false, + GcomOnlyExternalOrgRoleSync: false, + AuthEnabled: true, + AuthModule: login.GrafanaComAuthModule, + expectedCode: http.StatusOK, + }, + { + desc: "should not be able to change basicRole for a user synced through GCom if GcomOnlyExternalOrgRoleSync flag is set to true", + SkipOrgRoleSync: false, + GcomOnlyExternalOrgRoleSync: true, + AuthEnabled: true, + AuthModule: login.GrafanaComAuthModule, + expectedCode: http.StatusForbidden, + }, { desc: "should be able to change basicRole with a basic Auth", SkipOrgRoleSync: false, @@ -266,6 +283,9 @@ func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { } else if tt.AuthModule == login.GenericOAuthModule { hs.Cfg.GenericOAuthAuthEnabled = tt.AuthEnabled hs.Cfg.GenericOAuthSkipOrgRoleSync = tt.SkipOrgRoleSync + } else if tt.AuthModule == login.GrafanaComAuthModule { + hs.Cfg.GrafanaNetAuthEnabled = tt.AuthEnabled + hs.Cfg.GrafanaComSkipOrgRoleSync = tt.SkipOrgRoleSync } else if tt.AuthModule == "" { // authmodule empty means basic auth } else { @@ -275,6 +295,7 @@ func TestOrgUsersAPIEndpoint_updateOrgRole(t *testing.T) { hs.authInfoService = &logintest.AuthInfoServiceFake{ ExpectedUserAuth: &login.UserAuth{AuthModule: tt.AuthModule}, } + hs.Features = featuremgmt.WithFeatures(featuremgmt.FlagGcomOnlyExternalOrgRoleSync, tt.GcomOnlyExternalOrgRoleSync) hs.userService = &usertest.FakeUserService{ExpectedSignedInUser: userWithPermissions} hs.orgService = &orgtest.FakeOrgService{} hs.accesscontrolService = &actest.FakeService{ diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index ae783286245..4708e070ac1 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -316,6 +316,12 @@ var ( Stage: FeatureStageExperimental, Owner: grafanaBackendPlatformSquad, }, + { + Name: "gcomOnlyExternalOrgRoleSync", + Description: "Prohibits a user from changing organization roles synced with Grafana Cloud auth provider", + Stage: FeatureStageGeneralAvailability, + Owner: grafanaAuthnzSquad, + }, { Name: "prometheusMetricEncyclopedia", Description: "Adds the metrics explorer component to the Prometheus query builder as an option in metric select", diff --git a/pkg/services/featuremgmt/toggles_gen.csv b/pkg/services/featuremgmt/toggles_gen.csv index 115ae2fa0db..bf0324be72d 100644 --- a/pkg/services/featuremgmt/toggles_gen.csv +++ b/pkg/services/featuremgmt/toggles_gen.csv @@ -45,6 +45,7 @@ logsContextDatasourceUi,GA,@grafana/observability-logs,false,false,false,true lokiQuerySplitting,GA,@grafana/observability-logs,false,false,false,true lokiQuerySplittingConfig,experimental,@grafana/observability-logs,false,false,false,true individualCookiePreferences,experimental,@grafana/backend-platform,false,false,false,false +gcomOnlyExternalOrgRoleSync,GA,@grafana/grafana-authnz-team,false,false,false,false prometheusMetricEncyclopedia,GA,@grafana/observability-metrics,false,false,false,true timeSeriesTable,experimental,@grafana/app-o11y,false,false,false,true prometheusResourceBrowserCache,experimental,@grafana/observability-metrics,false,false,false,true diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index 9a4d4f556e2..17f4e076494 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -191,6 +191,10 @@ const ( // Support overriding cookie preferences per user FlagIndividualCookiePreferences = "individualCookiePreferences" + // FlagGcomOnlyExternalOrgRoleSync + // Prohibits a user from changing organization roles synced with Grafana Cloud auth provider + FlagGcomOnlyExternalOrgRoleSync = "gcomOnlyExternalOrgRoleSync" + // FlagPrometheusMetricEncyclopedia // Adds the metrics explorer component to the Prometheus query builder as an option in metric select FlagPrometheusMetricEncyclopedia = "prometheusMetricEncyclopedia" diff --git a/public/app/features/users/UsersTable.tsx b/public/app/features/users/UsersTable.tsx index 7c37a653da0..438041cabe9 100644 --- a/public/app/features/users/UsersTable.tsx +++ b/public/app/features/users/UsersTable.tsx @@ -5,6 +5,7 @@ import { Button, ConfirmModal } from '@grafana/ui'; import { UserRolePicker } from 'app/core/components/RolePicker/UserRolePicker'; import { fetchRoleOptions } from 'app/core/components/RolePicker/api'; import { TagBadge } from 'app/core/components/TagFilter/TagBadge'; +import config from 'app/core/config'; import { contextSrv } from 'app/core/core'; import { AccessControlAction, OrgUser, Role } from 'app/types'; @@ -56,8 +57,14 @@ export const UsersTable = ({ users, orgId, onRoleChange, onRemoveUser }: Props) {users.map((user, index) => { let basicRoleDisabled = !contextSrv.hasPermissionInMetadata(AccessControlAction.OrgUsersWrite, user); - const isUserSynced = user?.isExternallySynced; - basicRoleDisabled = isUserSynced || basicRoleDisabled; + let authLabel = Array.isArray(user.authLabels) && user.authLabels.length > 0 ? user.authLabels[0] : ''; + // A GCom specific feature toggle for role locking has been introduced, as the previous implementation had a bug with locking down external users synced through GCom (https://github.com/grafana/grafana/pull/72044) + // Remove this conditional once FlagGcomOnlyExternalOrgRoleSync feature toggle has been removed + if (authLabel !== 'grafana.com' || config.featureToggles.gcomOnlyExternalOrgRoleSync) { + const isUserSynced = user?.isExternallySynced; + basicRoleDisabled = isUserSynced || basicRoleDisabled; + } + return (