Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make captcha and password optional for external accounts #6606

Conversation

@solderjs
Copy link
Contributor

commented Apr 13, 2019

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

This comment has been minimized.

Copy link

commented Apr 13, 2019

Codecov Report

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

Impacted file tree graph

@@            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.

@lafriks lafriks added this to the 1.9.0 milestone Apr 14, 2019

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

@solderjs solderjs force-pushed the solderjs:ux-optional-external-cpatcha-and-password branch from 046312a to fca8843 May 4, 2019

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented May 4, 2019

Updated and rebased.

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

ctx.Data["AllowPassword"] = true

This comment has been minimized.

Copy link
@kolaente

kolaente May 13, 2019

Member

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

This comment has been minimized.

Copy link
@solderjs

solderjs May 13, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
@kolaente

kolaente May 13, 2019

Member

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

This comment has been minimized.

Copy link
@solderjs

solderjs May 13, 2019

Author Contributor

Okay.

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

This comment has been minimized.

Copy link
@kolaente

kolaente May 13, 2019

Member

as above

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

This comment has been minimized.

Copy link
@kolaente

kolaente May 13, 2019

Member

as above

@lunny lunny referenced this pull request Jul 1, 2019

Closed

UX Optional Password & Captcha for External Registration #5029

6 of 6 tasks complete
@lafriks

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

@solderjs are you planing to continue work on this?

@zeripath

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

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

@GiteaBot GiteaBot added lgtm/need 1 and removed lgtm/need 2 labels Jul 1, 2019

@techknowlogick techknowlogick requested a review from lafriks Jul 1, 2019

@zeripath

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2019

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

zeripath and others added some commits Jul 1, 2019

@lunny

lunny approved these changes Jul 2, 2019

@GiteaBot GiteaBot added lgtm/done and removed lgtm/need 1 labels Jul 2, 2019

@lunny

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

@lafriks need your approval.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

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

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

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.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 4, 2019

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.

@solderjs

This comment has been minimized.

Copy link
Contributor Author

commented Jul 4, 2019

👍

@lafriks

lafriks approved these changes Jul 5, 2019

lafriks and others added some commits Jul 5, 2019

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

1 check was pending

continuous-integration/drone/pr Build is running
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.