Reject password reset attempts for OAuth2 users without a current password (#9060)

Currently, if a user signed up via OAuth2 and then somehow gets their E-Mail account compromised, their Forgejo account can be taken over by requesting a password reset for their Forgejo account.
This PR changes the logic so that a password reset request is denied for a user using OAuth2 if they do not already have a password set.
Which should be the case for all users who only ever log in via their Auth-Provider.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/9060
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: BtbN <btbn@btbn.de>
Co-committed-by: BtbN <btbn@btbn.de>

routers/web/auth/password.go

@@ -74,7 +74,7 @@ func ForgotPasswdPost(ctx *context.Context) {
 		return
 	}
 
-	if !u.IsLocal() && !u.IsOAuth2() {
+	if !u.IsLocal() && !(u.IsOAuth2() && u.IsPasswordSet()) {
 		ctx.Data["Err_Email"] = true
 		ctx.RenderWithErr(ctx.Tr("auth.non_local_account"), tplForgotPassword, nil)
 		return
@@ -134,6 +134,11 @@ func commonResetPassword(ctx *context.Context, shouldDeleteToken bool) (*user_mo
 		}
 	}
 
+	if !u.IsLocal() && !(u.IsOAuth2() && u.IsPasswordSet()) {
+		ctx.Flash.Error(ctx.Tr("auth.non_local_account"), true)
+		return nil, nil
+	}
+
 	twofa, err := auth.GetTwoFactorByUID(ctx, u.ID)
 	if err != nil {
 		if !auth.IsErrTwoFactorNotEnrolled(err) {

tests/integration/fixtures/TestUserPasswordResetOAuth2/email_address.yml

@@ -0,0 +1,15 @@
+-
+  id: 1000
+  uid: 1000
+  email: oauth2_user_with_password@example.com
+  lower_email: oauth2_user_with_password@example.com
+  is_activated: true
+  is_primary: true
+
+-
+  id: 1001
+  uid: 1001
+  email: oauth2_user_no_password@example.com
+  lower_email: oauth2_user_no_password@example.com
+  is_activated: true
+  is_primary: true

tests/integration/fixtures/TestUserPasswordResetOAuth2/user.yml

@@ -0,0 +1,77 @@
+-
+  id: 1000
+  lower_name: oauth2_user_with_password
+  name: oauth2_user_with_password
+  full_name: OAuth2 User With Password
+  email: oauth2_user_with_password@example.com
+  keep_email_private: false
+  email_notifications_preference: enabled
+  passwd: ZogKvWdyEx:password
+  passwd_hash_algo: dummy
+  must_change_password: false
+  login_type: 6
+  login_source: 1001
+  login_name: oauth2_user_with_password
+  type: 0
+  salt: ZogKvWdyEx
+  max_repo_creation: -1
+  is_active: true
+  is_admin: false
+  is_restricted: false
+  allow_git_hook: false
+  allow_import_local: false
+  allow_create_organization: true
+  prohibit_login: false
+  avatar: ""
+  avatar_email: oauth2_user_with_password@example.com
+  use_custom_avatar: false
+  num_followers: 0
+  num_following: 0
+  num_stars: 0
+  num_repos: 0
+  num_teams: 0
+  num_members: 0
+  visibility: 0
+  repo_admin_change_team_access: false
+  theme: ""
+  keep_activity_private: false
+  created_unix: 1672578410
+
+-
+  id: 1001
+  lower_name: oauth2_user_no_password
+  name: oauth2_user_no_password
+  full_name: OAuth2 User Without Password
+  email: oauth2_user_no_password@example.com
+  keep_email_private: false
+  email_notifications_preference: enabled
+  passwd: ""
+  passwd_hash_algo: dummy
+  must_change_password: false
+  login_type: 6
+  login_source: 1001
+  login_name: oauth2_user_no_password
+  type: 0
+  salt: ZogKvWdyEx
+  max_repo_creation: -1
+  is_active: true
+  is_admin: false
+  is_restricted: false
+  allow_git_hook: false
+  allow_import_local: false
+  allow_create_organization: true
+  prohibit_login: false
+  avatar: ""
+  avatar_email: oauth2_user_no_password@example.com
+  use_custom_avatar: false
+  num_followers: 0
+  num_following: 0
+  num_stars: 0
+  num_repos: 0
+  num_teams: 0
+  num_members: 0
+  visibility: 0
+  repo_admin_change_team_access: false
+  theme: ""
+  keep_activity_private: false
+  created_unix: 1672578420

tests/integration/user_test.go

@@ -1092,22 +1092,21 @@ func TestUserActivate(t *testing.T) {
 	})
 }
 
-func TestUserPasswordReset(t *testing.T) {
+func parseMailHelper(t *testing.T, expectedTo, expectedSubject string) (cleanup func(), codeRes *string, calledRes *bool) {
-	defer tests.PrepareTestEnv(t)()
+	t.Helper()
-
-	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
 
 	called := false
 	code := ""
-	defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) {
+
+	cleanup = test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) {
 		if called {
 			return
 		}
 		called = true
 
 		assert.Len(t, msgs, 1)
-		assert.Equal(t, user2.EmailTo(), msgs[0].To)
+		assert.Equal(t, expectedTo, msgs[0].To)
-		assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.reset_password"), msgs[0].Subject)
+		assert.Equal(t, expectedSubject, msgs[0].Subject)
 
 		messageDoc := NewHTMLParser(t, bytes.NewBuffer([]byte(msgs[0].Body)))
 		link, ok := messageDoc.Find("a").Attr("href")
@@ -1115,7 +1114,18 @@ func TestUserPasswordReset(t *testing.T) {
 		u, err := url.Parse(link)
 		require.NoError(t, err)
 		code = u.Query()["code"][0]
-	})()
+	})
+
+	return cleanup, &code, &called
+}
+
+func TestUserPasswordReset(t *testing.T) {
+	defer tests.PrepareTestEnv(t)()
+
+	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+	cleanup, code, called := parseMailHelper(t, user2.EmailTo(), string(translation.NewLocale("en-US").Tr("mail.reset_password")))
+	defer cleanup()
 
 	session := emptyTestSession(t)
 	req := NewRequestWithValues(t, "POST", "/user/forgot_password", map[string]string{
@@ -1123,9 +1133,9 @@ func TestUserPasswordReset(t *testing.T) {
 		"email": user2.Email,
 	})
 	session.MakeRequest(t, req, http.StatusOK)
-	assert.True(t, called)
+	assert.True(t, *called)
 
-	queryCode, err := url.QueryUnescape(code)
+	queryCode, err := url.QueryUnescape(*code)
 	require.NoError(t, err)
 
 	lookupKey, validator, ok := strings.Cut(queryCode, ":")
@@ -1141,7 +1151,7 @@ func TestUserPasswordReset(t *testing.T) {
 
 	req = NewRequestWithValues(t, "POST", "/user/recover_account", map[string]string{
 		"_csrf":    GetCSRF(t, session, "/user/recover_account"),
-		"code":     code,
+		"code":     *code,
 		"password": "new_password",
 	})
 	session.MakeRequest(t, req, http.StatusSeeOther)
@@ -1150,31 +1160,78 @@ func TestUserPasswordReset(t *testing.T) {
 	assert.True(t, unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}).ValidatePassword(t.Context(), "new_password"))
 }
 
+func TestUserPasswordResetOAuth2(t *testing.T) {
+	defer unittest.OverrideFixtures("tests/integration/fixtures/TestUserPasswordResetOAuth2")()
+	defer tests.PrepareTestEnv(t)()
+
+	t.Run("OAuth2 user without password", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1001})
+		assert.True(t, user.IsOAuth2())
+		assert.False(t, user.IsPasswordSet())
+		assert.False(t, user.IsLocal())
+
+		session := emptyTestSession(t)
+		req := NewRequestWithValues(t, "POST", "/user/forgot_password", map[string]string{
+			"_csrf": GetCSRF(t, session, "/user/forgot_password"),
+			"email": user.Email,
+		})
+		resp := session.MakeRequest(t, req, http.StatusOK)
+
+		htmlDoc := NewHTMLParser(t, resp.Body)
+		assert.Contains(t,
+			htmlDoc.doc.Find(".ui.negative.message").Text(),
+			translation.NewLocale("en-US").TrString("auth.non_local_account"),
+		)
+	})
+
+	t.Run("OAuth2 user with password", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1000})
+		assert.True(t, user.IsOAuth2())
+		assert.True(t, user.IsPasswordSet())
+		assert.False(t, user.IsLocal())
+
+		cleanup, code, called := parseMailHelper(t, user.EmailTo(), string(translation.NewLocale("en-US").Tr("mail.reset_password")))
+		defer cleanup()
+
+		session := emptyTestSession(t)
+		req := NewRequestWithValues(t, "POST", "/user/forgot_password", map[string]string{
+			"_csrf": GetCSRF(t, session, "/user/forgot_password"),
+			"email": user.Email,
+		})
+		session.MakeRequest(t, req, http.StatusOK)
+		assert.True(t, *called)
+
+		user.Passwd = ""
+		err := user_model.UpdateUserCols(db.DefaultContext, user, "passwd")
+		require.NoError(t, err)
+
+		req = NewRequestWithValues(t, "POST", "/user/recover_account", map[string]string{
+			"_csrf":    GetCSRF(t, session, "/user/recover_account"),
+			"code":     *code,
+			"password": "new_password",
+		})
+		resp := session.MakeRequest(t, req, http.StatusOK)
+
+		htmlDoc := NewHTMLParser(t, resp.Body)
+		assert.Contains(t,
+			htmlDoc.doc.Find(".ui.negative.message").Text(),
+			translation.NewLocale("en-US").TrString("auth.non_local_account"),
+		)
+	})
+}
+
 func TestActivateEmailAddress(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 	defer test.MockVariableValue(&setting.Service.RegisterEmailConfirm, true)()
 
 	user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
 
-	called := false
+	cleanup, code, called := parseMailHelper(t, "newemail@example.org", string(translation.NewLocale("en-US").Tr("mail.activate_email")))
-	code := ""
+	defer cleanup()
-	defer test.MockVariableValue(&mailer.SendAsync, func(msgs ...*mailer.Message) {
-		if called {
-			return
-		}
-		called = true
-
-		assert.Len(t, msgs, 1)
-		assert.Equal(t, "newemail@example.org", msgs[0].To)
-		assert.EqualValues(t, translation.NewLocale("en-US").Tr("mail.activate_email"), msgs[0].Subject)
-
-		messageDoc := NewHTMLParser(t, bytes.NewBuffer([]byte(msgs[0].Body)))
-		link, ok := messageDoc.Find("a").Attr("href")
-		assert.True(t, ok)
-		u, err := url.Parse(link)
-		require.NoError(t, err)
-		code = u.Query()["code"][0]
-	})()
 
 	session := loginUser(t, user2.Name)
 	req := NewRequestWithValues(t, "POST", "/user/settings/account/email", map[string]string{
@@ -1182,9 +1239,9 @@ func TestActivateEmailAddress(t *testing.T) {
 		"email": "newemail@example.org",
 	})
 	session.MakeRequest(t, req, http.StatusSeeOther)
-	assert.True(t, called)
+	assert.True(t, *called)
 
-	queryCode, err := url.QueryUnescape(code)
+	queryCode, err := url.QueryUnescape(*code)
 	require.NoError(t, err)
 
 	lookupKey, validator, ok := strings.Cut(queryCode, ":")
@@ -1199,7 +1256,7 @@ func TestActivateEmailAddress(t *testing.T) {
 	assert.Equal(t, authToken.HashedValidator, auth_model.HashValidator(rawValidator))
 
 	req = NewRequestWithValues(t, "POST", "/user/activate_email", map[string]string{
-		"code":  code,
+		"code":  *code,
 		"email": "newemail@example.org",
 	})
 	session.MakeRequest(t, req, http.StatusSeeOther)