Make captcha and password optional for external accounts #6606

coolaj86 · 2019-04-13T20:22:06Z

Replaces https://github.com/go-gitea/gitea/pull/5029/files

Make CAPTCHAs optional for External Accounts (i.e. Github, OpenID Connect, etc)
- Add REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA
Make Passwords optional for External Accounts (i.e. Github, OpenID Connect, etc)
- Add REQUIRE_EXTERNAL_REGISTRATION_PASSWORD
SECURITY:
- This increases security by allowing users to eliminating the weakest link (password) on an otherwise secure account.
- This makes it more convenient for users to make the secure choice (using a secure external account) than the insecure choice (waiting for an email from haveibeenpwned.com about their shared password being exploited)

This is part of the group of work for collective UX improvement and account security related to passwords and secondary accounts.

codecov-io · 2019-04-13T21:26:38Z

Codecov Report

Merging #6606 into master will decrease coverage by 0.02%.
The diff coverage is 3.22%.

@@            Coverage Diff             @@
##           master    #6606      +/-   ##
==========================================
- Coverage   41.24%   41.22%   -0.03%     
==========================================
  Files         467      467              
  Lines       63336    63360      +24     
==========================================
- Hits        26124    26121       -3     
- Misses      33796    33823      +27     
  Partials     3416     3416

Impacted Files	Coverage Δ
modules/auth/user_form.go	`42.85% <ø> (ø)`	⬆️
routers/user/auth_openid.go	`0% <0%> (ø)`	⬆️
routers/user/auth.go	`12.04% <0%> (-0.22%)`	⬇️
modules/setting/service.go	`79.16% <100%> (+0.9%)`	⬆️
modules/avatar/avatar.go	`48% <0%> (-6%)`	⬇️
modules/log/event.go	`64.61% <0%> (-1.03%)`	⬇️

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 337d691...477774b. Read the comment docs.

modules/setting/service.go

docs/content/doc/advanced/config-cheat-sheet.en-us.md

…AL_REGISTRATION_PASSWORD

…ccounts

agreed Co-Authored-By: solderjs <coolaj86@gmail.com>

coolaj86 · 2019-05-04T17:49:25Z

Updated and rebased.

kolaente · 2019-05-13T20:58:00Z

routers/user/auth.go

@@ -116,6 +118,7 @@ func SignIn(ctx *context.Context) {
 		return
 	}

+	ctx.Data["AllowPassword"] = true


Why is this always true? Shouldn't this depend on the setting?

It's the sign-up with password page. It doesn't make sense to get to that page unless it's true.

kolaente · 2019-05-13T20:59:52Z

routers/user/auth.go

 	ctx.Data["Title"] = ctx.Tr("link_account")
 	ctx.Data["LinkAccountMode"] = true
-	ctx.Data["EnableCaptcha"] = setting.Service.EnableCaptcha
+	ctx.Data["EnableCaptcha"] = setting.Service.RequireExternalRegistrationCaptcha


Please also make this dependend on setting.Service.EnableCaptcha (either setting.Service.EnableCaptcha should be true, or setting.Service.EnableCaptcha and setting.Service.RequireExternalRegistrationCaptcha)

kolaente · 2019-05-13T20:59:58Z

routers/user/auth.go

 	ctx.Data["Title"] = ctx.Tr("link_account")
 	ctx.Data["LinkAccountMode"] = true
 	ctx.Data["LinkAccountModeSignIn"] = true
-	ctx.Data["EnableCaptcha"] = setting.Service.EnableCaptcha
+	ctx.Data["EnableCaptcha"] = setting.Service.RequireExternalRegistrationCaptcha


kolaente · 2019-05-13T21:00:05Z

routers/user/auth.go

 	ctx.Data["Title"] = ctx.Tr("link_account")
 	ctx.Data["LinkAccountMode"] = true
 	ctx.Data["LinkAccountModeRegister"] = true
-	ctx.Data["EnableCaptcha"] = setting.Service.EnableCaptcha
+	ctx.Data["EnableCaptcha"] = setting.Service.RequireExternalRegistrationCaptcha


lafriks · 2019-07-01T07:42:26Z

@solderjs are you planing to continue work on this?

…password

zeripath · 2019-07-01T17:57:27Z

I think AllowPassword is probably not the correct name for this template data switch - perhaps ShowPasswordFormField?

Actually, flipping it around to DisablePassword would be cleaner.

zeripath · 2019-07-01T19:29:25Z

OK I've taken the liberty of updating and addressing @kolaente's concerns

routers/user/auth.go

zeripath · 2019-07-01T20:02:33Z

I've also removed the random password as an empty password is just as good.

lunny · 2019-07-02T02:19:13Z

@lafriks need your approval.

modules/auth/user_form.go

coolaj86 · 2019-07-04T18:07:10Z

Is there anything else that this needs presently? It's a holiday today so I have some free cycles.

coolaj86 · 2019-07-04T18:12:47Z

I've also removed the random password as an empty password is just as good.

This is not particularly true.

It is very likely that a programmer will make a mistake (whether already in the past, or in the future), in which a comparison is made between some API or Form input and the password, not checking to see if the password is blank.

It is very, very unlikely that the programmer will accidentally check the password against a pre-computed random string.

coolaj86 · 2019-07-04T18:17:11Z

I think AllowPassword is probably not the correct name for this template data switch - perhaps ShowPasswordFormField?

AllowPassword was chosen intentionally because passwords represent one of the greatest security vulnerabilities.

My commits have been aimed at providing authentication patterns that are both more secure and easier to use than the default password authentication. My hope is that one day passwords will be disabled by default and you will have to specifically opt-in to using a password with some sort of "warning: you're opening up your gitea instance and all of your users to pwnage". Hence AllowPassword as in "Allow this bad thing, which normally shouldn't be allowed".

That said, call it what you will.

zeripath · 2019-07-04T20:29:26Z

Hi @solderjs I understand what you're saying about comparing empty passwords to empty strings - but we've already had to deal with this problem and have already fixed it. So whilst your concern would be correct - we've got the fix in.

Now because of our logic - if the password is empty we can assume that the password is not set. Thus we can migrate to remove the local password in future easily - because an empty password is an unset one. Yes, I agree this is not ideal and I would prefer to do this properly - however we cannot do that at this point in 1.9. If you stick a random password in - I won't be able to do that migration in future.

Removing the special status of the local db password is on my personal list of things to do, but my list is long. The technical debt of Gogs is very large IMHO.

Now, regarding AllowPassword marker in the templates. It just didn't make sense to me when reading the templates. I completely understand what you're aiming for, but we have to make sure that reading the templates can be done without that global understanding - TBH DisablePassword isn't perfect in that regard either.

I do appreciate what you're aiming for though.

coolaj86 · 2019-07-04T20:52:10Z

👍

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 13, 2019

lafriks added the type/enhancement An improvement of existing functionality label Apr 14, 2019

lafriks added this to the 1.9.0 milestone Apr 14, 2019

lafriks requested changes Apr 18, 2019

View reviewed changes

modules/setting/service.go Outdated Show resolved Hide resolved

lafriks reviewed Apr 23, 2019

View reviewed changes

docs/content/doc/advanced/config-cheat-sheet.en-us.md Show resolved Hide resolved

AJ ONeal and others added 6 commits May 4, 2019 11:41

Make captcha and password optional for external accounts

6a5649d

add docs for REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA and REQUIRE_EXTERN…

d2e0def

…AL_REGISTRATION_PASSWORD

whitespace fix

900e825

swap EnableCaptcha for RequireExternalRegistrationCaptcha for LinkedA…

ac5e5ca

…ccounts

Update modules/setting/service.go

fba60a7

agreed Co-Authored-By: solderjs <coolaj86@gmail.com>

note dependency on ENABLE_CAPTCHA

fca8843

coolaj86 force-pushed the ux-optional-external-cpatcha-and-password branch from 046312a to fca8843 Compare May 4, 2019 17:46

kolaente reviewed May 13, 2019

View reviewed changes

lunny mentioned this pull request Jul 1, 2019

UX Optional Password & Captcha for External Registration #5029

Closed

6 tasks

Merge branch 'master' into solderjs-ux-optional-external-cpatcha-and-…

dece3e1

…password

Address @kolaente issues

5394e9b

techknowlogick approved these changes Jul 1, 2019

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jul 1, 2019

techknowlogick requested a review from lafriks July 1, 2019 19:30

zeripath reviewed Jul 1, 2019

View reviewed changes

routers/user/auth.go Show resolved Hide resolved

Just set password to empty to disable login

00e8d8b

zeripath and others added 2 commits July 1, 2019 21:02

Merge branch 'master' into ux-optional-external-cpatcha-and-password

0caf608

Merge branch 'master' into ux-optional-external-cpatcha-and-password

b9ee6d1

lunny approved these changes Jul 2, 2019

View reviewed changes

GiteaBot 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 Jul 2, 2019

Merge branch 'master' into ux-optional-external-cpatcha-and-password

c76149a

zeripath reviewed Jul 2, 2019

View reviewed changes

modules/auth/user_form.go Show resolved Hide resolved

Merge branch 'master' into ux-optional-external-cpatcha-and-password

dbac60a

lafriks approved these changes Jul 5, 2019

View reviewed changes

lafriks and others added 3 commits July 5, 2019 12:02

Merge branch 'master' into ux-optional-external-cpatcha-and-password

85eadce

Merge branch 'master' into ux-optional-external-cpatcha-and-password

dfe269c

Merge branch 'master' into ux-optional-external-cpatcha-and-password

477774b

techknowlogick merged commit 62d6127 into go-gitea:master Jul 6, 2019

jeffliu27 pushed a commit to jeffliu27/gitea that referenced this pull request Jul 18, 2019

Make captcha and password optional for external accounts (go-gitea#6606)

a6c9781

blueworrybear mentioned this pull request Nov 24, 2019

Can't link Github/Gitlab to existing account #9066

Closed

7 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make captcha and password optional for external accounts #6606

Make captcha and password optional for external accounts #6606

coolaj86 commented Apr 13, 2019 •

edited

Loading

codecov-io commented Apr 13, 2019 •

edited

Loading

coolaj86 commented May 4, 2019

kolaente May 13, 2019

coolaj86 May 13, 2019

kolaente May 13, 2019

coolaj86 May 13, 2019 •

edited

Loading

kolaente May 13, 2019

kolaente May 13, 2019

lafriks commented Jul 1, 2019

zeripath commented Jul 1, 2019 •

edited

Loading

zeripath commented Jul 1, 2019

zeripath commented Jul 1, 2019

lunny commented Jul 2, 2019

coolaj86 commented Jul 4, 2019

coolaj86 commented Jul 4, 2019 •

edited

Loading

coolaj86 commented Jul 4, 2019

zeripath commented Jul 4, 2019

coolaj86 commented Jul 4, 2019

Make captcha and password optional for external accounts #6606

Make captcha and password optional for external accounts #6606

Conversation

coolaj86 commented Apr 13, 2019 • edited Loading

codecov-io commented Apr 13, 2019 • edited Loading

Codecov Report

coolaj86 commented May 4, 2019

kolaente May 13, 2019

Choose a reason for hiding this comment

coolaj86 May 13, 2019

Choose a reason for hiding this comment

kolaente May 13, 2019

Choose a reason for hiding this comment

coolaj86 May 13, 2019 • edited Loading

Choose a reason for hiding this comment

kolaente May 13, 2019

Choose a reason for hiding this comment

kolaente May 13, 2019

Choose a reason for hiding this comment

lafriks commented Jul 1, 2019

zeripath commented Jul 1, 2019 • edited Loading

zeripath commented Jul 1, 2019

zeripath commented Jul 1, 2019

lunny commented Jul 2, 2019

coolaj86 commented Jul 4, 2019

coolaj86 commented Jul 4, 2019 • edited Loading

coolaj86 commented Jul 4, 2019

zeripath commented Jul 4, 2019

coolaj86 commented Jul 4, 2019

coolaj86 commented Apr 13, 2019 •

edited

Loading

codecov-io commented Apr 13, 2019 •

edited

Loading

coolaj86 May 13, 2019 •

edited

Loading

zeripath commented Jul 1, 2019 •

edited

Loading

coolaj86 commented Jul 4, 2019 •

edited

Loading