Skip to content

Commit

Permalink
Chore: Remove bus from contexthandler (grafana#47374)
Browse files Browse the repository at this point in the history 
* Chore: Remove bus from contexthandler

* fix tests

* try different wire binding

* maybe remove a few more dispatches

* fix tests
  • Loading branch information
@zserge
zserge authored Apr 6, 2022
1 parent 8853fe5 commit d153d89
Show file tree
Hide file tree
Showing 10 changed files with 33 additions and 96 deletions.
13 changes: 5 additions & 8 deletions pkg/middleware/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import (
"testing"

"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/setting"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -39,9 +38,7 @@ func TestMiddlewareAuth(t *testing.T) {

middlewareScenario(t, "ReqSignIn true and NoAnonynmous true", func(
t *testing.T, sc *scenarioContext) {
sqlStore := mockstore.NewSQLStoreMock()
sqlStore.ExpectedOrg = &models.Org{Id: orgID, Name: "test"}
sc.sqlStore = sqlStore
sc.mockSQLStore.ExpectedOrg = &models.Org{Id: orgID, Name: "test"}
sc.m.Get("/api/secure", ReqSignedInNoAnonymous, sc.defaultHandler)
sc.fakeReq("GET", "/api/secure").exec()

Expand All @@ -50,9 +47,7 @@ func TestMiddlewareAuth(t *testing.T) {

middlewareScenario(t, "ReqSignIn true and request with forceLogin in query string", func(
t *testing.T, sc *scenarioContext) {
sqlStore := mockstore.NewSQLStoreMock()
sqlStore.ExpectedOrg = &models.Org{Id: orgID, Name: "test"}
sc.sqlStore = sqlStore
sc.mockSQLStore.ExpectedOrg = &models.Org{Id: orgID, Name: "test"}
sc.m.Get("/secure", reqSignIn, sc.defaultHandler)

sc.fakeReq("GET", "/secure?forceLogin=true").exec()
Expand All @@ -65,7 +60,8 @@ func TestMiddlewareAuth(t *testing.T) {

middlewareScenario(t, "ReqSignIn true and request with same org provided in query string", func(
t *testing.T, sc *scenarioContext) {
org, err := sc.sqlStore.CreateOrgWithMember(sc.cfg.AnonymousOrgName, 1)
sc.mockSQLStore.ExpectedOrg = &models.Org{Id: 1, Name: sc.cfg.AnonymousOrgName}
org, err := sc.mockSQLStore.CreateOrgWithMember(sc.cfg.AnonymousOrgName, 1)
require.NoError(t, err)

sc.m.Get("/secure", reqSignIn, sc.defaultHandler)
Expand All @@ -77,6 +73,7 @@ func TestMiddlewareAuth(t *testing.T) {

middlewareScenario(t, "ReqSignIn true and request with different org provided in query string", func(
t *testing.T, sc *scenarioContext) {
sc.mockSQLStore.ExpectedOrg = &models.Org{Id: 1, Name: sc.cfg.AnonymousOrgName}
sc.m.Get("/secure", reqSignIn, sc.defaultHandler)

sc.fakeReq("GET", "/secure?orgId=2").exec()
Expand Down
15 changes: 2 additions & 13 deletions pkg/middleware/middleware_basic_auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,7 @@ func TestMiddlewareBasicAuth(t *testing.T) {
keyhash, err := util.EncodePassword("v5nAwpMafFP6znaS4urhdWDLS5511M42", "asd")
require.NoError(t, err)

bus.AddHandler("test", func(ctx context.Context, query *models.GetApiKeyByNameQuery) error {
query.Result = &models.ApiKey{OrgId: orgID, Role: models.ROLE_EDITOR, Key: keyhash}
return nil
})
sc.mockSQLStore.ExpectedAPIKey = &models.ApiKey{OrgId: orgID, Role: models.ROLE_EDITOR, Key: keyhash}

authHeader := util.GetBasicAuthHeader("api_key", "eyJrIjoidjVuQXdwTWFmRlA2em5hUzR1cmhkV0RMUzU1MTFNNDIiLCJuIjoiYXNkIiwiaWQiOjF9")
sc.fakeReq("GET", "/").withAuthorizationHeader(authHeader).exec()
Expand Down Expand Up @@ -61,11 +58,7 @@ func TestMiddlewareBasicAuth(t *testing.T) {
return nil
})

bus.AddHandler("get-sign-user", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
t.Log("Handling GetSignedInUserQuery")
query.Result = &models.SignedInUser{OrgId: orgID, UserId: id}
return nil
})
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{OrgId: orgID, UserId: id}

authHeader := util.GetBasicAuthHeader("myUser", password)
sc.fakeReq("GET", "/").withAuthorizationHeader(authHeader).exec()
Expand All @@ -85,10 +78,6 @@ func TestMiddlewareBasicAuth(t *testing.T) {
sc.mockSQLStore.ExpectedUser = &models.User{Password: encoded, Id: id, Salt: salt}
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{UserId: id}
login.ProvideService(sc.mockSQLStore, &logintest.LoginServiceFake{})
bus.AddHandler("get-sign-user", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
query.Result = &models.SignedInUser{UserId: query.UserId}
return nil
})

authHeader := util.GetBasicAuthHeader("myUser", password)
sc.fakeReq("GET", "/").withAuthorizationHeader(authHeader).exec()
Expand Down
31 changes: 4 additions & 27 deletions pkg/middleware/middleware_jwt_auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,7 @@ func TestMiddlewareJWTAuth(t *testing.T) {
"foo-username": myUsername,
}, nil
}
bus.AddHandler("get-sign-user", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
query.Result = &models.SignedInUser{
UserId: id,
OrgId: orgID,
Login: query.Login,
}
return nil
})
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{UserId: id, OrgId: orgID, Login: myUsername}

sc.fakeReq("GET", "/").withJWTAuthHeader(token).exec()
assert.Equal(t, verifiedToken, token)
Expand All @@ -74,14 +67,7 @@ func TestMiddlewareJWTAuth(t *testing.T) {
"foo-email": myEmail,
}, nil
}
bus.AddHandler("get-sign-user", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
query.Result = &models.SignedInUser{
UserId: id,
OrgId: orgID,
Email: query.Email,
}
return nil
})
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{UserId: id, OrgId: orgID, Email: myEmail}

sc.fakeReq("GET", "/").withJWTAuthHeader(token).exec()
assert.Equal(t, verifiedToken, token)
Expand All @@ -103,9 +89,7 @@ func TestMiddlewareJWTAuth(t *testing.T) {
"foo-email": myEmail,
}, nil
}
bus.AddHandler("get-sign-user", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
return models.ErrUserNotFound
})
sc.mockSQLStore.ExpectedError = models.ErrUserNotFound

sc.fakeReq("GET", "/").withJWTAuthHeader(token).exec()
assert.Equal(t, verifiedToken, token)
Expand All @@ -124,14 +108,7 @@ func TestMiddlewareJWTAuth(t *testing.T) {
"foo-email": myEmail,
}, nil
}
bus.AddHandler("get-sign-user", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
query.Result = &models.SignedInUser{
UserId: id,
OrgId: orgID,
Email: query.Email,
}
return nil
})
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{UserId: id, OrgId: orgID, Email: myEmail}
bus.AddHandler("upsert-user", func(ctx context.Context, command *models.UpsertUserCommand) error {
command.Result = &models.User{
Id: id,
Expand Down
39 changes: 9 additions & 30 deletions pkg/middleware/middleware_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"github.com/grafana/grafana/pkg/services/contexthandler/authproxy"
"github.com/grafana/grafana/pkg/services/login/loginservice"
"github.com/grafana/grafana/pkg/services/rendering"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/setting"
"github.com/grafana/grafana/pkg/util"
Expand Down Expand Up @@ -150,10 +149,7 @@ func TestMiddlewareContext(t *testing.T) {
keyhash, err := util.EncodePassword("v5nAwpMafFP6znaS4urhdWDLS5511M42", "asd")
require.NoError(t, err)

bus.AddHandler("test", func(ctx context.Context, query *models.GetApiKeyByNameQuery) error {
query.Result = &models.ApiKey{OrgId: orgID, Role: models.ROLE_EDITOR, Key: keyhash}
return nil
})
sc.mockSQLStore.ExpectedAPIKey = &models.ApiKey{OrgId: orgID, Role: models.ROLE_EDITOR, Key: keyhash}

sc.fakeReq("GET", "/").withValidApiKey().exec()

Expand All @@ -166,11 +162,7 @@ func TestMiddlewareContext(t *testing.T) {

middlewareScenario(t, "Valid API key, but does not match DB hash", func(t *testing.T, sc *scenarioContext) {
const keyhash = "Something_not_matching"

bus.AddHandler("test", func(ctx context.Context, query *models.GetApiKeyByNameQuery) error {
query.Result = &models.ApiKey{OrgId: 12, Role: models.ROLE_EDITOR, Key: keyhash}
return nil
})
sc.mockSQLStore.ExpectedAPIKey = &models.ApiKey{OrgId: 12, Role: models.ROLE_EDITOR, Key: keyhash}

sc.fakeReq("GET", "/").withValidApiKey().exec()

Expand All @@ -184,13 +176,8 @@ func TestMiddlewareContext(t *testing.T) {
keyhash, err := util.EncodePassword("v5nAwpMafFP6znaS4urhdWDLS5511M42", "asd")
require.NoError(t, err)

bus.AddHandler("test", func(ctx context.Context, query *models.GetApiKeyByNameQuery) error {
// api key expired one second before
expires := sc.contextHandler.GetTime().Add(-1 * time.Second).Unix()
query.Result = &models.ApiKey{OrgId: 12, Role: models.ROLE_EDITOR, Key: keyhash,
Expires: &expires}
return nil
})
expires := sc.contextHandler.GetTime().Add(-1 * time.Second).Unix()
sc.mockSQLStore.ExpectedAPIKey = &models.ApiKey{OrgId: 12, Role: models.ROLE_EDITOR, Key: keyhash, Expires: &expires}

sc.fakeReq("GET", "/").withValidApiKey().exec()

Expand All @@ -203,11 +190,7 @@ func TestMiddlewareContext(t *testing.T) {
const userID int64 = 12

sc.withTokenSessionCookie("token")

bus.AddHandler("test", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
query.Result = &models.SignedInUser{OrgId: 2, UserId: userID}
return nil
})
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{OrgId: 2, UserId: userID}

sc.userAuthTokenService.LookupTokenProvider = func(ctx context.Context, unhashedToken string) (*models.UserToken, error) {
return &models.UserToken{
Expand All @@ -231,11 +214,7 @@ func TestMiddlewareContext(t *testing.T) {
const userID int64 = 12

sc.withTokenSessionCookie("token")

bus.AddHandler("test", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
query.Result = &models.SignedInUser{OrgId: 2, UserId: userID}
return nil
})
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{OrgId: 2, UserId: userID}

sc.userAuthTokenService.LookupTokenProvider = func(ctx context.Context, unhashedToken string) (*models.UserToken, error) {
return &models.UserToken{
Expand Down Expand Up @@ -332,7 +311,8 @@ func TestMiddlewareContext(t *testing.T) {
})

middlewareScenario(t, "When anonymous access is enabled", func(t *testing.T, sc *scenarioContext) {
org, err := sc.sqlStore.CreateOrgWithMember(sc.cfg.AnonymousOrgName, 1)
sc.mockSQLStore.ExpectedOrg = &models.Org{Id: 1, Name: sc.cfg.AnonymousOrgName}
org, err := sc.mockSQLStore.CreateOrgWithMember(sc.cfg.AnonymousOrgName, 1)
require.NoError(t, err)
sc.fakeReq("GET", "/").exec()

Expand Down Expand Up @@ -651,7 +631,6 @@ func middlewareScenario(t *testing.T, desc string, fn scenarioFunc, cbs ...func(
func getContextHandler(t *testing.T, cfg *setting.Cfg, mockSQLStore *mockstore.SQLStoreMock, loginService *loginservice.LoginServiceMock) *contexthandler.ContextHandler {
t.Helper()

sqlStore := sqlstore.InitTestDB(t)
if cfg == nil {
cfg = setting.NewCfg()
}
Expand All @@ -666,7 +645,7 @@ func getContextHandler(t *testing.T, cfg *setting.Cfg, mockSQLStore *mockstore.S
tracer, err := tracing.InitializeTracerForTest()
authProxy := authproxy.ProvideAuthProxy(cfg, remoteCacheSvc, loginService, mockSQLStore)
require.NoError(t, err)
return contexthandler.ProvideService(cfg, userAuthTokenSvc, authJWTSvc, remoteCacheSvc, renderSvc, sqlStore, tracer, authProxy)
return contexthandler.ProvideService(cfg, userAuthTokenSvc, authJWTSvc, remoteCacheSvc, renderSvc, mockSQLStore, tracer, authProxy)
}

type fakeRenderService struct {
Expand Down
12 changes: 2 additions & 10 deletions pkg/middleware/org_redirect_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,11 @@ func TestOrgRedirectMiddleware(t *testing.T) {
for _, tc := range testCases {
middlewareScenario(t, tc.desc, func(t *testing.T, sc *scenarioContext) {
sc.withTokenSessionCookie("token")
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{OrgId: 1, UserId: 12}
bus.AddHandler("test", func(ctx context.Context, query *models.SetUsingOrgCommand) error {
return nil
})

bus.AddHandler("test", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
query.Result = &models.SignedInUser{OrgId: 1, UserId: 12}
return nil
})

sc.userAuthTokenService.LookupTokenProvider = func(ctx context.Context, unhashedToken string) (*models.UserToken, error) {
return &models.UserToken{
UserId: 0,
Expand All @@ -75,11 +71,7 @@ func TestOrgRedirectMiddleware(t *testing.T) {
bus.AddHandler("test", func(ctx context.Context, query *models.SetUsingOrgCommand) error {
return fmt.Errorf("")
})

bus.AddHandler("test", func(ctx context.Context, query *models.GetSignedInUserQuery) error {
query.Result = &models.SignedInUser{OrgId: 1, UserId: 12}
return nil
})
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{OrgId: 1, UserId: 12}

sc.userAuthTokenService.LookupTokenProvider = func(ctx context.Context, unhashedToken string) (*models.UserToken, error) {
return &models.UserToken{
Expand Down
1 change: 1 addition & 0 deletions pkg/middleware/quota_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ func TestMiddlewareQuota(t *testing.T) {
const quotaUsed = 4
setUp := func(sc *scenarioContext) {
sc.withTokenSessionCookie("token")
sc.mockSQLStore.ExpectedSignedInUser = &models.SignedInUser{UserId: 12}
sc.userAuthTokenService.LookupTokenProvider = func(ctx context.Context, unhashedToken string) (*models.UserToken, error) {
return &models.UserToken{
UserId: 12,
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/wire.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ var wireTestSet = wire.NewSet(
wire.Bind(new(notifications.WebhookSender), new(*notifications.NotificationServiceMock)),
wire.Bind(new(notifications.EmailSender), new(*notifications.NotificationServiceMock)),
mockstore.NewSQLStoreMock,
wire.Bind(new(sqlstore.Store), new(*mockstore.SQLStoreMock)),
wire.Bind(new(sqlstore.Store), new(*sqlstore.SQLStore)),
)

func Initialize(cla setting.CommandLineArgs, opts Options, apiOpts api.ServerOptions) (*Server, error) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/contexthandler/auth_jwt.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (h *ContextHandler) initContextWithJWT(ctx *models.ReqContext, orgId int64)
}
}

if err := bus.Dispatch(ctx.Req.Context(), &query); err != nil {
if err := h.SQLStore.GetSignedInUserWithCacheCtx(ctx.Req.Context(), &query); err != nil {
if errors.Is(err, models.ErrUserNotFound) {
ctx.Logger.Debug(
"Failed to find user using JWT claims",
Expand Down
12 changes: 6 additions & 6 deletions pkg/services/contexthandler/contexthandler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const (
const ServiceName = "ContextHandler"

func ProvideService(cfg *setting.Cfg, tokenService models.UserTokenService, jwtService models.JWTService,
remoteCache *remotecache.RemoteCache, renderService rendering.Service, sqlStore *sqlstore.SQLStore,
remoteCache *remotecache.RemoteCache, renderService rendering.Service, sqlStore sqlstore.Store,
tracer tracing.Tracer, authProxy *authproxy.AuthProxy) *ContextHandler {
return &ContextHandler{
Cfg: cfg,
Expand Down Expand Up @@ -151,7 +151,7 @@ func (h *ContextHandler) Middleware(mContext *web.Context) {
// update last seen every 5min
if reqContext.ShouldUpdateLastSeenAt() {
reqContext.Logger.Debug("Updating last user_seen_at", "user_id", reqContext.UserId)
if err := bus.Dispatch(mContext.Req.Context(), &models.UpdateUserLastSeenAtCommand{UserId: reqContext.UserId}); err != nil {
if err := h.SQLStore.UpdateUserLastSeenAt(mContext.Req.Context(), &models.UpdateUserLastSeenAtCommand{UserId: reqContext.UserId}); err != nil {
reqContext.Logger.Error("Failed to update last_seen_at", "error", err)
}
}
Expand Down Expand Up @@ -209,7 +209,7 @@ func (h *ContextHandler) initContextWithAPIKey(reqContext *models.ReqContext) bo

// fetch key
keyQuery := models.GetApiKeyByNameQuery{KeyName: decoded.Name, OrgId: decoded.OrgId}
if err := bus.Dispatch(reqContext.Req.Context(), &keyQuery); err != nil {
if err := h.SQLStore.GetApiKeyByName(reqContext.Req.Context(), &keyQuery); err != nil {
reqContext.JsonApiErr(401, InvalidAPIKey, err)
return true
}
Expand Down Expand Up @@ -251,7 +251,7 @@ func (h *ContextHandler) initContextWithAPIKey(reqContext *models.ReqContext) bo

//Use service account linked to API key as the signed in user
query := models.GetSignedInUserQuery{UserId: *apikey.ServiceAccountId, OrgId: apikey.OrgId}
if err := bus.Dispatch(reqContext.Req.Context(), &query); err != nil {
if err := h.SQLStore.GetSignedInUserWithCacheCtx(reqContext.Req.Context(), &query); err != nil {
reqContext.Logger.Error(
"Failed to link API key to service account in",
"id", query.UserId,
Expand Down Expand Up @@ -308,7 +308,7 @@ func (h *ContextHandler) initContextWithBasicAuth(reqContext *models.ReqContext,
user := authQuery.User

query := models.GetSignedInUserQuery{UserId: user.Id, OrgId: orgID}
if err := bus.Dispatch(ctx, &query); err != nil {
if err := h.SQLStore.GetSignedInUserWithCacheCtx(ctx, &query); err != nil {
reqContext.Logger.Error(
"Failed at user signed in",
"id", user.Id,
Expand Down Expand Up @@ -344,7 +344,7 @@ func (h *ContextHandler) initContextWithToken(reqContext *models.ReqContext, org
}

query := models.GetSignedInUserQuery{UserId: token.UserId, OrgId: orgID}
if err := bus.Dispatch(ctx, &query); err != nil {
if err := h.SQLStore.GetSignedInUserWithCacheCtx(ctx, &query); err != nil {
reqContext.Logger.Error("Failed to get user with id", "userId", token.UserId, "error", err)
return false
}
Expand Down
2 changes: 2 additions & 0 deletions pkg/services/sqlstore/mockstore/mockstore.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type SQLStoreMock struct {
ExpectedNotifierUsageStats []*models.NotifierUsageStats
ExpectedPersistedDashboards models.HitList
ExpectedSignedInUser *models.SignedInUser
ExpectedAPIKey *models.ApiKey
ExpectedUserStars map[int64]bool
ExpectedLoginAttempts int64

Expand Down Expand Up @@ -622,6 +623,7 @@ func (m *SQLStoreMock) GetApiKeyById(ctx context.Context, query *models.GetApiKe
}

func (m *SQLStoreMock) GetApiKeyByName(ctx context.Context, query *models.GetApiKeyByNameQuery) error {
query.Result = m.ExpectedAPIKey
return m.ExpectedError
}

Expand Down

0 comments on commit d153d89

Please sign in to comment.