Codecov Report

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

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

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.

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.

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.

make lgtm work

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 ?

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.

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.

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.

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.