From a7fd13384b3f147d9b482fc43f886f747704134c Mon Sep 17 00:00:00 2001 From: Christopher Speller Date: Fri, 13 Apr 2018 17:09:06 -0700 Subject: Removing user cache clear from SessionHasPermissionsTo (#8625) --- app/authorization.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/app/authorization.go b/app/authorization.go index 632dd7566..2187472f7 100644 --- a/app/authorization.go +++ b/app/authorization.go @@ -12,12 +12,7 @@ import ( ) func (a *App) SessionHasPermissionTo(session model.Session, permission *model.Permission) bool { - if !a.RolesGrantPermission(session.GetUserRoles(), permission.Id) { - a.ClearSessionCacheForUser(session.UserId) - return false - } - - return true + return a.RolesGrantPermission(session.GetUserRoles(), permission.Id) } /// DO NOT USE: LEGACY -- cgit v1.2.3-1-g7c22 From 8056dc33e31d87ff81d286b9991883f953705f15 Mon Sep 17 00:00:00 2001 From: Jesse Hallam Date: Fri, 13 Apr 2018 20:09:38 -0400 Subject: Prevent disabling or modifying l4g logging filters (#8628) The underlying l4g library is not resilient to filter modifications in the presence of concurrent goroutines. In particular, it's not safe to call Close() on filters which might be actively held by a goroutine for logging. This change disables all modifications to existing filters once initialized by the App layer. In practice, we might be able to get away with some modifications to the existing filters (i.e. changing levels), but the [golang memory model](https://golang.org/ref/mem) makes no guarantees that it is safe to do so: > Programs that modify data being simultaneously accessed by multiple goroutines must serialize such access. We can solve this holistically by introducing the requisite locking within our fork of the l4g library. For now, we just disable all modifications. --- app/app.go | 7 +++++++ utils/config.go | 13 +++++++------ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/app.go b/app/app.go index 27227d271..462c45613 100644 --- a/app/app.go +++ b/app/app.go @@ -125,7 +125,14 @@ func New(options ...Option) (outApp *App, outErr error) { } } model.AppErrorInit(utils.T) + + // The first time we load config, clear any existing filters to allow the configuration + // changes to take effect. This is safe only because no one else is logging at this point. + l4g.Close() + if err := app.LoadConfig(app.configFile); err != nil { + // Re-initialize the default logger as we bail out. + l4g.Global = l4g.NewDefaultLogger(l4g.DEBUG) return nil, err } app.EnableConfigWatch() diff --git a/utils/config.go b/utils/config.go index 13295b362..6026f43f9 100644 --- a/utils/config.go +++ b/utils/config.go @@ -83,13 +83,15 @@ func ConfigureCmdLineLog() { ConfigureLog(&ls) } +// ConfigureLog enables and configures logging. +// +// Note that it is not currently possible to disable filters nor to modify previously enabled +// filters, given the lack of concurrency guarantees from the underlying l4g library. +// // TODO: this code initializes console and file logging. It will eventually be replaced by JSON logging in logger/logger.go // See PLT-3893 for more information func ConfigureLog(s *model.LogSettings) { - - l4g.Close() - - if s.EnableConsole { + if _, alreadySet := l4g.Global["stdout"]; !alreadySet && s.EnableConsole { level := l4g.DEBUG if s.ConsoleLevel == "INFO" { level = l4g.INFO @@ -104,8 +106,7 @@ func ConfigureLog(s *model.LogSettings) { l4g.AddFilter("stdout", level, lw) } - if s.EnableFile { - + if _, alreadySet := l4g.Global["file"]; !alreadySet && s.EnableFile { var fileFormat = s.FileFormat if fileFormat == "" { -- cgit v1.2.3-1-g7c22 From 3176e13b1f9b14192bef2337ff3fb9346f26ef66 Mon Sep 17 00:00:00 2001 From: Derrick Anderson Date: Fri, 13 Apr 2018 20:42:27 -0400 Subject: revert changes from MM9720 --- cmd/commands/user.go | 40 ---------------------------------------- cmd/commands/user_test.go | 28 ---------------------------- 2 files changed, 68 deletions(-) diff --git a/cmd/commands/user.go b/cmd/commands/user.go index 9f5e5ae0d..e3bc8208a 100644 --- a/cmd/commands/user.go +++ b/cmd/commands/user.go @@ -66,15 +66,6 @@ var ResetUserPasswordCmd = &cobra.Command{ RunE: resetUserPasswordCmdF, } -var updateUserEmailCmd = &cobra.Command{ - Use: "email [user] [new email]", - Short: "Change email of the user", - Long: "Change email of the user.", - Example: ` user email test user@example.com - user activate username`, - RunE: updateUserEmailCmdF, -} - var ResetUserMfaCmd = &cobra.Command{ Use: "resetmfa [users]", Short: "Turn off MFA", @@ -238,7 +229,6 @@ Global Flags: UserCreateCmd, UserInviteCmd, ResetUserPasswordCmd, - updateUserEmailCmd, ResetUserMfaCmd, DeleteUserCmd, DeleteAllUsersCmd, @@ -409,36 +399,6 @@ func resetUserPasswordCmdF(command *cobra.Command, args []string) error { return nil } -func updateUserEmailCmdF(command *cobra.Command, args []string) error { - a, err := cmd.InitDBCommandContextCobra(command) - if err != nil { - return err - } - - newEmail := args[1] - - if !model.IsValidEmail(newEmail) { - return errors.New("Invalid email: '" + newEmail + "'") - } - - if len(args) != 2 { - return errors.New("Expected two arguments. See help text for details.") - } - - user := getUserFromUserArg(a, args[0]) - if user == nil { - return errors.New("Unable to find user '" + args[0] + "'") - } - - user.Email = newEmail - _, errUpdate := a.UpdateUser(user, true) - if err != nil { - return errUpdate - } - - return nil -} - func resetUserMfaCmdF(command *cobra.Command, args []string) error { a, err := cmd.InitDBCommandContextCobra(command) if err != nil { diff --git a/cmd/commands/user_test.go b/cmd/commands/user_test.go index a1081c5d3..960ac3878 100644 --- a/cmd/commands/user_test.go +++ b/cmd/commands/user_test.go @@ -9,7 +9,6 @@ import ( "github.com/mattermost/mattermost-server/api" "github.com/mattermost/mattermost-server/cmd" "github.com/mattermost/mattermost-server/model" - "github.com/stretchr/testify/require" ) func TestCreateUserWithTeam(t *testing.T) { @@ -81,30 +80,3 @@ func TestMakeUserActiveAndInactive(t *testing.T) { // activate the inactive user cmd.CheckCommand(t, "user", "activate", th.BasicUser.Email) } - -func TestChangeUserEmail(t *testing.T) { - th := api.Setup().InitBasic() - defer th.TearDown() - - newEmail := model.NewId() + "@mattermost-test.com" - - cmd.CheckCommand(t, "user", "email", th.BasicUser.Username, newEmail) - if result := <-th.App.Srv.Store.User().GetByEmail(th.BasicUser.Email); result.Err == nil { - t.Fatal("should've updated to the new email") - } - if result := <-th.App.Srv.Store.User().GetByEmail(newEmail); result.Err != nil { - t.Fatal() - } else { - user := result.Data.(*model.User) - if user.Email != newEmail { - t.Fatal("should've updated to the new email") - } - } - - // should fail because using an invalid email - require.Error(t, cmd.RunCommand(t, "user", "email", th.BasicUser.Username, "wrong$email.com")) - - // should fail because user not found - require.Error(t, cmd.RunCommand(t, "user", "email", "invalidUser", newEmail)) - -} -- cgit v1.2.3-1-g7c22