From eb0bfd6f6d712fae76be4aad1a09b13f1deba231 Mon Sep 17 00:00:00 2001 From: Ben Cooke Date: Wed, 15 Mar 2023 09:14:37 -0400 Subject: [PATCH] [MM-50219] OAuth 2.0 (#22310) * tools updates * Revert "tools updates" This reverts commit 6293297b55803c5a263e200ebd80192899666ae9. * oauth fix * rename migration * migrations-extract * translations * unit test * lint --------- Co-authored-by: Benjamin Cooke Co-authored-by: Benjamin Cooke Co-authored-by: Benjamin Cooke Co-authored-by: Benjamin Cooke --- app/oauth.go | 4 ++ app/oauth_test.go | 45 +++++++++++++++++++ db/migrations/migrations.list | 4 ++ .../mysql/000105_remove_tokens.down.sql | 1 + .../mysql/000105_remove_tokens.up.sql | 4 ++ .../postgres/000105_remove_tokens.down.sql | 1 + .../postgres/000105_remove_tokens.up.sql | 13 ++++++ i18n/en.json | 4 ++ store/opentracinglayer/opentracinglayer.go | 18 ++++++++ store/retrylayer/retrylayer.go | 21 +++++++++ store/sqlstore/oauth_store.go | 8 ++++ store/store.go | 1 + store/storetest/mocks/OAuthStore.go | 14 ++++++ store/timerlayer/timerlayer.go | 16 +++++++ 14 files changed, 154 insertions(+) create mode 100644 db/migrations/mysql/000105_remove_tokens.down.sql create mode 100644 db/migrations/mysql/000105_remove_tokens.up.sql create mode 100644 db/migrations/postgres/000105_remove_tokens.down.sql create mode 100644 db/migrations/postgres/000105_remove_tokens.up.sql diff --git a/app/oauth.go b/app/oauth.go index 7b5b37723d..e09f9e0970 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -501,6 +501,10 @@ func (a *App) DeauthorizeOAuthAppForUser(userID, appID string) *model.AppError { } } + if err := a.Srv().Store().OAuth().RemoveAuthDataByClientId(appID, userID); err != nil { + return model.NewAppError("DeauthorizeOAuthAppForUser", "app.oauth.remove_auth_data_by_client_id.app_error", nil, "", http.StatusInternalServerError).Wrap(err) + } + // Deauthorize the app if err := a.Srv().Store().Preference().Delete(userID, model.PreferenceCategoryAuthorizedOAuthApp, appID); err != nil { return model.NewAppError("DeauthorizeOAuthAppForUser", "app.preference.delete.app_error", nil, "", http.StatusInternalServerError).Wrap(err) diff --git a/app/oauth_test.go b/app/oauth_test.go index 48930d2dbc..73b5fdd6b6 100644 --- a/app/oauth_test.go +++ b/app/oauth_test.go @@ -7,9 +7,11 @@ import ( "encoding/base64" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/stretchr/testify/assert" @@ -19,6 +21,7 @@ import ( "github.com/mattermost/mattermost-server/v6/einterfaces/mocks" "github.com/mattermost/mattermost-server/v6/model" "github.com/mattermost/mattermost-server/v6/plugin/plugintest/mock" + "github.com/mattermost/mattermost-server/v6/store" ) func TestGetOAuthAccessTokenForImplicitFlow(t *testing.T) { @@ -588,3 +591,45 @@ func TestGetAuthorizationCode(t *testing.T) { } }) } + +func TestDeauthorizeOAuthApp(t *testing.T) { + th := Setup(t).InitBasic() + defer th.TearDown() + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOAuthServiceProvider = true }) + + oapp := &model.OAuthApp{ + Name: "fakeoauthapp" + model.NewRandomString(10), + CreatorId: th.BasicUser2.Id, + Homepage: "https://nowhere.com", + Description: "test", + CallbackUrls: []string{"https://nowhere.com"}, + } + + oapp, err := th.App.CreateOAuthApp(oapp) + require.Nil(t, err) + + authRequest := &model.AuthorizeRequest{ + ResponseType: model.ImplicitResponseType, + ClientId: oapp.Id, + RedirectURI: oapp.CallbackUrls[0], + Scope: "", + State: "123", + } + + redirectUrl, err := th.App.GetOAuthCodeRedirect(th.BasicUser.Id, authRequest) + assert.Nil(t, err) + + dErr := th.App.DeauthorizeOAuthAppForUser(th.BasicUser.Id, oapp.Id) + assert.Nil(t, dErr) + + uri, uErr := url.Parse(redirectUrl) + require.NoError(t, uErr) + + queryParams := uri.Query() + code := queryParams.Get("code") + + data, nErr := th.App.Srv().Store().OAuth().GetAuthData(code) + require.Equal(t, store.NewErrNotFound("AuthData", fmt.Sprintf("code=%s", code)), nErr) + assert.Nil(t, data) +} diff --git a/db/migrations/migrations.list b/db/migrations/migrations.list index 9c06d5b88c..6035500adf 100644 --- a/db/migrations/migrations.list +++ b/db/migrations/migrations.list @@ -208,6 +208,8 @@ db/migrations/mysql/000103_add_sentat_to_notifyadmin.down.sql db/migrations/mysql/000103_add_sentat_to_notifyadmin.up.sql db/migrations/mysql/000104_upgrade_notifyadmin.down.sql db/migrations/mysql/000104_upgrade_notifyadmin.up.sql +db/migrations/mysql/000105_remove_tokens.down.sql +db/migrations/mysql/000105_remove_tokens.up.sql db/migrations/postgres/000001_create_teams.down.sql db/migrations/postgres/000001_create_teams.up.sql db/migrations/postgres/000002_create_team_members.down.sql @@ -416,3 +418,5 @@ db/migrations/postgres/000103_add_sentat_to_notifyadmin.down.sql db/migrations/postgres/000103_add_sentat_to_notifyadmin.up.sql db/migrations/postgres/000104_upgrade_notifyadmin.down.sql db/migrations/postgres/000104_upgrade_notifyadmin.up.sql +db/migrations/postgres/000105_remove_tokens.down.sql +db/migrations/postgres/000105_remove_tokens.up.sql diff --git a/db/migrations/mysql/000105_remove_tokens.down.sql b/db/migrations/mysql/000105_remove_tokens.down.sql new file mode 100644 index 0000000000..4743bd6462 --- /dev/null +++ b/db/migrations/mysql/000105_remove_tokens.down.sql @@ -0,0 +1 @@ +-- Skipping it because the forward migrations are destructive diff --git a/db/migrations/mysql/000105_remove_tokens.up.sql b/db/migrations/mysql/000105_remove_tokens.up.sql new file mode 100644 index 0000000000..49d41d7d7f --- /dev/null +++ b/db/migrations/mysql/000105_remove_tokens.up.sql @@ -0,0 +1,4 @@ +DELETE o, s from OAuthAccessData o +LEFT JOIN Preferences p ON o.clientid = p.name AND o.userid = p.userid AND p.category = 'oauth_app' +INNER JOIN Sessions s ON o.token = s.token +WHERE p.name IS NULL; diff --git a/db/migrations/postgres/000105_remove_tokens.down.sql b/db/migrations/postgres/000105_remove_tokens.down.sql new file mode 100644 index 0000000000..4743bd6462 --- /dev/null +++ b/db/migrations/postgres/000105_remove_tokens.down.sql @@ -0,0 +1 @@ +-- Skipping it because the forward migrations are destructive diff --git a/db/migrations/postgres/000105_remove_tokens.up.sql b/db/migrations/postgres/000105_remove_tokens.up.sql new file mode 100644 index 0000000000..ac9dc4dc7d --- /dev/null +++ b/db/migrations/postgres/000105_remove_tokens.up.sql @@ -0,0 +1,13 @@ +DO $$ +BEGIN +WITH oauthDelete AS ( + DELETE FROM oauthaccessdata o + WHERE NOT EXISTS ( + SELECT p.* FROM preferences p + WHERE o.clientid = p.name AND o.userid = p.userid AND p.category = 'oauth_app' + and p.name IS NULL + ) + RETURNING o.token +) +DELETE FROM sessions s WHERE s.token in (select oauthDelete.token from oauthDelete); +END $$; diff --git a/i18n/en.json b/i18n/en.json index 9abad6d78c..610f91cf7e 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -5951,6 +5951,10 @@ "id": "app.oauth.remove_access_data.app_error", "translation": "Unable to remove the access token." }, + { + "id": "app.oauth.remove_auth_data_by_client_id.app_error", + "translation": "Unable to remove oauth data." + }, { "id": "app.oauth.save_app.existing.app_error", "translation": "Must call update for existing app." diff --git a/store/opentracinglayer/opentracinglayer.go b/store/opentracinglayer/opentracinglayer.go index 0486a0e350..4d1cc1b961 100644 --- a/store/opentracinglayer/opentracinglayer.go +++ b/store/opentracinglayer/opentracinglayer.go @@ -5555,6 +5555,24 @@ func (s *OpenTracingLayerOAuthStore) RemoveAuthData(code string) error { return err } +func (s *OpenTracingLayerOAuthStore) RemoveAuthDataByClientId(clientId string, userId string) error { + origCtx := s.Root.Store.Context() + span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "OAuthStore.RemoveAuthDataByClientId") + s.Root.Store.SetContext(newCtx) + defer func() { + s.Root.Store.SetContext(origCtx) + }() + + defer span.Finish() + err := s.OAuthStore.RemoveAuthDataByClientId(clientId, userId) + if err != nil { + span.LogFields(spanlog.Error(err)) + ext.Error.Set(span, true) + } + + return err +} + func (s *OpenTracingLayerOAuthStore) SaveAccessData(accessData *model.AccessData) (*model.AccessData, error) { origCtx := s.Root.Store.Context() span, newCtx := tracing.StartSpanWithParentByContext(s.Root.Store.Context(), "OAuthStore.SaveAccessData") diff --git a/store/retrylayer/retrylayer.go b/store/retrylayer/retrylayer.go index 333caea8db..4cea13d0ff 100644 --- a/store/retrylayer/retrylayer.go +++ b/store/retrylayer/retrylayer.go @@ -6298,6 +6298,27 @@ func (s *RetryLayerOAuthStore) RemoveAuthData(code string) error { } +func (s *RetryLayerOAuthStore) RemoveAuthDataByClientId(clientId string, userId string) error { + + tries := 0 + for { + err := s.OAuthStore.RemoveAuthDataByClientId(clientId, userId) + if err == nil { + return nil + } + if !isRepeatableError(err) { + return err + } + tries++ + if tries >= 3 { + err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures") + return err + } + timepkg.Sleep(100 * timepkg.Millisecond) + } + +} + func (s *RetryLayerOAuthStore) SaveAccessData(accessData *model.AccessData) (*model.AccessData, error) { tries := 0 diff --git a/store/sqlstore/oauth_store.go b/store/sqlstore/oauth_store.go index b7ffbd316e..201e2bba27 100644 --- a/store/sqlstore/oauth_store.go +++ b/store/sqlstore/oauth_store.go @@ -261,6 +261,14 @@ func (as SqlOAuthStore) RemoveAuthData(code string) error { return nil } +func (as SqlOAuthStore) RemoveAuthDataByClientId(clientId string, userId string) error { + _, err := as.GetMasterX().Exec("DELETE FROM OAuthAuthData WHERE ClientId = ? and UserId = ?", clientId, userId) + if err != nil { + return errors.Wrapf(err, "failed to delete AuthData with clientId=%s and userId=%s", clientId, userId) + } + return nil +} + func (as SqlOAuthStore) PermanentDeleteAuthDataByUser(userId string) error { _, err := as.GetMasterX().Exec("DELETE FROM OAuthAccessData WHERE UserId = ?", userId) if err != nil { diff --git a/store/store.go b/store/store.go index bfebe49e19..8c6ef80daf 100644 --- a/store/store.go +++ b/store/store.go @@ -565,6 +565,7 @@ type OAuthStore interface { SaveAuthData(authData *model.AuthData) (*model.AuthData, error) GetAuthData(code string) (*model.AuthData, error) RemoveAuthData(code string) error + RemoveAuthDataByClientId(clientId string, userId string) error PermanentDeleteAuthDataByUser(userID string) error SaveAccessData(accessData *model.AccessData) (*model.AccessData, error) UpdateAccessData(accessData *model.AccessData) (*model.AccessData, error) diff --git a/store/storetest/mocks/OAuthStore.go b/store/storetest/mocks/OAuthStore.go index b67590e7df..77d8beeb6e 100644 --- a/store/storetest/mocks/OAuthStore.go +++ b/store/storetest/mocks/OAuthStore.go @@ -291,6 +291,20 @@ func (_m *OAuthStore) RemoveAuthData(code string) error { return r0 } +// RemoveAuthDataByClientId provides a mock function with given fields: clientId, userId +func (_m *OAuthStore) RemoveAuthDataByClientId(clientId string, userId string) error { + ret := _m.Called(clientId, userId) + + var r0 error + if rf, ok := ret.Get(0).(func(string, string) error); ok { + r0 = rf(clientId, userId) + } else { + r0 = ret.Error(0) + } + + return r0 +} + // SaveAccessData provides a mock function with given fields: accessData func (_m *OAuthStore) SaveAccessData(accessData *model.AccessData) (*model.AccessData, error) { ret := _m.Called(accessData) diff --git a/store/timerlayer/timerlayer.go b/store/timerlayer/timerlayer.go index df44340752..8dd13cb9f6 100644 --- a/store/timerlayer/timerlayer.go +++ b/store/timerlayer/timerlayer.go @@ -5036,6 +5036,22 @@ func (s *TimerLayerOAuthStore) RemoveAuthData(code string) error { return err } +func (s *TimerLayerOAuthStore) RemoveAuthDataByClientId(clientId string, userId string) error { + start := time.Now() + + err := s.OAuthStore.RemoveAuthDataByClientId(clientId, userId) + + elapsed := float64(time.Since(start)) / float64(time.Second) + if s.Root.Metrics != nil { + success := "false" + if err == nil { + success = "true" + } + s.Root.Metrics.ObserveStoreMethodDuration("OAuthStore.RemoveAuthDataByClientId", success, elapsed) + } + return err +} + func (s *TimerLayerOAuthStore) SaveAccessData(accessData *model.AccessData) (*model.AccessData, error) { start := time.Now()