From 5563e095932759ecee7783e2820184fe4f50c1a5 Mon Sep 17 00:00:00 2001 From: Maria A Nunez Date: Wed, 3 Jun 2026 02:54:16 -0400 Subject: [PATCH] MM-68983: Tighten OAuth token issuance and cleanup on user deactivation (#36743) (#36853) Automatic Merge --- server/channels/app/oauth.go | 8 +++ server/channels/app/oauth_test.go | 105 ++++++++++++++++++++++++++++++ server/channels/app/user.go | 4 ++ server/channels/app/user_test.go | 66 +++++++++++++++++++ 4 files changed, 183 insertions(+) diff --git a/server/channels/app/oauth.go b/server/channels/app/oauth.go index e676ad365e..278a9ae957 100644 --- a/server/channels/app/oauth.go +++ b/server/channels/app/oauth.go @@ -256,6 +256,10 @@ func (a *App) GetOAuthAccessTokenForImplicitFlow(c request.CTX, userID string, a return nil, err } + if user.DeleteAt != 0 { + return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.expired_code.app_error", nil, "", http.StatusForbidden) + } + session, err := a.newSession(c, oauthApp, user) if err != nil { return nil, err @@ -381,6 +385,10 @@ func (a *App) GetOAuthAccessTokenForCodeFlow(c request.CTX, clientId, grantType, return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.internal_user.app_error", nil, "", http.StatusNotFound).Wrap(nErr) } + if user.DeleteAt != 0 { + return nil, model.NewAppError("GetOAuthAccessToken", "api.oauth.get_access_token.expired_code.app_error", nil, "", http.StatusForbidden) + } + access, err := a.newSessionUpdateToken(c, oauthApp, accessData, user) if err != nil { return nil, err diff --git a/server/channels/app/oauth_test.go b/server/channels/app/oauth_test.go index 18c3237724..147e5dddad 100644 --- a/server/channels/app/oauth_test.go +++ b/server/channels/app/oauth_test.go @@ -793,6 +793,111 @@ func TestDifferentClientCannotUseRefreshToken(t *testing.T) { require.Equal(t, http.StatusBadRequest, appErr.StatusCode) } +func TestOAuthRefreshTokenGrantRejectsDeactivatedUser(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOAuthServiceProvider = true }) + + oapp := &model.OAuthApp{ + Name: "RefreshGrantDeactivated_" + model.NewRandomString(10), + CreatorId: th.BasicUser2.Id, + Homepage: "https://nowhere.com", + Description: "test", + CallbackUrls: []string{"https://example.com/callback"}, + ClientSecret: model.NewId(), + } + oapp, appErr := th.App.CreateOAuthApp(oapp) + require.Nil(t, appErr) + + user := th.CreateUser() + + authRequest := &model.AuthorizeRequest{ + ResponseType: model.AuthCodeResponseType, + ClientId: oapp.Id, + RedirectURI: oapp.CallbackUrls[0], + Scope: "user", + State: "test_state", + } + + redirectURL, appErr := th.App.AllowOAuthAppAccessToUser(th.Context, user.Id, authRequest) + require.Nil(t, appErr) + + uri, parseErr := url.Parse(redirectURL) + require.NoError(t, parseErr) + code := uri.Query().Get("code") + require.NotEmpty(t, code) + + tokenResp, appErr := th.App.GetOAuthAccessTokenForCodeFlow( + th.Context, + oapp.Id, + model.AccessTokenGrantType, + oapp.CallbackUrls[0], + code, + oapp.ClientSecret, + "", + ) + require.Nil(t, appErr) + require.NotEmpty(t, tokenResp.AccessToken) + require.NotEmpty(t, tokenResp.RefreshToken) + + require.NoError(t, th.App.Srv().Store().Session().Remove(tokenResp.AccessToken)) + + _, appErr = th.App.UpdateActive(th.Context, user, false) + require.Nil(t, appErr) + + refreshResp, appErr := th.App.GetOAuthAccessTokenForCodeFlow( + th.Context, + oapp.Id, + model.RefreshTokenGrantType, + oapp.CallbackUrls[0], + "", + oapp.ClientSecret, + tokenResp.RefreshToken, + ) + require.NotNil(t, appErr, "refresh token grant must fail for an inactive user") + require.Nil(t, refreshResp) +} + +func TestOAuthImplicitGrantRejectsDeactivatedUser(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOAuthServiceProvider = true }) + + oapp := &model.OAuthApp{ + Name: "ImplicitGrantDeactivated_" + model.NewRandomString(10), + CreatorId: th.BasicUser2.Id, + Homepage: "https://nowhere.com", + Description: "test", + CallbackUrls: []string{"https://example.com/callback"}, + ClientSecret: model.NewId(), + } + oapp, appErr := th.App.CreateOAuthApp(oapp) + require.Nil(t, appErr) + + user := th.CreateUser() + + _, appErr = th.App.UpdateActive(th.Context, user, false) + require.Nil(t, appErr) + + authRequest := &model.AuthorizeRequest{ + ResponseType: model.ImplicitResponseType, + ClientId: oapp.Id, + RedirectURI: oapp.CallbackUrls[0], + Scope: "user", + State: "test_state", + } + + session, appErr := th.App.GetOAuthAccessTokenForImplicitFlow(th.Context, user.Id, authRequest) + require.NotNil(t, appErr, "implicit grant must fail for an inactive user") + require.Nil(t, session) + + accessData, sErr := th.App.Srv().Store().OAuth().GetAccessDataByUserForApp(user.Id, oapp.Id) + require.NoError(t, sErr) + require.Empty(t, accessData, "no access data may be persisted for an inactive user") +} + func TestParseOAuthStateTokenExtra(t *testing.T) { t.Run("valid token with normal values", func(t *testing.T) { email, action, cookie, err := parseOAuthStateTokenExtra("user@example.com:email_to_sso:randomcookie123") diff --git a/server/channels/app/user.go b/server/channels/app/user.go index d873f4b430..08509e819e 100644 --- a/server/channels/app/user.go +++ b/server/channels/app/user.go @@ -1037,6 +1037,10 @@ func (a *App) userDeactivated(c request.CTX, userID string) *model.AppError { c.Logger().Warn("unable to remove auth data by user id", mlog.Err(nErr)) } + if nErr := a.Srv().Store().OAuth().PermanentDeleteAuthDataByUser(userID); nErr != nil { + c.Logger().Warn("unable to remove oauth access data by user id", mlog.Err(nErr)) + } + return nil } diff --git a/server/channels/app/user_test.go b/server/channels/app/user_test.go index 0306614901..b4703b4949 100644 --- a/server/channels/app/user_test.go +++ b/server/channels/app/user_test.go @@ -10,6 +10,7 @@ import ( "encoding/json" "errors" "net/http" + "net/url" "os" "path/filepath" "strings" @@ -420,6 +421,71 @@ func TestUpdateActiveBotsSideEffect(t *testing.T) { require.Nil(t, appErr) } +func TestUserDeactivationRevokesOAuthAccessTokens(t *testing.T) { + mainHelper.Parallel(t) + th := Setup(t).InitBasic() + + th.App.UpdateConfig(func(cfg *model.Config) { *cfg.ServiceSettings.EnableOAuthServiceProvider = true }) + + oapp := &model.OAuthApp{ + Name: "DeactivationCleanup_" + model.NewRandomString(10), + CreatorId: th.BasicUser2.Id, + Homepage: "https://nowhere.com", + Description: "test", + CallbackUrls: []string{"https://example.com/callback"}, + ClientSecret: model.NewId(), + } + oapp, appErr := th.App.CreateOAuthApp(oapp) + require.Nil(t, appErr) + + user := th.CreateUser() + + authRequest := &model.AuthorizeRequest{ + ResponseType: model.AuthCodeResponseType, + ClientId: oapp.Id, + RedirectURI: oapp.CallbackUrls[0], + Scope: "user", + State: "test_state", + } + + redirectURL, appErr := th.App.AllowOAuthAppAccessToUser(th.Context, user.Id, authRequest) + require.Nil(t, appErr) + + uri, parseErr := url.Parse(redirectURL) + require.NoError(t, parseErr) + code := uri.Query().Get("code") + require.NotEmpty(t, code) + + tokenResp, appErr := th.App.GetOAuthAccessTokenForCodeFlow( + th.Context, + oapp.Id, + model.AccessTokenGrantType, + oapp.CallbackUrls[0], + code, + oapp.ClientSecret, + "", + ) + require.Nil(t, appErr) + require.NotEmpty(t, tokenResp.AccessToken) + require.NotEmpty(t, tokenResp.RefreshToken) + + require.NoError(t, th.App.Srv().Store().Session().Remove(tokenResp.AccessToken)) + + preDeactivation, sErr := th.App.Srv().Store().OAuth().GetAccessDataByUserForApp(user.Id, oapp.Id) + require.NoError(t, sErr) + require.NotEmpty(t, preDeactivation) + + _, appErr = th.App.UpdateActive(th.Context, user, false) + require.Nil(t, appErr) + + postDeactivation, sErr := th.App.Srv().Store().OAuth().GetAccessDataByUserForApp(user.Id, oapp.Id) + require.NoError(t, sErr) + require.Empty(t, postDeactivation, "oauth access tokens for an inactive user must be removed") + + _, sErr = th.App.Srv().Store().OAuth().GetAccessDataByRefreshToken(tokenResp.RefreshToken) + require.Error(t, sErr, "refresh token row for an inactive user must be removed") +} + func TestUpdateOAuthUserAttrs(t *testing.T) { mainHelper.Parallel(t) th := Setup(t)