UX Optional Password & Captcha for External Registration #5029

coolaj86 · 2018-10-06T21:55:16Z

I see the primary purposes of external accounts as to increase security and convenience (which always go hand-in-hand).

If I have enabled external accounts, I should not require the user to weaken security by forcing them to create a (likely re-used) password.

Likewise, since I've already hand-picked the external account providers that I trust and want to allow, I should not reduce convenience by requiring a secondary captcha.

Do not require password for external accounts via REQUIRE_EXTERNAL_REGISTRATION_PASSWORD
Do not require password for external accounts via REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA
Only reject omitted passwords on forms that use a password
Do not provide the option for a password when ALLOW_ONLY_EXTERNAL_REGISTRATION

Some other fixes that I want to get in, but may deserve separate PRs:

Do not require logging in twice when creating account (PR UX treat register like sign in, even oauth #5033)
Allow resetting (creating) password via email, even when logged in (PR allow current user to reset their own password #5034, Issue Password reset link does not work when logged in #5008)

If you'd like to test it out

My pull request is against master, but I run it as backport to v1.5.1 (includes #5006, #5029, #5033, #5034):

git clone https://github.com/coolaj86/gitea.git gitea.coolaj86 -b v1.5.1-coolaj86
pushd gitea.coolaj86
TAGS="bindata sqlite" make generate all

I would not recommend replacing your existing gitea, but rather creating a symlink so that you can easily switch back if you don't like it. For example, if you keep gitea in /opt/gitea/bin:

rsync -av ./gitea /opt/gitea/bin/gitea-v1.5.1-coolaj86
pushd /opt/gitea/bin
mv gitea gitea-v1.5.1
ln -s gitea-v1.5.1-coolaj86 gitea

I've run a couple of manual tests so far, so I feel comfortable with someone else trying it out. I won't be pushing any additional changes to that branch (such as the upcoming changes to address the empty checkboxes in the issue) until I've tested them in production for myself.

codecov-io · 2018-10-06T22:08:03Z

Codecov Report

Merging #5029 into master will increase coverage by <.01%.
The diff coverage is 19.51%.

@@            Coverage Diff             @@
##           master    #5029      +/-   ##
==========================================
+ Coverage   41.24%   41.24%   +<.01%     
==========================================
  Files         464      464              
  Lines       62867    62893      +26     
==========================================
+ Hits        25928    25939      +11     
- Misses      33548    33561      +13     
- Partials     3391     3393       +2

Impacted Files	Coverage Δ
modules/auth/user_form.go	`42.85% <ø> (ø)`	⬆️
modules/setting/service.go	`79.16% <100%> (+0.9%)`	⬆️
routers/user/auth.go	`12.53% <15.38%> (+0.27%)`	⬆️
models/gpg_key.go	`55.83% <0%> (-0.84%)`	⬇️
routers/repo/view.go	`43.25% <0%> (+1.01%)`	⬆️
models/unit.go	`67.56% <0%> (+5.4%)`	⬆️

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 855ebbd...dd63dfc. Read the comment docs.

lafriks · 2018-10-08T05:16:57Z

What is reason REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA why no just reuse captcha setting that is already present?

coolaj86 · 2018-10-08T05:20:21Z

My view is that Captcha is the kind of thing you implement for local logins, not the kind of thing you enforce on delegated logins that you already trust, which already do that type of validation.

For my instance (and I think most people would feel this way), I want the greatest protection from bots with the least friction for new users and I think local-only captcha strikes the right balance.

coolaj86 · 2018-10-08T05:22:03Z

(that said, some people may want it for some reason, so I didn’t want to nix it entirely)

strk · 2018-10-09T18:02:33Z

Is "OpenID" considered "External Registration" ? Sorry but I'm a bit confused about the state of oAuth2 vs. OpenID(2) implementations

coolaj86 · 2018-10-09T18:52:00Z

TL;DR

OpenID(1): No change introduced by this PR
OpenID(2)/OIDC: Same changes as OAuth2

Explanation

I haven't changed the behavior of OpenID(1) and it follows a different flow from OAuth2 in the code.

As such I don't think that these changes affect the OpenID(1) flow. I don't want to pursue changing the OpenID(1) flow as part of this PR (it was never very well adopted and doesn't provide a lot of value in my opinion), but I'd be open to updating it as well later on.

OpenID(2) aka OIDC aka OpenID Connect is actually not related to OpenID in any way. It's a strict subset of OAuth2 that actually defines important things like data format (JSON vs urlencoded), endpoint URLs, etc. It's not very well designed for personal use, in my opinion, so it doesn't really meet the original goal of OpenID (IndieAuth is probably better for that), but it is far better than OAuth2 for enterprise-style use and maintains perfect OAuth2 compatibility (which isn't hard seeing how OAuth2 doesn't define any of those things so there's nothing much to be "compatible" with other than the generic concept of "there's some handshakes").

stale · 2019-02-17T13:02:19Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

nickho · 2019-02-21T18:18:40Z

Thanks for working on this @solderjs !
I tested your PR on a setup with :

local VM on Ubuntu and gitea on https
our corporate AzureAD as an OIDC provider

The workflow is working fine for me. Clicking the link "Connect With OpenId Connect" on the login page redirect to our AzureAD that redirect to the "Add Account Recovery Info". If i enter a username, then everything is working fine inside Gitea.

I do confirm ALLOW_ONLY_EXTERNAL_REGISTRATION is working, but you need to also to put DISABLE_REGISTRATION to false.

I think something is missaligned on the UI but is it an issue with #5006 ?

Keep up the good work.

stale · 2019-04-22T18:18:46Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

strk · 2019-05-29T20:04:39Z

Is this work still ongoing ? I'd love to see OpenID-Connect with discovery enabled on try.gitea.io ...

lunny · 2019-06-26T11:36:42Z

Please resolve the conflict

zeripath · 2019-06-28T21:11:55Z

@lunny @solderjs conflict resolved.

techknowlogick · 2019-06-29T00:31:17Z

modules/setting/service.go

@@ -61,6 +63,8 @@ func newService() {
 Service.EnableReverseProxyAutoRegister = sec.Key("ENABLE_REVERSE_PROXY_AUTO_REGISTRATION").MustBool()
 Service.EnableReverseProxyEmail = sec.Key("ENABLE_REVERSE_PROXY_EMAIL").MustBool()
 Service.EnableCaptcha = sec.Key("ENABLE_CAPTCHA").MustBool(false)
+ Service.RequireExternalRegistrationCaptcha = sec.Key("REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA").MustBool()


Suggested change

Service.RequireExternalRegistrationCaptcha = sec.Key("REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA").MustBool()

Service.RequireExternalRegistrationCaptcha = sec.Key("REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA").MustBool(Service.EnableCaptcha)

lunny · 2019-07-01T02:37:12Z

replaced by #6606

make password and captcha optional for external registration

64971f8

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

This was referenced Oct 6, 2018

ALLOW_ONLY_EXTERNAL_REGISTRATION Appears to have no effect #4226

Closed

Password reset link does not work when logged in #5008

Closed

Add support for federated OpenID-Connect logins #1299

Open

coolaj86 changed the title ~~[draft] Optional Password & Captcha for External Registration~~ Optional Password & Captcha for External Registration Oct 6, 2018

coolaj86 changed the title ~~Optional Password & Captcha for External Registration~~ UX Optional Password & Captcha for External Registration Oct 7, 2018

This was referenced Oct 7, 2018

UX of link account (Step 1) #5006

Merged

UX treat register like sign in, even oauth #5033

Closed

Simpler UX for OAuth2 login with GitHub #3837

Closed

Always put OAuth user info in the ExternalLoginUser table #1143

Closed

lafriks added the type/enhancement An improvement of existing functionality label Oct 8, 2018

lafriks added this to the 1.7.0 milestone Oct 8, 2018

coolaj86 mentioned this pull request Oct 20, 2018

OAuth2 auto-register #5123

Merged

techknowlogick modified the milestones: 1.7.0, 1.8.0 Dec 19, 2018

stale bot added the issue/stale label Feb 17, 2019

techknowlogick modified the milestones: 1.8.0, 1.9.0 Feb 19, 2019

stale bot removed the issue/stale label Feb 19, 2019

stale bot added the issue/stale label Apr 22, 2019

techknowlogick added issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented and removed issue/stale labels May 26, 2019

Merge branch 'master' into solderjs-ux-no-password

dd63dfc

techknowlogick reviewed Jun 29, 2019

View reviewed changes

lunny closed this Jul 1, 2019

lunny removed this from the 1.9.0 milestone Jul 1, 2019

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UX Optional Password & Captcha for External Registration #5029

UX Optional Password & Captcha for External Registration #5029

coolaj86 commented Oct 6, 2018 •

edited

Loading

codecov-io commented Oct 6, 2018 •

edited

Loading

lafriks commented Oct 8, 2018

coolaj86 commented Oct 8, 2018 •

edited

Loading

coolaj86 commented Oct 8, 2018

strk commented Oct 9, 2018 via email

coolaj86 commented Oct 9, 2018 •

edited

Loading

stale bot commented Feb 17, 2019

nickho commented Feb 21, 2019

stale bot commented Apr 22, 2019

strk commented May 29, 2019

lunny commented Jun 26, 2019

zeripath commented Jun 28, 2019

techknowlogick Jun 29, 2019

lunny commented Jul 1, 2019

	Service.RequireExternalRegistrationCaptcha = sec.Key("REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA").MustBool()
	Service.RequireExternalRegistrationCaptcha = sec.Key("REQUIRE_EXTERNAL_REGISTRATION_CAPTCHA").MustBool(Service.EnableCaptcha)

UX Optional Password & Captcha for External Registration #5029

UX Optional Password & Captcha for External Registration #5029

Conversation

coolaj86 commented Oct 6, 2018 • edited Loading

If you'd like to test it out

codecov-io commented Oct 6, 2018 • edited Loading

Codecov Report

lafriks commented Oct 8, 2018

coolaj86 commented Oct 8, 2018 • edited Loading

coolaj86 commented Oct 8, 2018

strk commented Oct 9, 2018 via email

coolaj86 commented Oct 9, 2018 • edited Loading

TL;DR

Explanation

stale bot commented Feb 17, 2019

nickho commented Feb 21, 2019

stale bot commented Apr 22, 2019

strk commented May 29, 2019

lunny commented Jun 26, 2019

zeripath commented Jun 28, 2019

techknowlogick Jun 29, 2019

Choose a reason for hiding this comment

lunny commented Jul 1, 2019

coolaj86 commented Oct 6, 2018 •

edited

Loading

codecov-io commented Oct 6, 2018 •

edited

Loading

coolaj86 commented Oct 8, 2018 •

edited

Loading

coolaj86 commented Oct 9, 2018 •

edited

Loading