Skip to content

Commit

Permalink
Auth: Fix orgrole picker disabled if isSynced user (#64033)
Browse files Browse the repository at this point in the history
* fix: disable orgrolepicker if externaluser is synced

* add disable to role picker

* just took me 2 hours to center the icon

* wip

* fix: check externallySyncedUser for API call

* remove check from store

* add: tests

* refactor authproxy and made tests run

* add: feature toggle

* set feature toggle for tests

* add: IsProviderEnabled

* refactor: featuretoggle name

* IsProviderEnabled tests

* add specific tests for isProviderEnabled

* fix: org_user tests

* add: owner to featuretoggle

* add missing authlabels

* remove fmt

* feature toggle

* change config

* add test for a different authmodule

* test refactor

* gen feature toggle again

* fix basic auth user able to change the org role

* test for basic auth role

* make err.base to error

* lowered lvl of log and input mesg

(cherry picked from commit 3cd952b)
  • Loading branch information
eleijonmarck committed Mar 29, 2023
1 parent 266a740 commit ee8d60d
Show file tree
Hide file tree
Showing 27 changed files with 675 additions and 92 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ Alpha features might be changed or removed without prior notice.
| `alertingBacktesting` | Rule backtesting API for alerting |
| `editPanelCSVDragAndDrop` | Enables drag and drop for CSV and Excel files |
| `logsContextDatasourceUi` | Allow datasource to provide custom UI for context view |
| `lokiQuerySplitting` | Split large interval queries into subqueries with smaller time intervals |
| `lokiQuerySplittingConfig` | Give users the option to configure split durations for Loki queries |
| `individualCookiePreferences` | Support overriding cookie preferences per user |
| `onlyExternalOrgRoleSync` | Prohibits a user from changing organization roles synced with external auth providers |
| `drawerDataSourcePicker` | Changes the user experience for data source selection to a drawer. |
| `traceqlSearch` | Enables the 'TraceQL Search' tab for the Tempo datasource which provides a UI to generate TraceQL queries |
| `prometheusMetricEncyclopedia` | Replaces the Prometheus query builder metric select option with a paginated and filterable component |
| `influxdbBackendMigration` | Query InfluxDB InfluxQL without the proxy |

Expand Down
6 changes: 6 additions & 0 deletions packages/grafana-data/src/types/featureToggles.gen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ export interface FeatureToggles {
topNavCommandPalette?: boolean;
logsSampleInExplore?: boolean;
logsContextDatasourceUi?: boolean;
lokiQuerySplitting?: boolean;
lokiQuerySplittingConfig?: boolean;
individualCookiePreferences?: boolean;
onlyExternalOrgRoleSync?: boolean;
drawerDataSourcePicker?: boolean;
traceqlSearch?: boolean;
prometheusMetricEncyclopedia?: boolean;
influxdbBackendMigration?: boolean;
}
4 changes: 2 additions & 2 deletions pkg/api/frontendsettings.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ func (hs *HTTPServer) getFrontendSettingsMap(c *contextmodel.ReqContext) (map[st
"appSubUrl": hs.Cfg.AppSubURL,
"allowOrgCreate": (setting.AllowUserOrgCreate && c.IsSignedIn) || c.IsGrafanaAdmin,
"authProxyEnabled": setting.AuthProxyEnabled,
"ldapEnabled": hs.Cfg.LDAPEnabled,
"ldapEnabled": hs.Cfg.LDAPAuthEnabled,
"jwtHeaderName": hs.Cfg.JWTAuthHeaderName,
"jwtUrlLogin": hs.Cfg.JWTAuthURLLogin,
"alertingEnabled": setting.AlertingEnabled,
Expand Down Expand Up @@ -148,7 +148,7 @@ func (hs *HTTPServer) getFrontendSettingsMap(c *contextmodel.ReqContext) (map[st
"OAuthSkipOrgRoleUpdateSync": hs.Cfg.OAuthSkipOrgRoleUpdateSync,
"SAMLSkipOrgRoleSync": hs.Cfg.SectionWithEnvOverrides("auth.saml").Key("skip_org_role_sync").MustBool(false),
"LDAPSkipOrgRoleSync": hs.Cfg.LDAPSkipOrgRoleSync,
"GithubSkipOrgRoleSync": hs.Cfg.GithubSkipOrgRoleSync,
"GithubSkipOrgRoleSync": hs.Cfg.GitHubSkipOrgRoleSync,
"GoogleSkipOrgRoleSync": hs.Cfg.GoogleSkipOrgRoleSync,
"JWTAuthSkipOrgRoleSync": hs.Cfg.JWTAuthSkipOrgRoleSync,
"GrafanaComSkipOrgRoleSync": hs.Cfg.GrafanaComSkipOrgRoleSync,
Expand Down
26 changes: 13 additions & 13 deletions pkg/api/ldap_debug_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,9 @@ func getUserFromLDAPContext(t *testing.T, requestURL string, searchOrgRst []*org

sc := setupScenarioContext(t, requestURL)

origLDAP := setting.LDAPEnabled
setting.LDAPEnabled = true
t.Cleanup(func() { setting.LDAPEnabled = origLDAP })
origLDAP := setting.LDAPAuthEnabled
setting.LDAPAuthEnabled = true
t.Cleanup(func() { setting.LDAPAuthEnabled = origLDAP })

hs := &HTTPServer{Cfg: setting.NewCfg(), ldapGroups: ldap.ProvideGroupsService(), orgService: &orgtest.FakeOrgService{ExpectedOrgs: searchOrgRst}}

Expand Down Expand Up @@ -313,9 +313,9 @@ func getLDAPStatusContext(t *testing.T) *scenarioContext {
requestURL := "/api/admin/ldap/status"
sc := setupScenarioContext(t, requestURL)

ldap := setting.LDAPEnabled
setting.LDAPEnabled = true
t.Cleanup(func() { setting.LDAPEnabled = ldap })
ldap := setting.LDAPAuthEnabled
setting.LDAPAuthEnabled = true
t.Cleanup(func() { setting.LDAPAuthEnabled = ldap })

hs := &HTTPServer{Cfg: setting.NewCfg()}

Expand Down Expand Up @@ -373,11 +373,11 @@ func postSyncUserWithLDAPContext(t *testing.T, requestURL string, preHook func(*
sc := setupScenarioContext(t, requestURL)
sc.authInfoService = &logintest.AuthInfoServiceFake{}

ldap := setting.LDAPEnabled
ldap := setting.LDAPAuthEnabled
t.Cleanup(func() {
setting.LDAPEnabled = ldap
setting.LDAPAuthEnabled = ldap
})
setting.LDAPEnabled = true
setting.LDAPAuthEnabled = true

hs := &HTTPServer{
Cfg: sc.cfg,
Expand Down Expand Up @@ -600,22 +600,22 @@ func TestLDAP_AccessControl(t *testing.T) {

for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
enabled := setting.LDAPEnabled
enabled := setting.LDAPAuthEnabled
configFile := setting.LDAPConfigFile

t.Cleanup(func() {
setting.LDAPEnabled = enabled
setting.LDAPAuthEnabled = enabled
setting.LDAPConfigFile = configFile
})

setting.LDAPEnabled = true
setting.LDAPAuthEnabled = true
path, err := filepath.Abs("../../conf/ldap.toml")
assert.NoError(t, err)
setting.LDAPConfigFile = path

server := SetupAPITestServer(t, func(hs *HTTPServer) {
cfg := setting.NewCfg()
cfg.LDAPEnabled = true
cfg.LDAPAuthEnabled = true
hs.Cfg = cfg
hs.SQLStore = dbtest.NewFakeDB()
hs.orgService = orgtest.NewOrgServiceFake()
Expand Down
19 changes: 19 additions & 0 deletions pkg/api/org_users.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -296,6 +297,7 @@ func (hs *HTTPServer) searchOrgUsersHelper(c *contextmodel.ReqContext, query *or

userIDs[fmt.Sprint(user.UserID)] = true
authLabelsUserIDs = append(authLabelsUserIDs, user.UserID)

filteredUsers = append(filteredUsers, user)
}

Expand All @@ -313,6 +315,7 @@ func (hs *HTTPServer) searchOrgUsersHelper(c *contextmodel.ReqContext, query *or
filteredUsers[i].AccessControl = accessControlMetadata[fmt.Sprint(filteredUsers[i].UserID)]
if module, ok := modules[filteredUsers[i].UserID]; ok {
filteredUsers[i].AuthLabels = []string{login.GetAuthProviderLabel(module)}
filteredUsers[i].IsExternallySynced = login.IsExternallySynced(hs.Cfg, module)
}
}

Expand Down Expand Up @@ -386,6 +389,22 @@ func (hs *HTTPServer) updateOrgUserHelper(c *contextmodel.ReqContext, cmd org.Up
if !c.OrgRole.Includes(cmd.Role) && !c.IsGrafanaAdmin {
return response.Error(http.StatusForbidden, "Cannot assign a role higher than user's role", nil)
}
if hs.Features.IsEnabled(featuremgmt.FlagOnlyExternalOrgRoleSync) {
// we do not allow to change role for external synced users
qAuth := login.GetAuthInfoQuery{UserId: cmd.UserID}
err := hs.authInfoService.GetAuthInfo(c.Req.Context(), &qAuth)
if err != nil {
if errors.Is(err, user.ErrUserNotFound) {
hs.log.Debug("Failed to get user auth info for basic auth user", cmd.UserID, nil)
} else {
hs.log.Error("Failed to get user auth info for external sync check", cmd.UserID, err)
return response.Error(http.StatusInternalServerError, "Failed to get user auth info", nil)
}
}
if qAuth.Result != nil && qAuth.Result.AuthModule != "" && login.IsExternallySynced(hs.Cfg, qAuth.Result.AuthModule) {
return response.Err(org.ErrCannotChangeRoleForExternallySyncedUser.Errorf("Cannot change role for externally synced user"))
}
}
if err := hs.orgService.UpdateOrgUser(c.Req.Context(), &cmd); err != nil {
if errors.Is(err, org.ErrLastOrgAdmin) {
return response.Error(400, "Cannot change role so that there is no organization admin left", nil)
Expand Down
99 changes: 99 additions & 0 deletions pkg/api/org_users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,13 @@ import (
"github.com/grafana/grafana/pkg/infra/db"
"github.com/grafana/grafana/pkg/infra/db/dbtest"
"github.com/grafana/grafana/pkg/infra/localcache"
"github.com/grafana/grafana/pkg/models/roletype"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/login/logintest"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/org/orgimpl"
"github.com/grafana/grafana/pkg/services/org/orgtest"
Expand All @@ -30,8 +34,10 @@ import (
"github.com/grafana/grafana/pkg/services/temp_user/tempuserimpl"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/services/user/userimpl"
"github.com/grafana/grafana/pkg/services/user/usertest"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/util"
"github.com/grafana/grafana/pkg/web/webtest"
)

func setUpGetOrgUsersDB(t *testing.T, sqlStore *sqlstore.SQLStore) {
Expand Down Expand Up @@ -200,6 +206,99 @@ 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
}
permissions := []accesscontrol.Permission{
{Action: accesscontrol.ActionOrgUsersRead, Scope: "users:*"},
{Action: accesscontrol.ActionOrgUsersWrite, Scope: "users:*"},
{Action: accesscontrol.ActionOrgUsersAdd, Scope: "users:*"},
{Action: accesscontrol.ActionOrgUsersRemove, Scope: "users:*"},
}
tests := []testCase{
{
desc: "should be able to change basicRole when skip_org_role_sync true",
SkipOrgRoleSync: true,
AuthEnabled: true,
AuthModule: login.LDAPAuthModule,
expectedCode: http.StatusOK,
},
{
desc: "should not be able to change basicRole when skip_org_role_sync false",
SkipOrgRoleSync: false,
AuthEnabled: true,
AuthModule: login.LDAPAuthModule,
expectedCode: http.StatusForbidden,
},
{
desc: "should not be able to change basicRole with a different provider",
SkipOrgRoleSync: false,
AuthEnabled: true,
AuthModule: login.GenericOAuthModule,
expectedCode: http.StatusForbidden,
},
{
desc: "should be able to change basicRole with a basic Auth",
SkipOrgRoleSync: false,
AuthEnabled: false,
AuthModule: "",
expectedCode: http.StatusOK,
},
{
desc: "should be able to change basicRole with a basic Auth",
SkipOrgRoleSync: true,
AuthEnabled: true,
AuthModule: "",
expectedCode: http.StatusOK,
},
}

userWithPermissions := userWithPermissions(1, permissions)
userRequesting := &user.User{ID: 2, OrgID: 1}
reqBody := `{"userId": "1", "role": "Admin", "orgId": "1"}`
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
server := SetupAPITestServer(t, func(hs *HTTPServer) {
hs.Cfg = setting.NewCfg()
hs.Cfg.LDAPAuthEnabled = tt.AuthEnabled
if tt.AuthModule == login.LDAPAuthModule {
hs.Cfg.LDAPAuthEnabled = tt.AuthEnabled
hs.Cfg.LDAPSkipOrgRoleSync = tt.SkipOrgRoleSync
} else if tt.AuthModule == login.GenericOAuthModule {
hs.Cfg.GenericOAuthAuthEnabled = tt.AuthEnabled
hs.Cfg.GenericOAuthSkipOrgRoleSync = tt.SkipOrgRoleSync
} else if tt.AuthModule == "" {
// authmodule empty means basic auth
} else {
t.Errorf("invalid auth module for test: %s", tt.AuthModule)
}

hs.authInfoService = &logintest.AuthInfoServiceFake{
ExpectedUserAuth: &login.UserAuth{AuthModule: tt.AuthModule},
}
hs.Features = featuremgmt.WithFeatures(featuremgmt.FlagOnlyExternalOrgRoleSync, true)
hs.userService = &usertest.FakeUserService{ExpectedSignedInUser: userWithPermissions}
hs.orgService = &orgtest.FakeOrgService{}
hs.accesscontrolService = &actest.FakeService{
ExpectedPermissions: permissions,
}
})
req := server.NewRequest(http.MethodPatch, fmt.Sprintf("/api/orgs/%d/users/%d", userRequesting.OrgID, userRequesting.ID), strings.NewReader(reqBody))
req.Header.Set("Content-Type", "application/json")
userWithPermissions.OrgRole = roletype.RoleAdmin
res, err := server.Send(webtest.RequestWithSignedInUser(req, userWithPermissions))
require.NoError(t, err)
assert.Equal(t, tt.expectedCode, res.StatusCode)
require.NoError(t, res.Body.Close())
})
}
}

func TestOrgUsersAPIEndpoint_LegacyAccessControl_FolderAdmin(t *testing.T) {
cfg := setting.NewCfg()
cfg.RBACEnabled = false
Expand Down
1 change: 1 addition & 0 deletions pkg/api/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ func (hs *HTTPServer) getUserUserProfile(c *contextmodel.ReqContext, userID int6
authLabel := login.GetAuthProviderLabel(getAuthQuery.Result.AuthModule)
userProfile.AuthLabels = append(userProfile.AuthLabels, authLabel)
userProfile.IsExternal = true
userProfile.IsExternallySynced = login.IsExternallySynced(hs.Cfg, getAuthQuery.Result.AuthModule)
}

userProfile.AccessControl = hs.getAccessControlMetadata(c, c.OrgID, "global.users:id:", strconv.FormatInt(userID, 10))
Expand Down
2 changes: 1 addition & 1 deletion pkg/infra/usagestats/service/usage_stats_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func TestMetrics(t *testing.T) {
BuildVersion: "5.0.0",
AnonymousEnabled: true,
BasicAuthEnabled: true,
LDAPEnabled: true,
LDAPAuthEnabled: true,
AuthProxyEnabled: true,
Packaging: "deb",
ReportingDistributor: "hosted-grafana",
Expand Down
4 changes: 2 additions & 2 deletions pkg/infra/usagestats/statscollector/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func TestCollectingUsageStats(t *testing.T) {
BuildVersion: "5.0.0",
AnonymousEnabled: true,
BasicAuthEnabled: true,
LDAPEnabled: true,
LDAPAuthEnabled: true,
AuthProxyEnabled: true,
Packaging: "deb",
ReportingDistributor: "hosted-grafana",
Expand Down Expand Up @@ -210,7 +210,7 @@ func TestElasticStats(t *testing.T) {
BuildVersion: "5.0.0",
AnonymousEnabled: true,
BasicAuthEnabled: true,
LDAPEnabled: true,
LDAPAuthEnabled: true,
AuthProxyEnabled: true,
Packaging: "deb",
ReportingDistributor: "hosted-grafana",
Expand Down
8 changes: 4 additions & 4 deletions pkg/login/ldap_login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ var errTest = errors.New("test error")

func TestLoginUsingLDAP(t *testing.T) {
LDAPLoginScenario(t, "When LDAP enabled and no server configured", func(sc *LDAPLoginScenarioContext) {
setting.LDAPEnabled = true
setting.LDAPAuthEnabled = true

sc.withLoginResult(false)
getLDAPConfig = func(*setting.Cfg) (*ldap.Config, error) {
Expand All @@ -39,7 +39,7 @@ func TestLoginUsingLDAP(t *testing.T) {
})

LDAPLoginScenario(t, "When LDAP disabled", func(sc *LDAPLoginScenarioContext) {
setting.LDAPEnabled = false
setting.LDAPAuthEnabled = false

sc.withLoginResult(false)
loginService := &logintest.LoginServiceFake{}
Expand Down Expand Up @@ -135,11 +135,11 @@ func LDAPLoginScenario(t *testing.T, desc string, fn LDAPLoginScenarioFunc) {

origNewLDAP := newLDAP
origGetLDAPConfig := getLDAPConfig
origLDAPEnabled := setting.LDAPEnabled
origLDAPEnabled := setting.LDAPAuthEnabled
t.Cleanup(func() {
newLDAP = origNewLDAP
getLDAPConfig = origGetLDAPConfig
setting.LDAPEnabled = origLDAPEnabled
setting.LDAPAuthEnabled = origLDAPEnabled
})

getLDAPConfig = func(*setting.Cfg) (*ldap.Config, error) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/login/social/social.go
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func ProvideService(cfg *setting.Cfg,
apiUrl: info.ApiUrl,
teamIds: sec.Key("team_ids").Ints(","),
allowedOrganizations: util.SplitString(sec.Key("allowed_organizations").String()),
skipOrgRoleSync: cfg.GithubSkipOrgRoleSync,
skipOrgRoleSync: cfg.GitHubSkipOrgRoleSync,
}
}

Expand Down
Loading

0 comments on commit ee8d60d

Please sign in to comment.