public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Chore: Remove bus.Dispatch from some login packages (#47248)

Browse Source 
* Chore: Remove bus.Dispatch from some login packages

* remove debug log

Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>

* remove login.Init()

* remove unused reset function

* remove AuthenticateUserFunc global

* swap conditional branches

Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>

* fix formatting

Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>
This commit is contained in:
Serge Zaitsev
2022-04-04 20:36:15 +02:00
committed by GitHub
co-authored by Emil Tullstedt
parent 6992d17924
commit 33006436cc
21 changed files with 190 additions and 143 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/admin_users_test.go
+4 -25
View File
@@ -13,6 +13,7 @@ import (
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/auth"
"github.com/grafana/grafana/pkg/services/login/loginservice"
"github.com/grafana/grafana/pkg/services/login/logintest"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/setting"
@@ -27,28 +28,6 @@ const (
existingTestLogin = "existing@example.com"
)
type mockAuthInfoService struct {
LatestUserID int64
ExpectedError error
}
func (m *mockAuthInfoService) LookupAndUpdate(ctx context.Context, query *models.GetUserByAuthInfoQuery) (*models.User, error) {
m.LatestUserID = query.UserId
return nil, m.ExpectedError
}
func (m *mockAuthInfoService) GetAuthInfo(ctx context.Context, query *models.GetAuthInfoQuery) error {
m.LatestUserID = query.UserId
return m.ExpectedError
}
func (m *mockAuthInfoService) SetAuthInfo(ctx context.Context, query *models.SetAuthInfoCommand) error {
return m.ExpectedError
}
func (m *mockAuthInfoService) UpdateAuthInfo(ctx context.Context, query *models.UpdateAuthInfoCommand) error {
return m.ExpectedError
}
func TestAdminAPIEndpoint(t *testing.T) {
const role = models.ROLE_ADMIN
@@ -282,7 +261,7 @@ func putAdminScenario(t *testing.T, desc string, url string, routePattern string
hs := &HTTPServer{
Cfg: setting.NewCfg(),
SQLStore: sqlStore,
authInfoService: &mockAuthInfoService{},
authInfoService: &logintest.AuthInfoServiceFake{},
}
sc := setupScenarioContext(t, url)
@@ -397,7 +376,7 @@ func adminDisableUserScenario(t *testing.T, desc string, action string, url stri
fakeAuthTokenService := auth.NewFakeUserAuthTokenService()
authInfoService := &mockAuthInfoService{}
authInfoService := &logintest.AuthInfoServiceFake{}
hs := HTTPServer{
Bus: bus.GetBus(),
@@ -435,7 +414,7 @@ func adminDeleteUserScenario(t *testing.T, desc string, url string, routePattern
sc := setupScenarioContext(t, url)
sc.sqlStore = hs.SQLStore
sc.authInfoService = &mockAuthInfoService{}
sc.authInfoService = &logintest.AuthInfoServiceFake{}
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
sc.context = c
sc.context.UserId = testUserID
pkg/api/common_test.go
+2 -1
View File
@@ -34,6 +34,7 @@ import (
"github.com/grafana/grafana/pkg/services/featuremgmt"
"github.com/grafana/grafana/pkg/services/ldap"
"github.com/grafana/grafana/pkg/services/login/loginservice"
"github.com/grafana/grafana/pkg/services/login/logintest"
"github.com/grafana/grafana/pkg/services/quota"
"github.com/grafana/grafana/pkg/services/rendering"
"github.com/grafana/grafana/pkg/services/searchusers"
@@ -168,7 +169,7 @@ type scenarioContext struct {
url string
userAuthTokenService *auth.FakeUserAuthTokenService
sqlStore sqlstore.Store
authInfoService *mockAuthInfoService
authInfoService *logintest.AuthInfoServiceFake
}
func (sc *scenarioContext) exec() {
pkg/api/http_server.go
+4 -1
View File
@@ -21,6 +21,7 @@ import (
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/remotecache"
"github.com/grafana/grafana/pkg/infra/tracing"
loginpkg "github.com/grafana/grafana/pkg/login"
"github.com/grafana/grafana/pkg/login/social"
"github.com/grafana/grafana/pkg/middleware"
"github.com/grafana/grafana/pkg/models"
@@ -134,6 +135,7 @@ type HTTPServer struct {
queryDataService *query.Service
serviceAccountsService serviceaccounts.Service
authInfoService login.AuthInfoService
authenticator loginpkg.Authenticator
teamPermissionsService accesscontrol.PermissionsService
permissionServices accesscontrol.PermissionsServices
NotificationService *notifications.NotificationService
@@ -160,7 +162,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
dataSourceCache datasources.CacheService, userTokenService models.UserTokenService,
cleanUpService *cleanup.CleanUpService, shortURLService shorturls.Service, queryHistoryService queryhistory.Service,
thumbService thumbs.Service, remoteCache *remotecache.RemoteCache, provisioningService provisioning.ProvisioningService,
loginService login.Service, accessControl accesscontrol.AccessControl,
loginService login.Service, authenticator loginpkg.Authenticator, accessControl accesscontrol.AccessControl,
dataSourceProxy *datasourceproxy.DataSourceProxyService, searchService *search.SearchService,
live *live.GrafanaLive, livePushGateway *pushhttp.Gateway, plugCtxProvider *plugincontext.Provider,
contextHandler *contexthandler.ContextHandler, features *featuremgmt.FeatureManager,
@@ -236,6 +238,7 @@ func ProvideHTTPServer(opts ServerOptions, cfg *setting.Cfg, routeRegister routi
queryDataService: queryDataService,
serviceAccountsService: serviceaccountsService,
authInfoService: authInfoService,
authenticator: authenticator,
NotificationService: notificationService,
dashboardService: dashboardService,
dashboardProvisioningService: dashboardProvisioningService,
pkg/api/ldap_debug.go
+1 -2
View File
@@ -9,7 +9,6 @@ import (
"github.com/grafana/grafana/pkg/api/response"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/login"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/ldap"
"github.com/grafana/grafana/pkg/services/multildap"
@@ -199,7 +198,7 @@ func (hs *HTTPServer) PostSyncUserWithLDAP(c *models.ReqContext) response.Respon
}
// Since the user was not in the LDAP server. Let's disable it.
err := login.DisableExternalUser(c.Req.Context(), query.Result.Login)
err := hs.Login.DisableExternalUser(c.Req.Context(), query.Result.Login)
if err != nil {
return response.Error(http.StatusInternalServerError, "Failed to disable the user", err)
}
pkg/api/ldap_debug_test.go
+6 -16
View File
@@ -1,7 +1,6 @@
package api
import (
"context"
"errors"
"net/http"
"net/http/httptest"
@@ -10,12 +9,12 @@ import (
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/login/loginservice"
"github.com/grafana/grafana/pkg/services/login/logintest"
"github.com/grafana/grafana/pkg/services/sqlstore"
"github.com/grafana/grafana/pkg/services/sqlstore/mockstore"
"github.com/grafana/grafana/pkg/api/response"
"github.com/grafana/grafana/pkg/api/routing"
"github.com/grafana/grafana/pkg/bus"
"github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/services/auth"
"github.com/grafana/grafana/pkg/services/ldap"
@@ -368,6 +367,7 @@ func postSyncUserWithLDAPContext(t *testing.T, requestURL string, preHook func(*
t.Helper()
sc := setupScenarioContext(t, requestURL)
sc.authInfoService = &logintest.AuthInfoServiceFake{}
ldap := setting.LDAPEnabled
t.Cleanup(func() {
@@ -380,7 +380,7 @@ func postSyncUserWithLDAPContext(t *testing.T, requestURL string, preHook func(*
AuthTokenService: auth.NewFakeUserAuthTokenService(),
SQLStore: sqlstoremock,
Login: loginservice.LoginServiceMock{},
authInfoService: &mockAuthInfoService{},
authInfoService: sc.authInfoService,
}
sc.defaultHandler = routing.Wrap(func(c *models.ReqContext) response.Response {
@@ -483,6 +483,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenGrafanaAdmin(t *testing.T) {
func TestPostSyncUserWithLDAPAPIEndpoint_WhenUserNotInLDAP(t *testing.T) {
sqlstoremock := mockstore.SQLStoreMock{ExpectedUser: &models.User{Login: "ldap-daniel", Id: 34}}
sc := postSyncUserWithLDAPContext(t, "/api/admin/ldap/sync/34", func(t *testing.T, sc *scenarioContext) {
sc.authInfoService.ExpectedExternalUser = &models.ExternalUserInfo{IsDisabled: true, UserId: 34}
getLDAPConfig = func(*setting.Cfg) (*ldap.Config, error) {
return &ldap.Config{}, nil
}
@@ -492,18 +493,7 @@ func TestPostSyncUserWithLDAPAPIEndpoint_WhenUserNotInLDAP(t *testing.T) {
}
userSearchResult = nil
bus.AddHandler("test", func(ctx context.Context, q *models.GetExternalUserInfoByLoginQuery) error {
assert.Equal(t, "ldap-daniel", q.LoginOrEmail)
q.Result = &models.ExternalUserInfo{IsDisabled: true, UserId: 34}
return nil
})
bus.AddHandler("test", func(ctx context.Context, cmd *models.DisableUserCommand) error {
assert.Equal(t, 34, cmd.UserId)
return nil
})
userSearchError = multildap.ErrDidNotFindUser
}, &sqlstoremock)
assert.Equal(t, http.StatusBadRequest, sc.resp.Code)
@@ -616,7 +606,7 @@ func TestLDAP_AccessControl(t *testing.T) {
cfg.LDAPEnabled = true
sc, hs := setupAccessControlScenarioContext(t, cfg, test.url, test.permissions)
hs.SQLStore = &mockstore.SQLStoreMock{ExpectedUser: &models.User{}}
hs.authInfoService = &mockAuthInfoService{}
hs.authInfoService = &logintest.AuthInfoServiceFake{}
hs.Login = &loginservice.LoginServiceMock{}
sc.resp = httptest.NewRecorder()
sc.req, err = http.NewRequest(test.method, test.url, nil)
pkg/api/login.go
+1 -1
View File
@@ -209,7 +209,7 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext) response.Response {
Cfg: hs.Cfg,
}
err := login.AuthenticateUserFunc(c.Req.Context(), authQuery)
err := hs.authenticator.AuthenticateUser(c.Req.Context(), authQuery)
authModule = authQuery.AuthModule
if err != nil {
resp = response.Error(401, "Invalid username or password", err)
pkg/api/login_test.go
+11 -12
View File
@@ -351,8 +351,7 @@ func TestLoginPostRedirect(t *testing.T) {
Email: "",
}
mockAuthenticateUserFunc(user, "", nil)
t.Cleanup(resetAuthenticateUserFunc)
hs.authenticator = &fakeAuthenticator{user, "", nil}
redirectCases := []redirectCase{
{
@@ -684,8 +683,7 @@ func TestLoginPostRunLokingHook(t *testing.T) {
for _, c := range testCases {
t.Run(c.desc, func(t *testing.T) {
mockAuthenticateUserFunc(c.authUser, c.authModule, c.authErr)
t.Cleanup(resetAuthenticateUserFunc)
hs.authenticator = &fakeAuthenticator{c.authUser, c.authModule, c.authErr}
sc.m.Post(sc.url, sc.defaultHandler)
sc.fakeReqNoAssertions("POST", sc.url).exec()
@@ -732,13 +730,14 @@ func (m *mockSocialService) GetConnector(string) (social.SocialConnector, error)
return m.socialConnector, m.err
}
func mockAuthenticateUserFunc(user *models.User, authmodule string, err error) {
login.AuthenticateUserFunc = func(ctx context.Context, query *models.LoginUserQuery) error {
query.User = user
query.AuthModule = authmodule
return err
}
type fakeAuthenticator struct {
ExpectedUser *models.User
ExpectedAuthModule string
ExpectedError error
}
func resetAuthenticateUserFunc() {
login.AuthenticateUserFunc = login.AuthenticateUser
func (fa *fakeAuthenticator) AuthenticateUser(c context.Context, query *models.LoginUserQuery) error {
query.User = fa.ExpectedUser
query.AuthModule = fa.ExpectedAuthModule
return fa.ExpectedError
}