From cca466caaed2ec4917d8acad4d5899ad2068c7a8 Mon Sep 17 00:00:00 2001 From: bircni Date: Sat, 3 Oct 2026 19:22:07 +0200 Subject: [PATCH] fix(api): allow bots with pending password changes (#39551) Allow bot accounts to use the API when a legacy password-change flag is set, since bots cannot complete the interactive password-change flow. Preserve password-change enforcement for human accounts and restrictions for inactive or prohibited accounts. Fixes: https://github.com/go-gitea/gitea/issues/39542 Co-authored-by: wxiaoguang Co-authored-by: silverwind --- routers/api/v1/api.go | 52 ++++++++++----------------- routers/common/auth.go | 15 ++++++++ routers/common/auth_test.go | 53 ++++++++++++++++++++++++++++ routers/web/auth/oauth.go | 2 +- routers/web/auth/password.go | 8 ++--- routers/web/home.go | 26 ++------------ routers/web/web.go | 50 ++++++++++---------------- services/context/context.go | 5 --- services/context/context_template.go | 2 +- services/context/user.go | 5 +++ tests/integration/admin_user_test.go | 11 ++++++ 11 files changed, 129 insertions(+), 100 deletions(-) create mode 100644 routers/common/auth_test.go diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 8953647a99a..9cc64adccad 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -76,6 +76,7 @@ import ( repo_model "gitea.dev/models/repo" "gitea.dev/models/unit" user_model "gitea.dev/models/user" + "gitea.dev/modules/httplib" "gitea.dev/modules/log" "gitea.dev/modules/setting" api "gitea.dev/modules/structs" @@ -935,31 +936,22 @@ func apiAuth(authMethod auth.Method) func(*context.APIContext) { } } -// verifyAuthWithOptions checks authentication according to options -func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.APIContext) { +// verifyAuthWithOptionsAPI checks authentication according to options +func verifyAuthWithOptionsAPI(options *common.VerifyOptions) func(ctx *context.APIContext) { return func(ctx *context.APIContext) { // Check prohibit login users. if ctx.IsSigned { - if !ctx.Doer.IsActive && setting.Service.RegisterEmailConfirm { - ctx.Data["Title"] = ctx.Tr("auth.active_your_account") - ctx.JSON(http.StatusForbidden, map[string]string{ - "message": "This account is not activated.", - }) + check := common.CheckSignedInUser(ctx.Doer, nil) + if check.NeedActivateAccount { + ctx.JSON(http.StatusForbidden, map[string]string{"message": "This account is not activated."}) return - } - if !ctx.Doer.IsActive || ctx.Doer.ProhibitLogin { + } else if check.LoginIsProhibited { log.Info("Failed authentication attempt for %s from %s", ctx.Doer.Name, ctx.RemoteAddr()) - ctx.Data["Title"] = ctx.Tr("auth.prohibit_login") - ctx.JSON(http.StatusForbidden, map[string]string{ - "message": "This account is prohibited from signing in, please contact your site administrator.", - }) + ctx.JSON(http.StatusForbidden, map[string]string{"message": "This account is prohibited from signing in, please contact your site administrator."}) return - } - - if ctx.Doer.MustChangePassword { - ctx.JSON(http.StatusForbidden, map[string]string{ - "message": "You must change your password. Change it at: " + setting.AppURL + "/user/change_password", - }) + } else if check.NeedChangePassword { + msg := "You must change your password. Change it at: " + httplib.MakeAbsoluteURL(ctx, setting.AppSubURL+"/user/settings/change_password") + ctx.JSON(http.StatusForbidden, map[string]string{"message": msg}) return } } @@ -970,20 +962,12 @@ func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.APIC return } - if options.SignInRequired { - if !ctx.IsSigned { - // Restrict API calls with error message. - ctx.JSON(http.StatusForbidden, map[string]string{ - "message": "Only signed in user is allowed to call APIs.", - }) - return - } else if !ctx.Doer.IsActive && setting.Service.RegisterEmailConfirm { - ctx.Data["Title"] = ctx.Tr("auth.active_your_account") - ctx.JSON(http.StatusForbidden, map[string]string{ - "message": "This account is not activated.", - }) - return - } + if options.SignInRequired && !ctx.IsSigned { + // Restrict API calls with error message. + ctx.JSON(http.StatusForbidden, map[string]string{ + "message": "Only signed in user is allowed to call APIs.", + }) + return } if options.AdminRequired { @@ -1035,7 +1019,7 @@ func Routes() *web.Router { // Get user from session if logged in. m.AfterRouting(apiAuth(buildAuthGroup())) - m.AfterRouting(verifyAuthWithOptions(&common.VerifyOptions{ + m.AfterRouting(verifyAuthWithOptionsAPI(&common.VerifyOptions{ SignInRequired: setting.Service.RequireSignInViewStrict, })) diff --git a/routers/common/auth.go b/routers/common/auth.go index 0520476f28a..14bfe09bd35 100644 --- a/routers/common/auth.go +++ b/routers/common/auth.go @@ -6,6 +6,8 @@ package common import ( user_model "gitea.dev/models/user" "gitea.dev/modules/log" + "gitea.dev/modules/session" + "gitea.dev/modules/setting" "gitea.dev/modules/web/middleware" auth_service "gitea.dev/services/auth" "gitea.dev/services/context" @@ -56,3 +58,16 @@ type VerifyOptions struct { AdminRequired bool DisableCrossOriginProtection bool } + +func CheckSignedInUser(doer *user_model.User, sess session.Store) (ret struct { + NeedActivateAccount bool + LoginIsProhibited bool + NeedChangePassword bool +}, +) { + ret.NeedActivateAccount = !doer.IsActive && setting.Service.RegisterEmailConfirm + ret.LoginIsProhibited = !doer.IsActive || doer.ProhibitLogin + isImpersonated := sess != nil && context.IsDoerSessionImpersonated(sess) + ret.NeedChangePassword = doer.MustChangePassword && !isImpersonated && !doer.IsTypeBot() + return ret +} diff --git a/routers/common/auth_test.go b/routers/common/auth_test.go new file mode 100644 index 00000000000..464bab25b71 --- /dev/null +++ b/routers/common/auth_test.go @@ -0,0 +1,53 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package common + +import ( + "testing" + + user_model "gitea.dev/models/user" + "gitea.dev/modules/session" + "gitea.dev/modules/setting" + "gitea.dev/modules/test" + + "github.com/stretchr/testify/assert" +) + +func TestCheckSignedInUser(t *testing.T) { + defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm)() + sessNormal := session.NewMockMemStore("session-a") + sessImpersonated := session.NewMockMemStore("session-b") + _ = sessImpersonated.Set(session.KeyImpersonatorData, "any-value") + + setting.Service.RegisterEmailConfirm = false + ret := CheckSignedInUser(&user_model.User{IsActive: false}, nil) + assert.False(t, ret.NeedActivateAccount) + assert.True(t, ret.LoginIsProhibited) + + setting.Service.RegisterEmailConfirm = true + ret = CheckSignedInUser(&user_model.User{IsActive: false}, nil) + assert.True(t, ret.NeedActivateAccount) + assert.True(t, ret.LoginIsProhibited) + + ret = CheckSignedInUser(&user_model.User{IsActive: true}, nil) + assert.False(t, ret.NeedActivateAccount) + assert.False(t, ret.LoginIsProhibited) + assert.False(t, ret.NeedChangePassword) + + ret = CheckSignedInUser(&user_model.User{IsActive: true, ProhibitLogin: true}, nil) + assert.False(t, ret.NeedActivateAccount) + assert.True(t, ret.LoginIsProhibited) + + ret = CheckSignedInUser(&user_model.User{MustChangePassword: true}, nil) + assert.True(t, ret.NeedChangePassword) + + ret = CheckSignedInUser(&user_model.User{MustChangePassword: true}, sessNormal) + assert.True(t, ret.NeedChangePassword) + + ret = CheckSignedInUser(&user_model.User{MustChangePassword: true, Type: user_model.UserTypeBot}, sessNormal) + assert.False(t, ret.NeedChangePassword) + + ret = CheckSignedInUser(&user_model.User{MustChangePassword: true}, sessImpersonated) + assert.False(t, ret.NeedChangePassword) +} diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 4e0a355d87e..89f6f7903a9 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -374,7 +374,7 @@ func handleOAuth2SignIn(ctx *context.Context, authSource *auth.Source, u *user_m // Reactivate user only if they were disabled by the OAuth2 auto sync cron (invalid_grant), // which clears AccessToken/RefreshToken/ExpiresAt on the ExternalLoginUser row // An admin-disabled user has no such signature, so we leave IsActive alone - // and let verifyAuthWithOptions route them through the prohibit-login / activate page. + // and let verifyAuthWithOptionsWeb route them through the prohibit-login / activate page. if !u.IsActive { extLogin, hasExt, err := user_model.GetExternalLogin(ctx, authSource.ID, gothUser.UserID) if err != nil { diff --git a/routers/web/auth/password.go b/routers/web/auth/password.go index 1f4cebdef30..883f4d6a17b 100644 --- a/routers/web/auth/password.go +++ b/routers/web/auth/password.go @@ -17,6 +17,7 @@ import ( "gitea.dev/modules/templates" "gitea.dev/modules/timeutil" "gitea.dev/modules/web" + "gitea.dev/routers/common" "gitea.dev/services/audit" "gitea.dev/services/context" "gitea.dev/services/forms" @@ -282,10 +283,9 @@ func MustChangePasswordPost(ctx *context.Context) { return } - // Make sure only requests for users who are eligible to change their password via - // this method passes through - if !ctx.Doer.MustChangePassword { - ctx.ServerError("MustUpdatePassword", errors.New("cannot update password. Please visit the settings page")) + if !common.CheckSignedInUser(ctx.Doer, ctx.Session).NeedChangePassword { + log.Debug("User %s attempted to access the must change password page, but they are not required to change their password", ctx.Doer.Name) + ctx.NotFound(nil) return } diff --git a/routers/web/home.go b/routers/web/home.go index 38496aaa28a..c0756294ba9 100644 --- a/routers/web/home.go +++ b/routers/web/home.go @@ -17,38 +17,18 @@ import ( "gitea.dev/modules/sitemap" "gitea.dev/modules/structs" "gitea.dev/modules/templates" - "gitea.dev/modules/web/middleware" - "gitea.dev/routers/web/auth" "gitea.dev/routers/web/user" "gitea.dev/services/context" ) -const ( - // tplHome home page template - tplHome templates.TplName = "home" -) +const tplHome templates.TplName = "home" -// Home render home page func Home(ctx *context.Context) { if ctx.IsSigned { - if !ctx.Doer.IsActive && setting.Service.RegisterEmailConfirm { - ctx.Data["Title"] = ctx.Tr("auth.active_your_account") - ctx.HTML(http.StatusOK, auth.TplActivate) - } else if !ctx.Doer.IsActive || ctx.Doer.ProhibitLogin { - log.Info("Failed authentication attempt for %s from %s", ctx.Doer.Name, ctx.RemoteAddr()) - ctx.Data["Title"] = ctx.Tr("auth.prohibit_login") - ctx.HTML(http.StatusOK, "user/auth/prohibit_login") - } else if doerMustChangePassword(ctx) { - ctx.Data["Title"] = ctx.Tr("auth.must_change_password") - ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/change_password" - middleware.SetRedirectToCookie(ctx.Resp, setting.AppSubURL+ctx.Req.URL.RequestURI()) - ctx.Redirect(setting.AppSubURL + "/user/settings/change_password") - } else { - user.Dashboard(ctx) - } + user.Dashboard(ctx) return - // Check non-logged users landing page. } else if setting.LandingPageURL != setting.LandingPageHome { + // Check non-logged users landing page ctx.Redirect(setting.AppSubURL + string(setting.LandingPageURL)) return } diff --git a/routers/web/web.go b/routers/web/web.go index 2cadc573d09..696ad97724a 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -172,38 +172,29 @@ func newWebAuthMiddleware() *AuthMiddleware { return webAuth } -func doerMustChangePassword(ctx *context.Context) bool { - // an impersonating admin must not be forced to set the impersonated user's password - return ctx.Doer != nil && ctx.Doer.MustChangePassword && !ctx.DoerIsImpersonated() -} - -// verifyAuthWithOptions checks authentication according to options -func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.Context) { +// verifyAuthWithOptionsWeb checks authentication according to options +func verifyAuthWithOptionsWeb(options *common.VerifyOptions) func(ctx *context.Context) { crossOriginProtection := http.NewCrossOriginProtection() return func(ctx *context.Context) { // Check prohibit login users. if ctx.IsSigned { - if !ctx.Doer.IsActive && setting.Service.RegisterEmailConfirm { + check := common.CheckSignedInUser(ctx.Doer, ctx.Session) + if check.NeedActivateAccount { ctx.Data["Title"] = ctx.Tr("auth.active_your_account") ctx.HTML(http.StatusOK, "user/auth/activate") return - } - if !ctx.Doer.IsActive || ctx.Doer.ProhibitLogin { + } else if check.LoginIsProhibited { log.Info("Failed authentication attempt for %s from %s", ctx.Doer.Name, ctx.RemoteAddr()) ctx.Data["Title"] = ctx.Tr("auth.prohibit_login") ctx.HTML(http.StatusOK, "user/auth/prohibit_login") return - } - - if doerMustChangePassword(ctx) { + } else if check.NeedChangePassword { if ctx.Req.URL.Path != "/user/settings/change_password" { if strings.HasPrefix(ctx.Req.UserAgent(), "git") { ctx.HTTPError(http.StatusUnauthorized, ctx.Locale.TrString("auth.must_change_password")) return } - ctx.Data["Title"] = ctx.Tr("auth.must_change_password") - ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/change_password" middleware.SetRedirectToCookie(ctx.Resp, setting.AppSubURL+ctx.Req.URL.RequestURI()) ctx.Redirect(setting.AppSubURL + "/user/settings/change_password") return @@ -228,15 +219,9 @@ func verifyAuthWithOptions(options *common.VerifyOptions) func(ctx *context.Cont } } - if options.SignInRequired { - if !ctx.IsSigned { - ctx.Redirect(middleware.RedirectLinkUserLogin(ctx.Req)) - return - } else if !ctx.Doer.IsActive && setting.Service.RegisterEmailConfirm { - ctx.Data["Title"] = ctx.Tr("auth.active_your_account") - ctx.HTML(http.StatusOK, "user/auth/activate") - return - } + if options.SignInRequired && !ctx.IsSigned { + ctx.Redirect(middleware.RedirectLinkUserLogin(ctx.Req)) + return } // Redirect to log in page if auto-signin info is provided and has not signed in. @@ -333,7 +318,7 @@ func Routes() *web.Router { // The CORS mechanism already protects cross-origin requests, and the CrossOriginProtection has no "allowed origin" list, so disable CrossOriginProtection. // - For non-browser client requests: git clone via http, no Sec-Fetch-Site header. // Such requests are not cross-origin requests, so disable CrossOriginProtection. -var optSignInFromAnyOrigin = verifyAuthWithOptions(&common.VerifyOptions{DisableCrossOriginProtection: true}) +var optSignInFromAnyOrigin = verifyAuthWithOptionsWeb(&common.VerifyOptions{DisableCrossOriginProtection: true}) // addProjectBoardRoutes registers a board's column and card routes, shared by the // repository and owner mount points. @@ -352,13 +337,14 @@ func addProjectBoardRoutes(m *web.Router) { // registerWebRoutes register routes func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) { // middleware: required to be signed in or signed out - reqSignIn := verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: true}) - reqSignOut := verifyAuthWithOptions(&common.VerifyOptions{SignOutRequired: true}) + reqSignIn := verifyAuthWithOptionsWeb(&common.VerifyOptions{SignInRequired: true}) + reqSignOut := verifyAuthWithOptionsWeb(&common.VerifyOptions{SignOutRequired: true}) // middleware: optional sign in (if signed in, use the user as doer, if not, no doer) - optSignIn := verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: setting.Service.RequireSignInViewStrict}) - optExploreSignIn := verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: setting.Service.RequireSignInViewStrict || setting.Service.Explore.RequireSigninView}) + optSignInHome := verifyAuthWithOptionsWeb(&common.VerifyOptions{SignInRequired: false}) // site home doesn't need "require sign-in" protection + optSignIn := verifyAuthWithOptionsWeb(&common.VerifyOptions{SignInRequired: setting.Service.RequireSignInViewStrict}) + optExploreSignIn := verifyAuthWithOptionsWeb(&common.VerifyOptions{SignInRequired: setting.Service.RequireSignInViewStrict || setting.Service.Explore.RequireSigninView}) // middleware: only apply CrossOriginProtection - crossOriginProtect := verifyAuthWithOptions(&common.VerifyOptions{DisableCrossOriginProtection: false}) + crossOriginProtect := verifyAuthWithOptionsWeb(&common.VerifyOptions{DisableCrossOriginProtection: false}) openIDSignInEnabled := func(ctx *context.Context) { if !setting.Service.EnableOpenIDSignIn { @@ -530,7 +516,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) { // FIXME: not all routes need go through same middleware. // Especially some AJAX requests, we can reduce middleware number to improve performance. - m.Get("/", Home) + m.Get("/", optSignInHome, Home) m.Get("/sitemap.xml", sitemapEnabled, optExploreSignIn, HomeSitemap) m.Group("/.well-known", func() { m.Get("/openid-configuration", auth.OIDCWellKnown) @@ -777,7 +763,7 @@ func registerWebRoutes(m *web.Router, webAuth *AuthMiddleware) { m.Get("/avatar/{hash}", user.AvatarByEmailHash) - adminReq := verifyAuthWithOptions(&common.VerifyOptions{SignInRequired: true, AdminRequired: true}) + adminReq := verifyAuthWithOptionsWeb(&common.VerifyOptions{SignInRequired: true, AdminRequired: true}) // ***** START: Admin ***** m.Group("/-/admin", func() { diff --git a/services/context/context.go b/services/context/context.go index 8969188675a..61e91bfe8db 100644 --- a/services/context/context.go +++ b/services/context/context.go @@ -211,11 +211,6 @@ func (ctx *Context) DoerNeedTwoFactorAuth() bool { return ctx.Session.Get(session.KeyUserHasTwoFactorAuth) == false } -// DoerIsImpersonated returns true if the current session is an admin impersonating the doer -func (ctx *Context) DoerIsImpersonated() bool { - return ctx.Session.Get(session.KeyImpersonatorData) != nil -} - // HasError returns true if error occurs in form validation. // Attention: this function changes ctx.Data and ctx.Flash // If HasError is called, then before Redirect, the error message should be stored by ctx.Flash.Error(ctx.GetErrMsg()) again. diff --git a/services/context/context_template.go b/services/context/context_template.go index 3109559b7e7..e981ad9afe2 100644 --- a/services/context/context_template.go +++ b/services/context/context_template.go @@ -69,7 +69,7 @@ func (c TemplateContext) CurrentWebTheme() *webtheme.ThemeMetaInfo { func (c TemplateContext) ImpersonatedUser() *user_model.User { webCtx := GetWebContext(c) - if webCtx == nil || webCtx.Doer == nil || !webCtx.DoerIsImpersonated() { + if webCtx == nil || webCtx.Doer == nil || !IsDoerSessionImpersonated(webCtx.Session) { return nil } return webCtx.Doer diff --git a/services/context/user.go b/services/context/user.go index d335b9738a5..90b72c0470e 100644 --- a/services/context/user.go +++ b/services/context/user.go @@ -9,6 +9,7 @@ import ( "strings" user_model "gitea.dev/models/user" + "gitea.dev/modules/session" ) // UserAssignmentWeb returns a middleware to handle context-user assignment for web routes @@ -58,3 +59,7 @@ func userAssignment(ctx *Base, doer *user_model.User, errCb func(int, string)) ( } return contextUser } + +func IsDoerSessionImpersonated(sess session.Store) bool { + return sess.Get(session.KeyImpersonatorData) != nil +} diff --git a/tests/integration/admin_user_test.go b/tests/integration/admin_user_test.go index 3b79de1b32f..c70f0094009 100644 --- a/tests/integration/admin_user_test.go +++ b/tests/integration/admin_user_test.go @@ -280,6 +280,17 @@ func TestAdminBotUser(t *testing.T) { } }) + t.Run("TokenIgnoresMustChangePassword", func(t *testing.T) { + bot := unittest.AssertExistsAndLoadBean(t, &user_model.User{LowerName: "bot-user"}) + bot.IsActive, bot.MustChangePassword = true, true + require.NoError(t, user_model.UpdateUserCols(t.Context(), bot, "is_active", "must_change_password")) + token := &auth_model.AccessToken{UID: bot.ID, Name: "git", Scope: auth_model.AccessTokenScopeAll} + require.NoError(t, auth_model.NewAccessToken(t.Context(), token)) + + MakeRequest(t, NewRequest(t, "GET", "/api/v1/repos/user2/repo1").AddTokenAuth(token.Token), http.StatusOK) + MakeRequest(t, NewRequest(t, "GET", "/user2/repo1.git/info/refs?service=git-upload-pack").AddBasicAuth(bot.Name, token.Token), http.StatusOK) + }) + t.Run("APIRejectsAuthSource", func(t *testing.T) { bot := unittest.AssertExistsAndLoadBean(t, &user_model.User{LowerName: "bot-user"}) req := NewRequestWithJSON(t, "PATCH", "/api/v1/admin/users/"+bot.Name, map[string]any{"source_id": 1}).AddBasicAuth("user1")