Force user to change password #4489

adelowo · 2018-07-20T22:36:24Z

This fixes #4340

The Toggle middleware was updated to check for users whose accounts was created by an admin.. It restricts access to operations by redirecting to a new route /user/settings/change_password .. Once the password is updated, normal operations can now be performed by the user

techknowlogick · 2018-07-20T22:40:32Z

models/migrations/v70.go

@@ -9,7 +9,6 @@ import (
 	"time"

 	"code.gitea.io/gitea/modules/setting"
-


This line should remain

techknowlogick · 2018-07-20T22:41:24Z

routers/user/auth.go

@@ -1,5 +1,4 @@
 // Copyright 2014 The Gogs Authors. All rights reserved.
-// Copyright 2018 The Gitea Authors. All rights reserved.


Why remove copyright?

Oops, my bad... Must be when I was resolving conflicts

Fixed @techknowlogick

codecov-io · 2018-07-20T22:44:04Z

Codecov Report

Merging #4489 into master will decrease coverage by <.01%.
The diff coverage is 10.75%.

@@            Coverage Diff             @@
##           master    #4489      +/-   ##
==========================================
- Coverage   37.21%   37.21%   -0.01%     
==========================================
  Files         305      306       +1     
  Lines       45043    45125      +82     
==========================================
+ Hits        16763    16793      +30     
- Misses      25842    25887      +45     
- Partials     2438     2445       +7

Impacted Files	Coverage Δ
models/migrations/migrations.go	`2.61% <ø> (ø)`	⬆️
models/user.go	`44.13% <ø> (ø)`	⬆️
models/migrations/v73.go	`0% <0%> (ø)`
routers/user/auth.go	`13.61% <0%> (-0.82%)`	⬇️
modules/auth/user_form.go	`20% <0%> (-1.43%)`	⬇️
modules/context/auth.go	`21.87% <10.52%> (-2.13%)`	⬇️
routers/admin/users.go	`31.18% <100%> (+12.78%)`	⬆️
routers/routes/routes.go	`86.2% <100%> (+0.04%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a5cc3a9...16b5b61. Read the comment docs.

techknowlogick · 2018-07-20T23:49:01Z

models/migrations/v71.go

+
+import (
+	"code.gitea.io/gitea/models"
+	"github.com/go-xorm/xorm"


same new line as v70 should also go between these two imports.

lafriks

User should be allowed to access this password change form only when he has must change password set, otherwise if opening this url manually it will be possible to change password without knowing current password

lafriks · 2018-07-21T07:18:16Z

modules/context/auth.go

+			}
+
+			// Prevent infinite redirection loop
+			if ctx.Req.URL.Path == "/user/change_password" {


This will not work if appsuburl is used

@lafriks I don't quite get this ... By the way, I fixed the "main" review with 586c080

@adelowo if gitea is hosted as https://example.com/gitea/ as root url than url path would be /gitea/user/change_password

jonasfranz · 2018-07-21T10:34:58Z

models/migrations/v71.go

+func addMustChangePassword(x *xorm.Engine) error {
+	type User struct {
+		ID                 int64                `xorm:"pk autoincr"`
+		MustChangePassword models.PasswordState `xorm:"NOT NULL DEFAULT 0"`


Do not reference models from migrations

jonasfranz · 2018-07-21T10:35:43Z

models/user.go

@@ -40,6 +40,23 @@ import (
 	"code.gitea.io/gitea/modules/util"
 )

+// PasswordState defines the action for a user to take on his password
+// To change it or not ?
+type PasswordState uint8


Why do we need a separate type for this? Couldn't we use just a bool?

Changed it to a bool

lafriks · 2018-07-21T15:03:54Z

models/migrations/v71.go

+func addMustChangePassword(x *xorm.Engine) error {
+	type User struct {
+		ID                 int64 `xorm:"pk autoincr"`
+		MustChangePassword bool  `xorm:"NOT NULL DEFAULT 0"`


Should be NOT NULL DEFAULT false

Not sure why this review wasn't collapsed but it has been fixed in https://github.com/go-gitea/gitea/pull/4489/files#diff-c6d02501c53821722868adbdd6d59a34R14

lafriks · 2018-07-21T15:05:06Z

models/user.go

+	// MustChangePassword is an attribute that determines if a user
+	// is to change his/her password after registration.
+	// PS : This scenario only plays out if an admin created the account
+	MustChangePassword bool `xorm:"INDEX"`


Why INDEX this column?
Should be same as in migration xorm:"NOT NULL DEFAULT false"

Not familiar with xorm and it broke earlier ( https://drone.gitea.io/go-gitea/gitea/2097 )... Would fix

lafriks · 2018-07-21T15:06:45Z

models/user.go

+
+	// MustChangePassword is an attribute that determines if a user
+	// is to change his/her password after registration.
+	// PS : This scenario only plays out if an admin created the account


NIT: No need for last comment line

lafriks · 2018-07-21T15:09:37Z

modules/context/auth.go

+			}
+
+			// Prevent infinite redirection loop
+			if ctx.Req.URL.Path == "/user/change_password" {


@adelowo if gitea is hosted as https://example.com/gitea/ as root url than url path would be /gitea/user/change_password

adelowo · 2018-07-21T15:56:48Z

~~Oops... Looks like I screwed this up while trying to rebase... Forgotten I had merged master into this already.~~

Fixed

…_fix

adelowo · 2018-07-21T16:19:32Z

modules/context/auth.go

+				return
+			}
+
+			if ctx.Req.URL.Path == "/user/change_password" {


@lafriks Would it just be safe to use strings.HasSuffix ?

Or what is the potential fix here ? other than giving the url path it's own middleware definition so we can safely handle the scenario without the infinite redirection

Just use setting.AppSubURL

strings.HasSuffix(setting.AppSubURL + ctx.Req.URL.Path, "/user/change_password") ???

Just ctx.Req.URL.Path == setting.AppSubURL + "/user/change_password"

lafriks · 2018-07-21T16:57:50Z

models/user.go

@@ -14,6 +14,7 @@ import (
 	"errors"
 	"fmt"
 	"image"
+


No need for newline here

lafriks · 2018-07-21T17:03:47Z

modules/context/auth.go

+				return
+			}
+
+			if ctx.Req.URL.Path == "/user/change_password" {


Just use setting.AppSubURL

… users can't view the page

lafriks

Also I think when user uses forgot password functionality and changes his password using it than MustChangePassword should also be set to false after this line:
https://github.com/go-gitea/gitea/blob/master/routers/user/auth.go#L1174

Otherwise LG-TM

lafriks · 2018-07-21T21:47:38Z

routers/user/auth.go

+	}
+
+	var err error
+	if u.Rands, err = models.GetUserSalt(); err != nil {


I don't think this should be updated when changing password, only if resetting password.

User should be allowed to access this password change form only when he has must change password set, otherwise if opening this url manually it will be possible to change password without knowing current password

By the way, the above review has been implemented

lafriks

In future would be nice to see this improved to have admin ability to specify if user must change password or not. Also when adding user for gitea cli or api to have this option set. But that can be done as other PR also. Good work, thumbs up from me

lafriks · 2018-07-21T22:13:59Z

routers/user/auth.go

@@ -28,6 +28,7 @@ import (
 )

 const (
+	tplMustChangePassword = "user/auth/change_passwd"


nit: comment would be recommended here like it is for other constants below

lunny · 2018-08-01T10:00:37Z

routers/admin/main_test.go

@@ -0,0 +1,16 @@
+// Copyright 2017 The Gitea Authors. All rights reserved.


Oh snap... fixed

Signed-off-by: Lanre Adelowo <[email protected]>

…_user_to_change_password

lafriks · 2018-08-06T11:41:40Z

@JonasFranzDEV need your approval

techknowlogick · 2018-09-05T02:50:53Z

@JonasFranzDEV need your approval. If you don't have time to review could you remove your status?

No response for more than month to rereview

routers/admin/users.go

Prepare for wild/v1.6 branch * BREAKING * Respect email privacy option in user search via API (go-gitea#4512) * Simply remove tidb and deps (go-gitea#3993) * Swagger.v1.json template (go-gitea#3572) * FEATURE * Pull request review/approval and comment on code (go-gitea#3748) * Added dependencies for issues (go-gitea#2196) (go-gitea#2531) * Add the ability to have built in themes in Gitea and provide dark theme arc-green (go-gitea#4198) * Add sudo functionality to the API (go-gitea#4809) * Add oauth providers via cli (go-gitea#4591) * Disable merging a WIP Pull request (go-gitea#4529) * Force user to change password (go-gitea#4489) * Add letsencrypt to Gitea (go-gitea#4189) * Add push webhook support for mirrored repositories (go-gitea#4127) * Add csv file render support defaultly (go-gitea#4105) * Add Recaptcha functionality to Gitea (go-gitea#4044) * BUGFIXES * Fix release creation via API (go-gitea#5076) * Remove links from topics in edit mode (go-gitea#5026) * Fix missing AppSubUrl in few more templates (fixup) (go-gitea#5021) * Fix missing AppSubUrl in some templates (go-gitea#5020) * Hide outdated comments in file view (go-gitea#5017) * Upgrade gopkg.in/testfixtures.v2 (go-gitea#4999) * Disable debug routes unless PPROF is enabled in configuration (go-gitea#4995) * Fix user menu item styling (go-gitea#4985) * Fix layout of the topics editing form (go-gitea#4971) * Fix null pointer dereference in ParseCommitWithSignature (go-gitea#4962) * Fix url in discord webhook (go-gitea#4953) * Detect charset and convert non UTF-8 files for display (go-gitea#4950) * Make sure to catch the right error so it is displayed on the UI (go-gitea#4945) * Fix(topics): don't redirect to explore page. (go-gitea#4938) * Fix bug forget to remove Stopwatch when remove repository (go-gitea#4928) * Fix bug when repo remained bare if multiple branches pushed in single push (go-gitea#4923) * Fix: Let's Encrypt configuration settings (go-gitea#4911) * Fix: Crippled diff (go-gitea#4726) (go-gitea#4900) * Fix trimming of markup section names (go-gitea#4863) * Issues api allow pulls and fix go-gitea#4832 (go-gitea#4852) * Do not autocreate directory for new users/orgs (go-gitea#4828) (go-gitea#4849) * Fix redirect with non-ascii branch names (go-gitea#4764) (go-gitea#4810) * Fix missing release title in webhook (go-gitea#4783) (go-gitea#4796) * User shouldn't be able to approve or reject his/her own PR (go-gitea#4729) * Make sure to reset commit count in the cache on mirror syncing (go-gitea#4720) * Fixed bug where team with admin privelege type doesn't get any unit (go-gitea#4719) * Fix incorrect caption of webhook setting (go-gitea#4701) (go-gitea#4717) * Allow WIP marker to contains < or > (go-gitea#4709) * Hide org/create menu item in Dashboard if user has no rights (go-gitea#4678) (go-gitea#4680) * Site admin could create repos even MAX_CREATION_LIMIT=0 (go-gitea#4645) * Fix custom templates being ignored (go-gitea#4638) * Fix starring icon after semantic ui update (go-gitea#4628) * Fix Split-View line adjustment (go-gitea#4622) * Fix integer constant overflows in tests (go-gitea#4616) * Push whitelist now doesn't apply to branch deletion (go-gitea#4601) (go-gitea#4607) * Fix bugs when too many IN variables (go-gitea#4594) * Fix failure on creating pull request with assignees (go-gitea#4419) (go-gitea#4583) * Fix panic issue on update avatar email (go-gitea#4580) (go-gitea#4581) * Fix status code label for a successful webhook (go-gitea#4540) * An inactive user shouldn't be able to be added as a collaborator (go-gitea#4535) * Don't fail silently if trying to add a collaborator twice (go-gitea#4533) * Fix incorrect MergeWhitelistTeamIDs check in CanUserMerge function (go-gitea#4519) (go-gitea#4525) * Fix out-of-transaction query in removeOrgUser (go-gitea#4521) (go-gitea#4522) * Fix migration from older releases (go-gitea#4495) * Accept 'Data:' in commit graph (go-gitea#4487) * Update xorm to latest version and fix correct `user` table referencing in sql (go-gitea#4473) * Relative URLs for LibreJS page (go-gitea#4460) * Redirect to correct page after using scratch token (go-gitea#4458) * Fix column droping for MSSQL that need new transaction for that (go-gitea#4440) * Replace src with raw to fix image paths (go-gitea#4377) * Add default merge options when creating new repository (go-gitea#4369) * Fix docker build (go-gitea#4358) * Fixes repo membership check in API (go-gitea#4341) * Dep upgrade mysql lib (go-gitea#4161) * Fix some issues with special chars in branch names (go-gitea#3767) * Responsive design fixes (go-gitea#4508) * ENHANCEMENT * Fix milestones sorted wrongly (go-gitea#4987) * Allow api to create tags for releases if they don't exist (go-gitea#4890) * Fix go-gitea#4877 to follow the OpenID Connect Audiences spec (go-gitea#4878) * Enforce token on api routes [fixed critical security issue go-gitea#4357] (go-gitea#4840) * Update legacy branch and tag URLs in dashboard to new format (go-gitea#4812) * Slack webhook channel name cannot be empty or just contain an hashtag (go-gitea#4786) * Add whitespace handling to PR-comparsion (go-gitea#4683) * Make reverse proxy auth optional (go-gitea#4643) * MySQL TLS (go-gitea#4642) * Make sure to set PR split view when creating/previewing a pull request (go-gitea#4617) * Log user in after a successful sign up (go-gitea#4615) * Fix typo IsPullReuqestBroken -> IsPullRequestBroken (go-gitea#4578) * Allow admin toggle forcing a password change for newly created users (go-gitea#4563) * Update jQuery to v1.12.4 (go-gitea#4551) * Env var GITEA_PUSHER_EMAIL (go-gitea#4516) * Feat(repo): support search repository by topic name (go-gitea#4505) * Small improvements to dependency UI (go-gitea#4503) * Make max commits in graph configurable (go-gitea#4498) * Add valid for lfs oid (go-gitea#4461) * Add shortcut to save wiki page (go-gitea#4452) * Allow administrator to create repository for any organization (go-gitea#4368) * Fix repository last updated time update when delete a user who watched the repo (go-gitea#4363) * Switch plaintext scratch tokens to use hash instead (go-gitea#4331) * Increase default TOTP secret size to 320 bits (go-gitea#4287) * Keep preseeded database password (go-gitea#4284) * Implemented hover text showing user FullName (go-gitea#4261) * Add ability to delete a token (go-gitea#4235) * Fix typos in i18n variable names. (go-gitea#4080) * Api: repos/search: add parameters to control the sort order (go-gitea#3964) * Add missing path in the Docker app.ini template (go-gitea#2181) * Add file name and branch to page title (go-gitea#4902) * Offline use of google fonts (go-gitea#4872) * Add missing History link to directory listings v2 (go-gitea#4829) * Locale for Edit and Remove due date issue (go-gitea#4802) * Disable 'May Import Local Repository' when is disabled by setting (Is… (go-gitea#4780) * API /admin/users/{username} missing parameter (go-gitea#4775) * Display error when adding a user to a team twice (go-gitea#4746) * Remove UsePrivilegeSeparation from the Docker sshd_config, see go-gitea#2876 (go-gitea#4722) * Focus title input when clicking helper link (go-gitea#4696) * Add vendor to user reserved words and format words list according alphabet (go-gitea#4685) * Add gitea/issues link to 500 page (go-gitea#4654) * Hide home button when landing page is not set to home (go-gitea#4651) * Remove link to GitHub issues in 404 template (go-gitea#4639) * Cmd/serve: pprof cpu and memory profile dumps to disk (go-gitea#4560) * Add flash message after an account has been successfully activated (go-gitea#4510) * Prevent html entity escaping on delete branch (go-gitea#4471) * Locale for button Edit on protected branch (go-gitea#4442) * Update notification icon (go-gitea#4343) * Added front-end topics validation (go-gitea#4316) * Don't display buttons if there are no system notifications (go-gitea#4280) * Issue due date api (go-gitea#3890) * SECURITY * Improve URL validation for external wiki and external issues (go-gitea#4710) * Make cookies HttpOnly and obey COOKIE_SECURE flag (go-gitea#4706) * Don't disclose emails of all users when sending out emails (go-gitea#4664) * Check that repositories can only be migrated to own user or organizations (go-gitea#4366) * TRANSLATION * Fix punctuation in English translation (go-gitea#4958) * Fix translation (go-gitea#4355)

redirect to login page after successfully activating account

0bef325

techknowlogick reviewed Jul 20, 2018

View reviewed changes

bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 20, 2018

techknowlogick reviewed Jul 20, 2018

View reviewed changes

lafriks requested changes Jul 21, 2018

View reviewed changes

lafriks added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/changelog Adds the changelog for a new Gitea version labels Jul 21, 2018

lafriks added this to the 1.6.0 milestone Jul 21, 2018

jonasfranz previously requested changes Jul 21, 2018

View reviewed changes

lafriks requested changes Jul 21, 2018

View reviewed changes

adelowo force-pushed the force_user_to_change_password branch from bfda6f9 to bb89613 Compare July 21, 2018 15:55

adelowo added 3 commits July 21, 2018 16:59

Merge remote-tracking branch 'origin' into force_user_password_change…

68d57a1

…_fix

force users to change password if account was created by an admin

23acb29

force users to change password if account was created by an admin

ff42bfd

adelowo force-pushed the force_user_to_change_password branch from bb89613 to ff42bfd Compare July 21, 2018 16:11

adelowo added 2 commits July 21, 2018 17:15

fixed build

26fccdf

fixed build

4562460

adelowo commented Jul 21, 2018

View reviewed changes

fix pending issues with translation and wrong routes

5a2ea86

lafriks requested changes Jul 21, 2018

View reviewed changes

adelowo added 4 commits July 21, 2018 20:26

make sure path check is safe

3b87fef

remove unneccessary newline

7e67ecc

make sure users that don't have to view the form get redirected

59432fa

move route to use /settings prefix so as to make sure unauthenticated…

741ef66

… users can't view the page

lafriks requested changes Jul 21, 2018

View reviewed changes

update as per @lafriks review

9b4f70f

lafriks approved these changes Jul 21, 2018

View reviewed changes

lunny reviewed Aug 1, 2018

View reviewed changes

adelowo added 3 commits August 1, 2018 11:49

fix copyright year

0181ebf

Signed-off-by: Lanre Adelowo <[email protected]>

Merge branch 'master' into force_user_to_change_password

d083410

Merge branch 'master' of https://github.com/go-gitea/gitea into force…

d3febca

…_user_to_change_password

adelowo added 9 commits August 10, 2018 22:56

Merge branch 'master' into force_user_to_change_password

dafa9c8

Merge branch 'master' into force_user_to_change_password

8c6e9c6

Merge branch 'master' into force_user_to_change_password

454cd3a

Merge branch 'master' into force_user_to_change_password

d7ee5f8

Merge branch 'master' into force_user_to_change_password

5441645

Merge branch 'master' into force_user_to_change_password

98933c6

Merge branch 'master' into force_user_to_change_password

d2ddc69

Merge branch 'master' into force_user_to_change_password

4c6658a

Merge branch 'master' into force_user_to_change_password

e2ef29b

techknowlogick and others added 2 commits September 4, 2018 22:50

Merge branch 'master' into force_user_to_change_password

16b5b61

Merge branch 'master' into force_user_to_change_password

a4152f2

lunny requested changes Sep 13, 2018

View reviewed changes

routers/admin/users.go Show resolved Hide resolved

lunny approved these changes Sep 13, 2018

View reviewed changes

bkcsoft added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 13, 2018

Merge branch 'master' into force_user_to_change_password

a03c9a3

lafriks merged commit 126ba79 into go-gitea:master Sep 13, 2018

adelowo deleted the force_user_to_change_password branch September 13, 2018 12:09

SagePtr mentioned this pull request Sep 13, 2018

Proposal: gitea admin create-user random password #4931

Closed

adelowo mentioned this pull request Sep 18, 2018

Add must-change-password flag to cli for creating a user #4955

Merged

go-gitea locked and limited conversation to collaborators Nov 24, 2020

delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023

		@@ -9,7 +9,6 @@ import (
		"time"

		"code.gitea.io/gitea/modules/setting"

		@@ -1,5 +1,4 @@
		// Copyright 2014 The Gogs Authors. All rights reserved.
		// Copyright 2018 The Gitea Authors. All rights reserved.

		@@ -0,0 +1,16 @@
		// Copyright 2017 The Gitea Authors. All rights reserved.

Force user to change password #4489

Force user to change password #4489

Conversation

adelowo commented Jul 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 20, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

lafriks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelowo Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adelowo commented Jul 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks commented Aug 6, 2018

techknowlogick commented Sep 5, 2018

adelowo commented Jul 20, 2018 •

edited

Loading

codecov-io commented Jul 20, 2018 •

edited

Loading

adelowo Jul 21, 2018 •

edited

Loading

adelowo commented Jul 21, 2018 •

edited

Loading