Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access Control: Clear user's permission cache after resource creation #59101

Merged
merged 5 commits into from Nov 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 11 additions & 9 deletions pkg/api/common_test.go
Expand Up @@ -25,6 +25,7 @@ import (
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/acimpl"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/accesscontrol/ossaccesscontrol"
"github.com/grafana/grafana/pkg/services/annotations/annotationstest"
Expand Down Expand Up @@ -250,15 +251,16 @@ func (s *fakeRenderService) Init() error {
func setupAccessControlScenarioContext(t *testing.T, cfg *setting.Cfg, url string, permissions []accesscontrol.Permission) (*scenarioContext, *HTTPServer) {
store := sqlstore.InitTestDB(t)
hs := &HTTPServer{
Cfg: cfg,
Live: newTestLive(t, store),
License: &licensing.OSSLicensingService{},
Features: featuremgmt.WithFeatures(),
QuotaService: quotatest.New(false, nil),
RouteRegister: routing.NewRouteRegister(),
AccessControl: accesscontrolmock.New().WithPermissions(permissions),
searchUsersService: searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), usertest.NewUserServiceFake()),
ldapGroups: ldap.ProvideGroupsService(),
Cfg: cfg,
Live: newTestLive(t, store),
License: &licensing.OSSLicensingService{},
Features: featuremgmt.WithFeatures(),
QuotaService: quotatest.New(false, nil),
RouteRegister: routing.NewRouteRegister(),
AccessControl: accesscontrolmock.New().WithPermissions(permissions),
searchUsersService: searchusers.ProvideUsersService(filters.ProvideOSSSearchUserFilter(), usertest.NewUserServiceFake()),
ldapGroups: ldap.ProvideGroupsService(),
accesscontrolService: actest.FakeService{},
}

sc := setupScenarioContext(t, url)
Expand Down
8 changes: 7 additions & 1 deletion pkg/api/dashboard.go
Expand Up @@ -470,14 +470,20 @@ func (hs *HTTPServer) postDashboard(c *models.ReqContext, cmd models.SaveDashboa
}

if liveerr != nil {
hs.log.Warn("unable to broadcast save event", "uid", dashboard.Uid, "error", err)
hs.log.Warn("unable to broadcast save event", "uid", dashboard.Uid, "error", liveerr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to ignore this comment.

I really don't like this variable name, since it reminds me of the human organ liver rather than and error from live.

Suggested change
hs.log.Warn("unable to broadcast save event", "uid", dashboard.Uid, "error", liveerr)
hs.log.Warn("unable to broadcast save event", "uid", dashboard.Uid, "error", liveErr)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, that's pretty funny :) I'll leave it as it is for now, as I didn't introduce this error variable and don't want to add too many changes to this PR. But I'll keep it in mind for later.

}
}

if err != nil {
return apierrors.ToDashboardErrorResponse(ctx, hs.pluginStore, err)
}

// Clear permission cache for the user who's created the dashboard, so that new permissions are fetched for their next call
// Required for cases when caller wants to immediately interact with the newly created object
if newDashboard && !hs.accesscontrolService.IsDisabled() {
hs.accesscontrolService.ClearUserPermissionCache(c.SignedInUser)
}

// connect library panels for this dashboard after the dashboard is stored and has an ID
err = hs.LibraryPanelService.ConnectLibraryPanelsForDashboard(ctx, c.SignedInUser, dashboard)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions pkg/api/dashboard_test.go
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/plugins"
"github.com/grafana/grafana/pkg/registry/corekind"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/alerting"
"github.com/grafana/grafana/pkg/services/annotations/annotationstest"
Expand Down Expand Up @@ -1093,6 +1094,7 @@ func postDashboardScenario(t *testing.T, desc string, url string, routePattern s
folderService: folderService,
Features: featuremgmt.WithFeatures(),
Kinds: corekind.NewBase(nil),
accesscontrolService: actest.FakeService{},
}

sc := setupScenarioContext(t, url)
Expand Down Expand Up @@ -1201,6 +1203,7 @@ func restoreDashboardVersionScenario(t *testing.T, desc string, url string, rout
Features: featuremgmt.WithFeatures(),
dashboardVersionService: fakeDashboardVersionService,
Kinds: corekind.NewBase(nil),
accesscontrolService: actest.FakeService{},
}

sc := setupScenarioContext(t, url)
Expand Down
6 changes: 6 additions & 0 deletions pkg/api/datasources.go
Expand Up @@ -396,6 +396,12 @@ func (hs *HTTPServer) AddDataSource(c *models.ReqContext) response.Response {
return response.Error(500, "Failed to add datasource", err)
}

// Clear permission cache for the user who's created the data source, so that new permissions are fetched for their next call
// Required for cases when caller wants to immediately interact with the newly created object
if !hs.AccessControl.IsDisabled() {
hs.accesscontrolService.ClearUserPermissionCache(c.SignedInUser)
}

ds := hs.convertModelToDtos(c.Req.Context(), cmd.Result)
return response.JSON(http.StatusOK, util.DynMap{
"message": "Datasource added",
Expand Down
10 changes: 8 additions & 2 deletions pkg/api/datasources_test.go
Expand Up @@ -19,6 +19,8 @@ import (
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/plugins"
ac "github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/acimpl"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
"github.com/grafana/grafana/pkg/services/datasources"
"github.com/grafana/grafana/pkg/services/datasources/permissions"
"github.com/grafana/grafana/pkg/services/org"
Expand Down Expand Up @@ -112,7 +114,9 @@ func TestAddDataSource_URLWithoutProtocol(t *testing.T) {
DataSourcesService: &dataSourcesServiceMock{
expectedDatasource: &datasources.DataSource{},
},
Cfg: setting.NewCfg(),
Cfg: setting.NewCfg(),
AccessControl: acimpl.ProvideAccessControl(setting.NewCfg()),
accesscontrolService: actest.FakeService{},
}

sc := setupScenarioContext(t, "/api/datasources")
Expand Down Expand Up @@ -224,7 +228,9 @@ func TestUpdateDataSource_URLWithoutProtocol(t *testing.T) {
DataSourcesService: &dataSourcesServiceMock{
expectedDatasource: &datasources.DataSource{},
},
Cfg: setting.NewCfg(),
Cfg: setting.NewCfg(),
AccessControl: acimpl.ProvideAccessControl(setting.NewCfg()),
accesscontrolService: actest.FakeService{},
}

sc := setupScenarioContext(t, "/api/datasources/1234")
Expand Down
6 changes: 6 additions & 0 deletions pkg/api/folder.go
Expand Up @@ -128,6 +128,12 @@ func (hs *HTTPServer) CreateFolder(c *models.ReqContext) response.Response {
return apierrors.ToFolderErrorResponse(err)
}

// Clear permission cache for the user who's created the folder, so that new permissions are fetched for their next call
// Required for cases when caller wants to immediately interact with the newly created object
if !hs.AccessControl.IsDisabled() {
hs.accesscontrolService.ClearUserPermissionCache(c.SignedInUser)
}

g := guardian.New(c.Req.Context(), folder.ID, c.OrgID, c.SignedInUser)
// TODO set ParentUID if nested folders are enabled
return response.JSON(http.StatusOK, hs.newToFolderDto(c, g, folder))
Expand Down
10 changes: 6 additions & 4 deletions pkg/api/folder_test.go
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/grafana/grafana/pkg/api/routing"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/featuremgmt"
Expand Down Expand Up @@ -242,10 +243,11 @@ func createFolderScenario(t *testing.T, desc string, url string, routePattern st
store := mockstore.NewSQLStoreMock()
guardian.InitLegacyGuardian(store, dashSvc, teamSvc)
hs := HTTPServer{
AccessControl: acmock.New(),
folderService: folderService,
Cfg: setting.NewCfg(),
Features: featuremgmt.WithFeatures(),
AccessControl: acmock.New(),
folderService: folderService,
Cfg: setting.NewCfg(),
Features: featuremgmt.WithFeatures(),
accesscontrolService: actest.FakeService{},
}

sc := setupScenarioContext(t, url)
Expand Down
6 changes: 6 additions & 0 deletions pkg/api/team.go
Expand Up @@ -41,6 +41,12 @@ func (hs *HTTPServer) CreateTeam(c *models.ReqContext) response.Response {
return response.Error(500, "Failed to create Team", err)
}

// Clear permission cache for the user who's created the team, so that new permissions are fetched for their next call
// Required for cases when caller wants to immediately interact with the newly created object
if !hs.AccessControl.IsDisabled() {
hs.accesscontrolService.ClearUserPermissionCache(c.SignedInUser)
}

if accessControlEnabled || (c.OrgRole == org.RoleEditor && hs.Cfg.EditorsCanAdmin) {
// if the request is authenticated using API tokens
// the SignedInUser is an empty struct therefore
Expand Down
4 changes: 4 additions & 0 deletions pkg/api/team_test.go
Expand Up @@ -15,6 +15,8 @@ import (
"github.com/grafana/grafana/pkg/infra/log/logtest"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/acimpl"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
"github.com/grafana/grafana/pkg/services/org"
pref "github.com/grafana/grafana/pkg/services/preference"
"github.com/grafana/grafana/pkg/services/preference/preftest"
Expand Down Expand Up @@ -213,6 +215,8 @@ func TestTeamAPIEndpoint_CreateTeam_RBAC(t *testing.T) {
server := SetupAPITestServer(t, func(hs *HTTPServer) {
hs.Cfg = setting.NewCfg()
hs.teamService = teamtest.NewFakeService()
hs.AccessControl = acimpl.ProvideAccessControl(setting.NewCfg())
hs.accesscontrolService = actest.FakeService{}
})

input := strings.NewReader(fmt.Sprintf(teamCmd, 1))
Expand Down
2 changes: 2 additions & 0 deletions pkg/services/accesscontrol/accesscontrol.go
Expand Up @@ -26,6 +26,8 @@ type Service interface {
registry.ProvidesUsageStats
// GetUserPermissions returns user permissions with only action and scope fields set.
GetUserPermissions(ctx context.Context, user *user.SignedInUser, options Options) ([]Permission, error)
// ClearUserPermissionCache removes the permission cache entry for the given user
ClearUserPermissionCache(user *user.SignedInUser)
// DeleteUserPermissions removes all permissions user has in org and all permission to that user
// If orgID is set to 0 remove permissions from all orgs
DeleteUserPermissions(ctx context.Context, orgID, userID int64) error
Expand Down
8 changes: 8 additions & 0 deletions pkg/services/accesscontrol/acimpl/service.go
Expand Up @@ -147,6 +147,14 @@ func (s *Service) getCachedUserPermissions(ctx context.Context, user *user.Signe
return permissions, nil
}

func (s *Service) ClearUserPermissionCache(user *user.SignedInUser) {
key, err := permissionCacheKey(user)
if err != nil {
return
}
s.cache.Delete(key)
}

func (s *Service) DeleteUserPermissions(ctx context.Context, orgID int64, userID int64) error {
return s.store.DeleteUserPermissions(ctx, orgID, userID)
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/services/accesscontrol/actest/fake.go
Expand Up @@ -24,6 +24,8 @@ func (f FakeService) GetUserPermissions(ctx context.Context, user *user.SignedIn
return f.ExpectedPermissions, f.ExpectedErr
}

func (f FakeService) ClearUserPermissionCache(user *user.SignedInUser) {}

func (f FakeService) DeleteUserPermissions(ctx context.Context, orgID, userID int64) error {
return f.ExpectedErr
}
Expand Down
10 changes: 10 additions & 0 deletions pkg/services/accesscontrol/mock/mock.go
Expand Up @@ -20,6 +20,7 @@ type fullAccessControl interface {
type Calls struct {
Evaluate []interface{}
GetUserPermissions []interface{}
ClearUserPermissionCache []interface{}
IsDisabled []interface{}
DeclareFixedRoles []interface{}
DeclarePluginRoles []interface{}
Expand All @@ -43,6 +44,7 @@ type Mock struct {
// Override functions
EvaluateFunc func(context.Context, *user.SignedInUser, accesscontrol.Evaluator) (bool, error)
GetUserPermissionsFunc func(context.Context, *user.SignedInUser, accesscontrol.Options) ([]accesscontrol.Permission, error)
ClearUserPermissionCacheFunc func(*user.SignedInUser)
IsDisabledFunc func() bool
DeclareFixedRolesFunc func(...accesscontrol.RoleRegistration) error
DeclarePluginRolesFunc func(context.Context, string, string, []plugins.RoleRegistration) error
Expand Down Expand Up @@ -138,6 +140,14 @@ func (m *Mock) GetUserPermissions(ctx context.Context, user *user.SignedInUser,
return m.permissions, nil
}

func (m *Mock) ClearUserPermissionCache(user *user.SignedInUser) {
m.Calls.ClearUserPermissionCache = append(m.Calls.ClearUserPermissionCache, []interface{}{user})
// Use override if provided
if m.ClearUserPermissionCacheFunc != nil {
m.ClearUserPermissionCacheFunc(user)
}
}

// Middleware checks if service disabled or not to switch to fallback authorization.
// This mock return m.disabled unless an override is provided.
func (m *Mock) IsDisabled() bool {
Expand Down
35 changes: 21 additions & 14 deletions pkg/services/serviceaccounts/api/api.go
Expand Up @@ -21,31 +21,34 @@ import (
)

type ServiceAccountsAPI struct {
cfg *setting.Cfg
service serviceaccounts.Service
accesscontrol accesscontrol.AccessControl
RouterRegister routing.RouteRegister
store serviceaccounts.Store
log log.Logger
permissionService accesscontrol.ServiceAccountPermissionsService
cfg *setting.Cfg
service serviceaccounts.Service
accesscontrol accesscontrol.AccessControl
accesscontrolService accesscontrol.Service
RouterRegister routing.RouteRegister
store serviceaccounts.Store
log log.Logger
permissionService accesscontrol.ServiceAccountPermissionsService
}

func NewServiceAccountsAPI(
cfg *setting.Cfg,
service serviceaccounts.Service,
accesscontrol accesscontrol.AccessControl,
accesscontrolService accesscontrol.Service,
routerRegister routing.RouteRegister,
store serviceaccounts.Store,
permissionService accesscontrol.ServiceAccountPermissionsService,
) *ServiceAccountsAPI {
return &ServiceAccountsAPI{
cfg: cfg,
service: service,
accesscontrol: accesscontrol,
RouterRegister: routerRegister,
store: store,
log: log.New("serviceaccounts.api"),
permissionService: permissionService,
cfg: cfg,
service: service,
accesscontrol: accesscontrol,
accesscontrolService: accesscontrolService,
RouterRegister: routerRegister,
store: store,
log: log.New("serviceaccounts.api"),
permissionService: permissionService,
}
}

Expand Down Expand Up @@ -127,6 +130,10 @@ func (api *ServiceAccountsAPI) CreateServiceAccount(c *models.ReqContext) respon
return response.Error(http.StatusInternalServerError, "Failed to set permissions for service account creator", err)
}
}

// Clear permission cache for the user who's created the service account, so that new permissions are fetched for their next call
// Required for cases when caller wants to immediately interact with the newly created object
api.accesscontrolService.ClearUserPermissionCache(c.SignedInUser)
}

return response.JSON(http.StatusCreated, serviceAccount)
Expand Down
4 changes: 3 additions & 1 deletion pkg/services/serviceaccounts/api/api_test.go
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/accesscontrol/ossaccesscontrol"
"github.com/grafana/grafana/pkg/services/apikey/apikeyimpl"
Expand Down Expand Up @@ -296,8 +297,9 @@ func setupTestServer(t *testing.T, svc *tests.ServiceAccountMock,
saPermissionService, err := ossaccesscontrol.ProvideServiceAccountPermissions(
cfg, routing.NewRouteRegister(), sqlStore, acmock, &licensing.OSSLicensingService{}, saStore, acmock, teamSvc, userSvc)
require.NoError(t, err)
acService := actest.FakeService{}

a := NewServiceAccountsAPI(cfg, svc, acmock, routerRegister, saStore, saPermissionService)
a := NewServiceAccountsAPI(cfg, svc, acmock, acService, routerRegister, saStore, saPermissionService)
a.RegisterAPIEndpoints()

a.cfg.ApiKeyMaxSecondsToLive = -1 // disable api key expiration
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/serviceaccounts/manager/service.go
Expand Up @@ -51,7 +51,7 @@ func ProvideServiceAccountsService(

usageStats.RegisterMetricsFunc(s.getUsageMetrics)

serviceaccountsAPI := api.NewServiceAccountsAPI(cfg, s, ac, routeRegister, s.store, permissionService)
serviceaccountsAPI := api.NewServiceAccountsAPI(cfg, s, ac, accesscontrolService, routeRegister, s.store, permissionService)
serviceaccountsAPI.RegisterAPIEndpoints()

s.secretScanEnabled = cfg.SectionWithEnvOverrides("secretscan").Key("enabled").MustBool(false)
Expand Down