diff --git a/app/oauth.go b/app/oauth.go index 818f78a2d4..a05fe1e947 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -342,11 +342,10 @@ func (a *App) newSession(appName string, user *model.User) (*model.Session, *mod session.AddProp(model.SESSION_PROP_OS, "OAuth2") session.AddProp(model.SESSION_PROP_BROWSER, "OAuth2") - result := <-a.Srv.Store.Session().Save(session) - if result.Err != nil { + session, err := a.Srv.Store.Session().Save(session) + if err != nil { return nil, model.NewAppError("newSession", "api.oauth.get_access_token.internal_session.app_error", nil, "", http.StatusInternalServerError) } - session = result.Data.(*model.Session) a.AddSessionToCache(session) diff --git a/app/session.go b/app/session.go index 8b552b111b..b70a7bae8e 100644 --- a/app/session.go +++ b/app/session.go @@ -14,11 +14,10 @@ import ( func (a *App) CreateSession(session *model.Session) (*model.Session, *model.AppError) { session.Token = "" - result := <-a.Srv.Store.Session().Save(session) - if result.Err != nil { - return nil, result.Err + session, err := a.Srv.Store.Session().Save(session) + if err != nil { + return nil, err } - session = result.Data.(*model.Session) a.AddSessionToCache(session) @@ -303,11 +302,10 @@ func (a *App) createSessionForUserAccessToken(tokenString string) (*model.Sessio session.AddProp(model.SESSION_PROP_TYPE, model.SESSION_TYPE_USER_ACCESS_TOKEN) session.SetExpireInDays(model.SESSION_USER_ACCESS_TOKEN_EXPIRY) - result = <-a.Srv.Store.Session().Save(session) - if result.Err != nil { - return nil, result.Err + session, err = a.Srv.Store.Session().Save(session) + if err != nil { + return nil, err } - session = result.Data.(*model.Session) a.AddSessionToCache(session) diff --git a/store/sqlstore/session_store.go b/store/sqlstore/session_store.go index 9ef850b3af..23d771e54a 100644 --- a/store/sqlstore/session_store.go +++ b/store/sqlstore/session_store.go @@ -45,37 +45,34 @@ func (me SqlSessionStore) CreateIndexesIfNotExists() { me.CreateIndexIfNotExists("idx_sessions_last_activity_at", "Sessions", "LastActivityAt") } -func (me SqlSessionStore) Save(session *model.Session) store.StoreChannel { - return store.Do(func(result *store.StoreResult) { - if len(session.Id) > 0 { - result.Err = model.NewAppError("SqlSessionStore.Save", "store.sql_session.save.existing.app_error", nil, "id="+session.Id, http.StatusBadRequest) - return +func (me SqlSessionStore) Save(session *model.Session) (*model.Session, *model.AppError) { + if len(session.Id) > 0 { + return nil, model.NewAppError("SqlSessionStore.Save", "store.sql_session.save.existing.app_error", nil, "id="+session.Id, http.StatusBadRequest) + } + + session.PreSave() + + tcs := me.Team().GetTeamsForUser(session.UserId) + + if err := me.GetMaster().Insert(session); err != nil { + return nil, model.NewAppError("SqlSessionStore.Save", "store.sql_session.save.app_error", nil, "id="+session.Id+", "+err.Error(), http.StatusInternalServerError) + } + + rtcs := <-tcs + + if rtcs.Err != nil { + return nil, model.NewAppError("SqlSessionStore.Save", "store.sql_session.save.app_error", nil, "id="+session.Id+", "+rtcs.Err.Error(), http.StatusInternalServerError) + } + + tempMembers := rtcs.Data.([]*model.TeamMember) + session.TeamMembers = make([]*model.TeamMember, 0, len(tempMembers)) + for _, tm := range tempMembers { + if tm.DeleteAt == 0 { + session.TeamMembers = append(session.TeamMembers, tm) } + } - session.PreSave() - - tcs := me.Team().GetTeamsForUser(session.UserId) - - if err := me.GetMaster().Insert(session); err != nil { - result.Err = model.NewAppError("SqlSessionStore.Save", "store.sql_session.save.app_error", nil, "id="+session.Id+", "+err.Error(), http.StatusInternalServerError) - return - } else { - result.Data = session - } - - if rtcs := <-tcs; rtcs.Err != nil { - result.Err = model.NewAppError("SqlSessionStore.Save", "store.sql_session.save.app_error", nil, "id="+session.Id+", "+rtcs.Err.Error(), http.StatusInternalServerError) - return - } else { - tempMembers := rtcs.Data.([]*model.TeamMember) - session.TeamMembers = make([]*model.TeamMember, 0, len(tempMembers)) - for _, tm := range tempMembers { - if tm.DeleteAt == 0 { - session.TeamMembers = append(session.TeamMembers, tm) - } - } - } - }) + return session, nil } func (me SqlSessionStore) Get(sessionIdOrToken string) store.StoreChannel { diff --git a/store/store.go b/store/store.go index 38bb012dbc..c5c771f109 100644 --- a/store/store.go +++ b/store/store.go @@ -312,7 +312,7 @@ type BotStore interface { } type SessionStore interface { - Save(session *model.Session) StoreChannel + Save(session *model.Session) (*model.Session, *model.AppError) Get(sessionIdOrToken string) StoreChannel GetSessions(userId string) StoreChannel GetSessionsWithActiveDeviceIds(userId string) ([]*model.Session, *model.AppError) diff --git a/store/storetest/mocks/SessionStore.go b/store/storetest/mocks/SessionStore.go index 6bff8311ef..4e94fa28b8 100644 --- a/store/storetest/mocks/SessionStore.go +++ b/store/storetest/mocks/SessionStore.go @@ -147,19 +147,28 @@ func (_m *SessionStore) RemoveAllSessions() store.StoreChannel { } // Save provides a mock function with given fields: session -func (_m *SessionStore) Save(session *model.Session) store.StoreChannel { +func (_m *SessionStore) Save(session *model.Session) (*model.Session, *model.AppError) { ret := _m.Called(session) - var r0 store.StoreChannel - if rf, ok := ret.Get(0).(func(*model.Session) store.StoreChannel); ok { + var r0 *model.Session + if rf, ok := ret.Get(0).(func(*model.Session) *model.Session); ok { r0 = rf(session) } else { if ret.Get(0) != nil { - r0 = ret.Get(0).(store.StoreChannel) + r0 = ret.Get(0).(*model.Session) } } - return r0 + var r1 *model.AppError + if rf, ok := ret.Get(1).(func(*model.Session) *model.AppError); ok { + r1 = rf(session) + } else { + if ret.Get(1) != nil { + r1 = ret.Get(1).(*model.AppError) + } + } + + return r0, r1 } // UpdateDeviceId provides a mock function with given fields: id, deviceId, expiresAt diff --git a/store/storetest/oauth_store.go b/store/storetest/oauth_store.go index bec0553591..eb77f381e4 100644 --- a/store/storetest/oauth_store.go +++ b/store/storetest/oauth_store.go @@ -8,7 +8,6 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" - "github.com/stretchr/testify/require" ) @@ -409,12 +408,13 @@ func testOAuthStoreDeleteApp(t *testing.T, ss store.Store) { t.Fatal(err) } - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() s1.Token = model.NewId() s1.IsOAuth = true - store.Must(ss.Session().Save(&s1)) + s1, err := ss.Session().Save(s1) + require.Nil(t, err) ad1 := model.AccessData{} ad1.ClientId = a1.Id diff --git a/store/storetest/session_store.go b/store/storetest/session_store.go index bed9795af1..2bcee6ebf5 100644 --- a/store/storetest/session_store.go +++ b/store/storetest/session_store.go @@ -10,6 +10,7 @@ import ( "github.com/mattermost/mattermost-server/store" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSessionStore(t *testing.T, ss store.Store) { @@ -30,27 +31,33 @@ func TestSessionStore(t *testing.T, ss store.Store) { } func testSessionStoreSave(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - if err := (<-ss.Session().Save(&s1)).Err; err != nil { + if _, err := ss.Session().Save(s1); err != nil { t.Fatal(err) } } func testSessionGet(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - store.Must(ss.Session().Save(&s1)) - s2 := model.Session{} + s1, err := ss.Session().Save(s1) + require.Nil(t, err) + + s2 := &model.Session{} s2.UserId = s1.UserId - store.Must(ss.Session().Save(&s2)) - s3 := model.Session{} + s2, err = ss.Session().Save(s2) + require.Nil(t, err) + + s3 := &model.Session{} s3.UserId = s1.UserId s3.ExpiresAt = 1 - store.Must(ss.Session().Save(&s3)) + + s3, err = ss.Session().Save(s3) + require.Nil(t, err) if rs1 := (<-ss.Session().Get(s1.Id)); rs1.Err != nil { t.Fatal(rs1.Err) @@ -70,22 +77,28 @@ func testSessionGet(t *testing.T, ss store.Store) { } func testSessionGetWithDeviceId(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() s1.ExpiresAt = model.GetMillis() + 10000 - store.Must(ss.Session().Save(&s1)) - s2 := model.Session{} + s1, err := ss.Session().Save(s1) + require.Nil(t, err) + + s2 := &model.Session{} s2.UserId = s1.UserId s2.DeviceId = model.NewId() s2.ExpiresAt = model.GetMillis() + 10000 - store.Must(ss.Session().Save(&s2)) - s3 := model.Session{} + s2, err = ss.Session().Save(s2) + require.Nil(t, err) + + s3 := &model.Session{} s3.UserId = s1.UserId s3.ExpiresAt = 1 s3.DeviceId = model.NewId() - store.Must(ss.Session().Save(&s3)) + + s3, err = ss.Session().Save(s3) + require.Nil(t, err) if data, err := ss.Session().GetSessionsWithActiveDeviceIds(s1.UserId); err != nil { t.Fatal(err) @@ -97,9 +110,11 @@ func testSessionGetWithDeviceId(t *testing.T, ss store.Store) { } func testSessionRemove(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - store.Must(ss.Session().Save(&s1)) + + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if rs1 := (<-ss.Session().Get(s1.Id)); rs1.Err != nil { t.Fatal(rs1.Err) @@ -117,9 +132,11 @@ func testSessionRemove(t *testing.T, ss store.Store) { } func testSessionRemoveAll(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - store.Must(ss.Session().Save(&s1)) + + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if rs1 := (<-ss.Session().Get(s1.Id)); rs1.Err != nil { t.Fatal(rs1.Err) @@ -137,9 +154,11 @@ func testSessionRemoveAll(t *testing.T, ss store.Store) { } func testSessionRemoveByUser(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - store.Must(ss.Session().Save(&s1)) + + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if rs1 := (<-ss.Session().Get(s1.Id)); rs1.Err != nil { t.Fatal(rs1.Err) @@ -157,9 +176,11 @@ func testSessionRemoveByUser(t *testing.T, ss store.Store) { } func testSessionRemoveToken(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - store.Must(ss.Session().Save(&s1)) + + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if rs1 := (<-ss.Session().Get(s1.Id)); rs1.Err != nil { t.Fatal(rs1.Err) @@ -185,17 +206,21 @@ func testSessionRemoveToken(t *testing.T, ss store.Store) { } func testSessionUpdateDeviceId(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - store.Must(ss.Session().Save(&s1)) + + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if rs1 := (<-ss.Session().UpdateDeviceId(s1.Id, model.PUSH_NOTIFY_APPLE+":1234567890", s1.ExpiresAt)); rs1.Err != nil { t.Fatal(rs1.Err) } - s2 := model.Session{} + s2 := &model.Session{} s2.UserId = model.NewId() - store.Must(ss.Session().Save(&s2)) + + s2, err = ss.Session().Save(s2) + require.Nil(t, err) if rs2 := (<-ss.Session().UpdateDeviceId(s2.Id, model.PUSH_NOTIFY_APPLE+":1234567890", s1.ExpiresAt)); rs2.Err != nil { t.Fatal(rs2.Err) @@ -203,17 +228,21 @@ func testSessionUpdateDeviceId(t *testing.T, ss store.Store) { } func testSessionUpdateDeviceId2(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - store.Must(ss.Session().Save(&s1)) + + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if rs1 := (<-ss.Session().UpdateDeviceId(s1.Id, model.PUSH_NOTIFY_APPLE_REACT_NATIVE+":1234567890", s1.ExpiresAt)); rs1.Err != nil { t.Fatal(rs1.Err) } - s2 := model.Session{} + s2 := &model.Session{} s2.UserId = model.NewId() - store.Must(ss.Session().Save(&s2)) + + s2, err = ss.Session().Save(s2) + require.Nil(t, err) if rs2 := (<-ss.Session().UpdateDeviceId(s2.Id, model.PUSH_NOTIFY_APPLE_REACT_NATIVE+":1234567890", s1.ExpiresAt)); rs2.Err != nil { t.Fatal(rs2.Err) @@ -221,9 +250,11 @@ func testSessionUpdateDeviceId2(t *testing.T, ss store.Store) { } func testSessionStoreUpdateLastActivityAt(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() - store.Must(ss.Session().Save(&s1)) + + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if err := (<-ss.Session().UpdateLastActivityAt(s1.Id, 1234567890)).Err; err != nil { t.Fatal(err) @@ -240,10 +271,12 @@ func testSessionStoreUpdateLastActivityAt(t *testing.T, ss store.Store) { } func testSessionCount(t *testing.T, ss store.Store) { - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() s1.ExpiresAt = model.GetMillis() + 100000 - store.Must(ss.Session().Save(&s1)) + + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if count, err := ss.Session().AnalyticsSessionCount(); err != nil { t.Fatal(err) @@ -257,29 +290,37 @@ func testSessionCount(t *testing.T, ss store.Store) { func testSessionCleanup(t *testing.T, ss store.Store) { now := model.GetMillis() - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = model.NewId() s1.ExpiresAt = 0 // never expires - store.Must(ss.Session().Save(&s1)) - s2 := model.Session{} + s1, err := ss.Session().Save(s1) + require.Nil(t, err) + + s2 := &model.Session{} s2.UserId = s1.UserId s2.ExpiresAt = now + 1000000 // expires in the future - store.Must(ss.Session().Save(&s2)) - s3 := model.Session{} + s2, err = ss.Session().Save(s2) + require.Nil(t, err) + + s3 := &model.Session{} s3.UserId = model.NewId() s3.ExpiresAt = 1 // expired - store.Must(ss.Session().Save(&s3)) - s4 := model.Session{} + s3, err = ss.Session().Save(s3) + require.Nil(t, err) + + s4 := &model.Session{} s4.UserId = model.NewId() s4.ExpiresAt = 2 // expired - store.Must(ss.Session().Save(&s4)) + + s4, err = ss.Session().Save(s4) + require.Nil(t, err) ss.Session().Cleanup(now, 1) - err := (<-ss.Session().Get(s1.Id)).Err + err = (<-ss.Session().Get(s1.Id)).Err assert.Nil(t, err) err = (<-ss.Session().Get(s2.Id)).Err diff --git a/store/storetest/user_access_token_store.go b/store/storetest/user_access_token_store.go index 3c5521617a..cc39ccd8c3 100644 --- a/store/storetest/user_access_token_store.go +++ b/store/storetest/user_access_token_store.go @@ -8,6 +8,7 @@ import ( "github.com/mattermost/mattermost-server/model" "github.com/mattermost/mattermost-server/store" + "github.com/stretchr/testify/require" ) func TestUserAccessTokenStore(t *testing.T, ss store.Store) { @@ -23,11 +24,12 @@ func testUserAccessTokenSaveGetDelete(t *testing.T, ss store.Store) { Description: "testtoken", } - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = uat.UserId s1.Token = uat.Token - store.Must(ss.Session().Save(&s1)) + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if result := <-ss.UserAccessToken().Save(uat); result.Err != nil { t.Fatal(result.Err) @@ -65,19 +67,20 @@ func testUserAccessTokenSaveGetDelete(t *testing.T, ss store.Store) { t.Fatal(result.Err) } - if err := (<-ss.Session().Get(s1.Token)).Err; err == nil { + if err = (<-ss.Session().Get(s1.Token)).Err; err == nil { t.Fatal("should error - session should be deleted") } - if err := (<-ss.UserAccessToken().GetByToken(s1.Token)).Err; err == nil { + if err = (<-ss.UserAccessToken().GetByToken(s1.Token)).Err; err == nil { t.Fatal("should error - access token should be deleted") } - s2 := model.Session{} + s2 := &model.Session{} s2.UserId = uat.UserId s2.Token = uat.Token - store.Must(ss.Session().Save(&s2)) + s2, err = ss.Session().Save(s2) + require.Nil(t, err) if result := <-ss.UserAccessToken().Save(uat); result.Err != nil { t.Fatal(result.Err) @@ -103,29 +106,31 @@ func testUserAccessTokenDisableEnable(t *testing.T, ss store.Store) { Description: "testtoken", } - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = uat.UserId s1.Token = uat.Token - store.Must(ss.Session().Save(&s1)) + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if result := <-ss.UserAccessToken().Save(uat); result.Err != nil { t.Fatal(result.Err) } - if err := (<-ss.UserAccessToken().UpdateTokenDisable(uat.Id)).Err; err != nil { + if err = (<-ss.UserAccessToken().UpdateTokenDisable(uat.Id)).Err; err != nil { t.Fatal(err) } - if err := (<-ss.Session().Get(s1.Token)).Err; err == nil { + if err = (<-ss.Session().Get(s1.Token)).Err; err == nil { t.Fatal("should error - session should be deleted") } - s2 := model.Session{} + s2 := &model.Session{} s2.UserId = uat.UserId s2.Token = uat.Token - store.Must(ss.Session().Save(&s2)) + s2, err = ss.Session().Save(s2) + require.Nil(t, err) if err := (<-ss.UserAccessToken().UpdateTokenEnable(uat.Id)).Err; err != nil { t.Fatal(err) @@ -145,11 +150,12 @@ func testUserAccessTokenSearch(t *testing.T, ss store.Store) { Description: "testtoken", } - s1 := model.Session{} + s1 := &model.Session{} s1.UserId = uat.UserId s1.Token = uat.Token - store.Must(ss.Session().Save(&s1)) + s1, err := ss.Session().Save(s1) + require.Nil(t, err) if result := <-ss.UserAccessToken().Save(uat); result.Err != nil { t.Fatal(result.Err)