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 LDAP be able to skip local 2FA #16954

Merged
merged 7 commits into from
Sep 17, 2021

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Sep 4, 2021

This PR extends #16594 to allow LDAP to be able to be set to skip local 2FA too. The technique used here would be extensible to PAM and SMTP sources.

This PR adds a setting to OAuth and OpenID login sources to allow the source to
override local 2FA requirements.

Fix go-gitea#13939

Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
This is slightly more involved than with OAuth2, but would be extensible
to PAM and SMTP.

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Sep 4, 2021
@zeripath zeripath added this to the 1.16.0 milestone Sep 4, 2021
@codecov-commenter
Copy link

codecov-commenter commented Sep 4, 2021

Codecov Report

Merging #16954 (564222a) into main (358555f) will decrease coverage by 0.01%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16954      +/-   ##
==========================================
- Coverage   45.16%   45.15%   -0.02%     
==========================================
  Files         765      765              
  Lines       86298    86317      +19     
==========================================
- Hits        38976    38974       -2     
- Misses      41014    41029      +15     
- Partials     6308     6314       +6     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
cmd/admin_auth_ldap.go 76.47% <0.00%> (-1.15%) ⬇️
modules/context/api.go 74.17% <0.00%> (-0.71%) ⬇️
modules/context/auth.go 19.81% <0.00%> (-0.37%) ⬇️
routers/web/user/auth_openid.go 0.00% <0.00%> (ø)
routers/web/user/setting/account.go 25.00% <0.00%> (ø)
services/auth/source/ldap/source.go 80.00% <ø> (ø)
services/auth/source/oauth2/source.go 25.00% <ø> (ø)
services/auth/source/oauth2/source_authenticate.go 0.00% <ø> (ø)
services/forms/auth_form.go 100.00% <ø> (ø)
... and 8 more

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 358555f...564222a. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 4, 2021
@wxiaoguang
Copy link
Contributor

Should something like LocalTwoFARequirment int be used instead of SkipLocalTwoFA bool?

eg: LocalTwoFARequirment=0 means optional, LocalTwoFARequirment=1 means required, LocalTwoFARequirment=2 means skipped.

Then the PR about Enforce Two-factor can be based on this field.

@zeripath
Copy link
Contributor Author

Should something like LocalTwoFARequirment int be used instead of SkipLocalTwoFA bool?

eg: LocalTwoFARequirment=0 means optional, LocalTwoFARequirment=1 means required, LocalTwoFARequirment=2 means skipped.

Then the PR about Enforce Two-factor can be based on this field.

I think we can just have two bools instead. With Skip overriding the others.

@GiteaBot 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 Sep 15, 2021
@@ -13,3 +13,6 @@ import (
func (source *Source) Authenticate(user *models.User, login, password string) (*models.User, error) {
return db.Authenticate(user, login, password)
}

// NB: Oauth2 does not implement LocalTwoFASkipper for password authentication
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a nit that didn't make it into #16594 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

Oauth2 Password authentication is simply a fall back to local DB source and so its skip local 2fa does not apply on that source.

This is a weirdness because of the broken way in which we have specialised the local db instead of just making it a source of authentication just like all of the others.

I have ideas for how to fix this but it's quite fiddly.

@GiteaBot 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 Sep 17, 2021
@zeripath
Copy link
Contributor Author

make lgtm work

@zeripath zeripath merged commit 27b351a into go-gitea:main Sep 17, 2021
@zeripath zeripath deleted the fix-13939-make-2fa-optional-inc-ldap branch September 17, 2021 11:43
zeripath added a commit to zeripath/gitea that referenced this pull request Sep 17, 2021
Extend go-gitea#16954 to allow setting skip local 2fa on pam and SMTP authentication sources

Signed-off-by: Andrew Thornton <art27@cantab.net>
@wxiaoguang
Copy link
Contributor

I think we can just have two bools instead. With Skip overriding the others.

It is very very strange to have two bools to indicate these exclusive options. What if Require2FA=true and Skip2FA=true ?

@zeripath
Copy link
Contributor Author

It's isomorphic.

What does 3 mean in the other scheme? Or even worse 4+? A pair of booleans is the same as the integers 0, 1, 2, 3 - but at least they can't express 4+.

Having bools suggests that the UI is a pair of checkboxes with SkipLocal2FA disabling the Enforce2FA.
Having an integer enum suggests a radio button.

I think in most cases we have used a checkboxes with disabling rather than radio buttons - in which case we should make the UI easy to make by using bools.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Sep 18, 2021

It's isomorphic.

What does 3 mean in the other scheme? Or even worse 4+? A pair of booleans is the same as the integers 0, 1, 2, 3 - but at least they can't express 4+.

Firstly, we only need three values: Default, Required, Skipped.

Secondly, these numbers are enum in source code, you won't feel uncomfortable with these codes like:

const (
	// AccessModeNone no access
	AccessModeNone AccessMode = iota // 0
	// AccessModeRead read access
	AccessModeRead // 1
	// AccessModeWrite write access
	AccessModeWrite // 2
	// AccessModeAdmin admin access
	AccessModeAdmin // 3
	// AccessModeOwner owner access
	AccessModeOwner // 4
)

Thirdly, if you do not like numbers, we can use some string constants, just use default/required/skipped for option value.

Having bools suggests that the UI is a pair of checkboxes with SkipLocal2FA disabling the Enforce2FA.
Having an integer enum suggests a radio button.

I think in most cases we have used a checkboxes with disabling rather than radio buttons - in which case we should make the UI easy to make by using bools.

A good design can make UI easier and more clear, we can use "radio group" or "drop down" instead of check-boxes.

@zeripath
Copy link
Contributor Author

I just don't understand what the problem with two bools is.

If you use an enum - be it a string or a number you're still going to have to deal with invalid values. THESE VALUES ARE NOT STORED IN A RELATIONAL TABLE. The situation is worse with strings.

Nothing about 2 bools prevents a radio group - I'm fairly certain however, that actually checkboxes would make better more consistent UI here. A dropdown is a bad idea for 3 values.

Certainly an enum flag could be faster and more efficient to parse than the current verbose json - however, we're nowhere near finished with this so we should probably think a bit more before we start early optimising to an opaque flag. In particular using an enum too early is going to make parsing an absolute nightmare.

Two bools is clearly sufficient and simple enough for handling 3 states, it's completely obvious that SkipLocal2FA beats Enforce2FA. If you were actually considering different types of Enforce2FA then that's a different question and an enum might be better.

@wxiaoguang
Copy link
Contributor

OK, I can get your point (although I can not fully agree about "checkboxes are better", "dropbox is bad idea for 3 values", "using enum makes parsing nightmare" 😊 ), since the PR was just merged and 1.16 hasn't been released, so there is still a chance to improve the whole design.

Do you have more thoughts about how to implement Enforce2FA? It can be more complicated with Skip2FA.

I just tried to introduce the Enforce2FA setting based on 1.15, it works for me, but I think there will be more work to do with both Enforce2FA and Skip2FA.

techknowlogick pushed a commit that referenced this pull request Sep 27, 2021
* Add SkipLocal2FA option to other pam and smtp sources

Extend #16954 to allow setting skip local 2fa on pam and SMTP authentication sources

Signed-off-by: Andrew Thornton <art27@cantab.net>

* make SkipLocal2FA omitempty

Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: 6543 <6543@obermui.de>
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/authentication type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants