From 7710a2fe3741d42b6eba7f4904a9e653e789f7c3 Mon Sep 17 00:00:00 2001 From: Ben Schumacher Date: Tue, 15 Mar 2022 18:00:27 +0100 Subject: [PATCH] [MM-40735] Add MattermostAppID to OAuth app (#19214) --- app/oauth.go | 18 +++++++------ ...78_create_oauth_mattermost_app_id.down.sql | 14 ++++++++++ ...0078_create_oauth_mattermost_app_id.up.sql | 14 ++++++++++ ...78_create_oauth_mattermost_app_id.down.sql | 1 + ...0078_create_oauth_mattermost_app_id.up.sql | 1 + i18n/en.json | 4 +++ model/oauth.go | 27 +++++++++++-------- model/oauth_test.go | 3 +++ model/session.go | 2 ++ store/sqlstore/oauth_store.go | 6 ++--- store/storetest/oauth_store.go | 7 ++++- 11 files changed, 74 insertions(+), 23 deletions(-) create mode 100644 db/migrations/mysql/000078_create_oauth_mattermost_app_id.down.sql create mode 100644 db/migrations/mysql/000078_create_oauth_mattermost_app_id.up.sql create mode 100644 db/migrations/postgres/000078_create_oauth_mattermost_app_id.down.sql create mode 100644 db/migrations/postgres/000078_create_oauth_mattermost_app_id.up.sql diff --git a/app/oauth.go b/app/oauth.go index 9e3bbc9dd9..b1a565159b 100644 --- a/app/oauth.go +++ b/app/oauth.go @@ -244,7 +244,7 @@ func (a *App) GetOAuthAccessTokenForImplicitFlow(userID string, authRequest *mod return nil, err } - session, err := a.newSession(oauthApp.Name, user) + session, err := a.newSession(oauthApp, user) if err != nil { return nil, err } @@ -306,7 +306,7 @@ func (a *App) GetOAuthAccessTokenForCodeFlow(clientId, grantType, redirectURI, c if accessData != nil { if accessData.IsExpired() { var access *model.AccessResponse - access, err := a.newSessionUpdateToken(oauthApp.Name, accessData, user) + access, err := a.newSessionUpdateToken(oauthApp, accessData, user) if err != nil { return nil, err } @@ -323,7 +323,7 @@ func (a *App) GetOAuthAccessTokenForCodeFlow(clientId, grantType, redirectURI, c } else { var session *model.Session // Create a new session and return new access token - session, err := a.newSession(oauthApp.Name, user) + session, err := a.newSession(oauthApp, user) if err != nil { return nil, err } @@ -357,7 +357,7 @@ func (a *App) GetOAuthAccessTokenForCodeFlow(clientId, grantType, redirectURI, c return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.internal_user.app_error", nil, "", http.StatusNotFound) } - access, err := a.newSessionUpdateToken(oauthApp.Name, accessData, user) + access, err := a.newSessionUpdateToken(oauthApp, accessData, user) if err != nil { return nil, err } @@ -367,12 +367,14 @@ func (a *App) GetOAuthAccessTokenForCodeFlow(clientId, grantType, redirectURI, c return accessRsp, nil } -func (a *App) newSession(appName string, user *model.User) (*model.Session, *model.AppError) { +func (a *App) newSession(app *model.OAuthApp, user *model.User) (*model.Session, *model.AppError) { // Set new token an session session := &model.Session{UserId: user.Id, Roles: user.Roles, IsOAuth: true} session.GenerateCSRF() a.ch.srv.userService.SetSessionExpireInDays(session, *a.Config().ServiceSettings.SessionLengthSSOInDays) - session.AddProp(model.SessionPropPlatform, appName) + session.AddProp(model.SessionPropPlatform, app.Name) + session.AddProp(model.SessionPropOAuthAppID, app.Id) + session.AddProp(model.SessionPropMattermostAppID, app.MattermostAppID) session.AddProp(model.SessionPropOs, "OAuth2") session.AddProp(model.SessionPropBrowser, "OAuth2") @@ -386,13 +388,13 @@ func (a *App) newSession(appName string, user *model.User) (*model.Session, *mod return session, nil } -func (a *App) newSessionUpdateToken(appName string, accessData *model.AccessData, user *model.User) (*model.AccessResponse, *model.AppError) { +func (a *App) newSessionUpdateToken(app *model.OAuthApp, accessData *model.AccessData, user *model.User) (*model.AccessResponse, *model.AppError) { // Remove the previous session if err := a.Srv().Store.Session().Remove(accessData.Token); err != nil { mlog.Warn("error removing access data token from session", mlog.Err(err)) } - session, err := a.newSession(appName, user) + session, err := a.newSession(app, user) if err != nil { return nil, err } diff --git a/db/migrations/mysql/000078_create_oauth_mattermost_app_id.down.sql b/db/migrations/mysql/000078_create_oauth_mattermost_app_id.down.sql new file mode 100644 index 0000000000..0aaaaa3a49 --- /dev/null +++ b/db/migrations/mysql/000078_create_oauth_mattermost_app_id.down.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS + WHERE table_name = 'OAuthApps' + AND table_schema = DATABASE() + AND column_name = 'MattermostAppID' + ) > 0, + 'ALTER TABLE OAuthApps DROP COLUMN MattermostAppID;', + 'SELECT 1' +)); + +PREPARE alterIfExists FROM @preparedStatement; +EXECUTE alterIfExists; +DEALLOCATE PREPARE alterIfExists; diff --git a/db/migrations/mysql/000078_create_oauth_mattermost_app_id.up.sql b/db/migrations/mysql/000078_create_oauth_mattermost_app_id.up.sql new file mode 100644 index 0000000000..5e68967e85 --- /dev/null +++ b/db/migrations/mysql/000078_create_oauth_mattermost_app_id.up.sql @@ -0,0 +1,14 @@ +SET @preparedStatement = (SELECT IF( + ( + SELECT COUNT(*) FROM INFORMATION_SCHEMA.COLUMNS + WHERE table_name = 'OAuthApps' + AND table_schema = DATABASE() + AND column_name = 'MattermostAppID' + ) > 0, + 'SELECT 1', + 'ALTER TABLE OAuthApps ADD COLUMN MattermostAppID varchar(32);' +)); + +PREPARE alterIfExists FROM @preparedStatement; +EXECUTE alterIfExists; +DEALLOCATE PREPARE alterIfExists; diff --git a/db/migrations/postgres/000078_create_oauth_mattermost_app_id.down.sql b/db/migrations/postgres/000078_create_oauth_mattermost_app_id.down.sql new file mode 100644 index 0000000000..5c8270acff --- /dev/null +++ b/db/migrations/postgres/000078_create_oauth_mattermost_app_id.down.sql @@ -0,0 +1 @@ +ALTER TABLE OAuthApps DROP COLUMN IF EXISTS MattermostAppID; diff --git a/db/migrations/postgres/000078_create_oauth_mattermost_app_id.up.sql b/db/migrations/postgres/000078_create_oauth_mattermost_app_id.up.sql new file mode 100644 index 0000000000..c4501d89c5 --- /dev/null +++ b/db/migrations/postgres/000078_create_oauth_mattermost_app_id.up.sql @@ -0,0 +1 @@ +ALTER TABLE OAuthApps ADD COLUMN IF NOT EXISTS MattermostAppID varchar(32); diff --git a/i18n/en.json b/i18n/en.json index fa9cb87050..33257eea06 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -8671,6 +8671,10 @@ "id": "model.oauth.is_valid.icon_url.app_error", "translation": "Icon URL must be a valid URL and start with http:// or https://." }, + { + "id": "model.oauth.is_valid.mattermost_app_id.app_error", + "translation": "The maximum length of MattermostAppID is 32 characters." + }, { "id": "model.oauth.is_valid.name.app_error", "translation": "Invalid name." diff --git a/model/oauth.go b/model/oauth.go index a3d7f95fac..82dd8fb2f9 100644 --- a/model/oauth.go +++ b/model/oauth.go @@ -18,17 +18,18 @@ const ( ) type OAuthApp struct { - Id string `json:"id"` - CreatorId string `json:"creator_id"` - CreateAt int64 `json:"create_at"` - UpdateAt int64 `json:"update_at"` - ClientSecret string `json:"client_secret"` - Name string `json:"name"` - Description string `json:"description"` - IconURL string `json:"icon_url"` - CallbackUrls StringArray `json:"callback_urls"` - Homepage string `json:"homepage"` - IsTrusted bool `json:"is_trusted"` + Id string `json:"id"` + CreatorId string `json:"creator_id"` + CreateAt int64 `json:"create_at"` + UpdateAt int64 `json:"update_at"` + ClientSecret string `json:"client_secret"` + Name string `json:"name"` + Description string `json:"description"` + IconURL string `json:"icon_url"` + CallbackUrls StringArray `json:"callback_urls"` + Homepage string `json:"homepage"` + IsTrusted bool `json:"is_trusted"` + MattermostAppID string `json:"mattermost_app_id"` } // IsValid validates the app and returns an error if it isn't configured @@ -83,6 +84,10 @@ func (a *OAuthApp) IsValid() *AppError { } } + if len(a.MattermostAppID) > 32 { + return NewAppError("OAuthApp.IsValid", "model.oauth.is_valid.mattermost_app_id.app_error", nil, "app_id="+a.Id, http.StatusBadRequest) + } + return nil } diff --git a/model/oauth_test.go b/model/oauth_test.go index f3255865c8..bb6fab861d 100644 --- a/model/oauth_test.go +++ b/model/oauth_test.go @@ -59,6 +59,9 @@ func TestOAuthAppIsValid(t *testing.T) { app.CallbackUrls = []string{"https://nowhere.com"} require.NotNil(t, app.IsValid()) + app.MattermostAppID = "Some app ID" + require.NotNil(t, app.IsValid()) + app.Homepage = "https://nowhere.com" require.Nil(t, app.IsValid()) diff --git a/model/session.go b/model/session.go index c2194aefa6..fcf8eea12d 100644 --- a/model/session.go +++ b/model/session.go @@ -24,6 +24,8 @@ const ( SessionPropUserAccessTokenId = "user_access_token_id" SessionPropIsBot = "is_bot" SessionPropIsBotValue = "true" + SessionPropOAuthAppID = "oauth_app_id" + SessionPropMattermostAppID = "mattermost_app_id" SessionTypeUserAccessToken = "UserAccessToken" SessionTypeCloudKey = "CloudKey" SessionTypeRemoteclusterToken = "RemoteClusterToken" diff --git a/store/sqlstore/oauth_store.go b/store/sqlstore/oauth_store.go index 4bcad0b91a..dec6db893e 100644 --- a/store/sqlstore/oauth_store.go +++ b/store/sqlstore/oauth_store.go @@ -32,9 +32,9 @@ func (as SqlOAuthStore) SaveApp(app *model.OAuthApp) (*model.OAuthApp, error) { } if _, err := as.GetMasterX().NamedExec(`INSERT INTO OAuthApps - (Id, CreatorId, CreateAt, UpdateAt, ClientSecret, Name, Description, IconURL, CallbackUrls, Homepage, IsTrusted) + (Id, CreatorId, CreateAt, UpdateAt, ClientSecret, Name, Description, IconURL, CallbackUrls, Homepage, IsTrusted, MattermostAppID) VALUES - (:Id, :CreatorId, :CreateAt, :UpdateAt, :ClientSecret, :Name, :Description, :IconURL, :CallbackUrls, :Homepage, :IsTrusted)`, app); err != nil { + (:Id, :CreatorId, :CreateAt, :UpdateAt, :ClientSecret, :Name, :Description, :IconURL, :CallbackUrls, :Homepage, :IsTrusted, :MattermostAppID)`, app); err != nil { return nil, errors.Wrap(err, "failed to save OAuthApp") } return app, nil @@ -63,7 +63,7 @@ func (as SqlOAuthStore) UpdateApp(app *model.OAuthApp) (*model.OAuthApp, error) res, err := as.GetMasterX().NamedExec(`UPDATE OAuthApps SET UpdateAt=:UpdateAt, ClientSecret=:ClientSecret, Name=:Name, Description=:Description, IconURL=:IconURL, CallbackUrls=:CallbackUrls, - Homepage=:Homepage, IsTrusted=:IsTrusted + Homepage=:Homepage, IsTrusted=:IsTrusted, MattermostAppID=:MattermostAppID WHERE Id=:Id`, app) if err != nil { return nil, errors.Wrapf(err, "failed to update OAuthApp with id=%s", app.Id) diff --git a/store/storetest/oauth_store.go b/store/storetest/oauth_store.go index 991dc431ac..c45136d290 100644 --- a/store/storetest/oauth_store.go +++ b/store/storetest/oauth_store.go @@ -48,9 +48,14 @@ func testOAuthStoreSaveApp(t *testing.T, ss store.Store) { _, err = ss.OAuth().SaveApp(&a1) require.Error(t, err, "Should have failed, app should be invalid cause it doesn' have a name set") + a1.Name = "TestApp" + model.NewId() // Valid name + a1.MattermostAppID = "a very, very, very, very, very, very, very long id" + _, err = ss.OAuth().SaveApp(&a1) + require.Error(t, err, "Should have failed, app should be invalid cause the MattermostAppID is to long") + // Save the app a1.Id = "" - a1.Name = "TestApp" + model.NewId() + a1.MattermostAppID = "some small id" // Valid id _, err = ss.OAuth().SaveApp(&a1) require.NoError(t, err) }