Skip to content

Commit

Permalink
Address comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
@Joerger
Joerger committed Oct 10, 2024
1 parent 6ab4fdc commit 07c1201
Show file tree
Hide file tree
Showing 5 changed files with 103 additions and 31 deletions.
3 changes: 1 addition & 2 deletions api/constants/constants.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,7 @@ const (
// is required for all users.
SecondFactorWebauthn = SecondFactorType("webauthn")
// SecondFactorOn means that all 2FA protocols are supported and 2FA is
// required for all users, or that SSO MFA is enabled which cannot be
// expressed through a different SecondFactorType.
// required for all users.
SecondFactorOn = SecondFactorType("on")
// SecondFactorOptional means that all 2FA protocols are supported and 2FA
// is required only for users that have MFA devices registered.
Expand Down
16 changes: 12 additions & 4 deletions api/types/authentication.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,8 @@ type AuthPreference interface {
IsSecondFactorEnabled() bool
// IsSecondFactorEnforced checks if second factor is enforced.
IsSecondFactorEnforced() bool
// IsSecondFactorLocalAllowed checks if a local second factor method is enabled (webauthn, totp).
IsSecondFactorLocalAllowed() bool
// IsSecondFactorTOTPAllowed checks if users can use TOTP as an MFA method.
IsSecondFactorTOTPAllowed() bool
// IsSecondFactorWebauthnAllowed checks if users can use WebAuthn as an MFA method.
Expand Down Expand Up @@ -384,6 +386,11 @@ func (c *AuthPreferenceV2) IsSecondFactorEnforced() bool {
return len(c.GetSecondFactors()) > 0 && c.Spec.SecondFactor != constants.SecondFactorOptional
}

// IsSecondFactorLocalAllowed checks if a local second factor method is enabled.
func (c *AuthPreferenceV2) IsSecondFactorLocalAllowed() bool {
return c.IsSecondFactorTOTPAllowed() || c.IsSecondFactorWebauthnAllowed()
}

// IsSecondFactorTOTPAllowed checks if users can use TOTP as an MFA method.
func (c *AuthPreferenceV2) IsSecondFactorTOTPAllowed() bool {
return slices.Contains(c.GetSecondFactors(), SecondFactorType_SECOND_FACTOR_TYPE_OTP)
Expand Down Expand Up @@ -776,10 +783,11 @@ func (c *AuthPreferenceV2) CheckAndSetDefaults() error {
}

// Prevent local lockout by disabling local second factor methods.
hasSSO := c.IsSecondFactorSSOAllowed()
hasTOTP := c.IsSecondFactorTOTPAllowed()
if c.GetAllowLocalAuth() && hasSSO && !hasTOTP && !hasWebauthn {
return trace.BadParameter("missing a local second factor method for local users (otp, webauthn), either add a local second factor method or disable local auth")
if c.GetAllowLocalAuth() && c.IsSecondFactorEnforced() && !c.IsSecondFactorLocalAllowed() {
if c.IsSecondFactorSSOAllowed() {
trace.BadParameter("missing a local second factor method for local users (otp, webauthn), either add a local second factor method or disable local auth")
}
return trace.BadParameter("missing a local second factor method for local users (otp, webauthn)")
}

// Validate connector name for type=local.
Expand Down
64 changes: 46 additions & 18 deletions api/types/authentication_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package types
import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/gravitational/trace"
Expand Down Expand Up @@ -69,7 +70,7 @@ func TestEncodeDecodeRequireMFAType(t *testing.T) {
}
}

func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
func TestNewAuthPreference_secondFactors(t *testing.T) {
for _, tt := range []struct {
name string
spec AuthPreferenceSpecV2
Expand All @@ -80,23 +81,25 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
name: "OK default to OTP",
spec: AuthPreferenceSpecV2{},
assertAuthPref: func(t *testing.T, authPref AuthPreference) {
require.Equal(t, []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_OTP}, authPref.GetSecondFactors())
assert.Equal(t, []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_OTP}, authPref.GetSecondFactors())
},
},
{
name: "OK OTP",
name: "OK OTP default settings",
spec: AuthPreferenceSpecV2{
SecondFactors: []SecondFactorType{
SecondFactorType_SECOND_FACTOR_TYPE_OTP,
},
},
assertAuthPref: func(t *testing.T, authPref AuthPreference) {
require.False(t, authPref.GetAllowPasswordless())
require.False(t, authPref.GetAllowHeadless())
assert.False(t, authPref.GetAllowPasswordless())
assert.False(t, authPref.GetAllowHeadless())
assert.True(t, authPref.GetAllowLocalAuth())
assert.False(t, authPref.IsAdminActionMFAEnforced())
},
},
{
name: "OK WebAuthn",
name: "OK WebAuthn default settings",
spec: AuthPreferenceSpecV2{
SecondFactors: []SecondFactorType{
SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN,
Expand All @@ -106,22 +109,42 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
},
},
assertAuthPref: func(t *testing.T, authPref AuthPreference) {
require.True(t, authPref.GetAllowPasswordless())
require.True(t, authPref.GetAllowHeadless())
assert.True(t, authPref.GetAllowPasswordless())
assert.True(t, authPref.GetAllowHeadless())
assert.True(t, authPref.GetAllowLocalAuth())
assert.True(t, authPref.IsAdminActionMFAEnforced())
},
},
{
name: "OK SSO",
name: "OK SSO default settings",
spec: AuthPreferenceSpecV2{
SecondFactors: []SecondFactorType{
SecondFactorType_SECOND_FACTOR_TYPE_SSO,
},
AllowLocalAuth: NewBoolOption(false),
},
assertAuthPref: func(t *testing.T, authPref AuthPreference) {
require.False(t, authPref.GetAllowPasswordless())
require.False(t, authPref.GetAllowHeadless())
require.False(t, authPref.GetAllowLocalAuth())
assert.False(t, authPref.GetAllowPasswordless())
assert.False(t, authPref.GetAllowHeadless())
assert.False(t, authPref.GetAllowLocalAuth())
assert.True(t, authPref.IsAdminActionMFAEnforced())
},
},
{
name: "OK all second factors",
spec: AuthPreferenceSpecV2{
SecondFactors: []SecondFactorType{
SecondFactorType_SECOND_FACTOR_TYPE_OTP,
SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN,
SecondFactorType_SECOND_FACTOR_TYPE_SSO,
},
},
assertAuthPref: func(t *testing.T, authPref AuthPreference) {
assert.True(t, authPref.GetAllowPasswordless())
assert.True(t, authPref.GetAllowHeadless())
assert.True(t, authPref.GetAllowLocalAuth())
// enabling OTP disables admin mfa.
assert.False(t, authPref.IsAdminActionMFAEnforced())
},
},
{
Expand All @@ -134,6 +157,11 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
AppID: "https://localhost",
},
},
assertAuthPref: func(t *testing.T, authPref AuthPreference) {
w, err := authPref.GetWebauthn()
assert.NoError(t, err)
assert.Equal(t, &Webauthn{RPID: "localhost"}, w)
},
},
{
name: "NOK SecondFactor and SecondFactors both set",
Expand All @@ -144,7 +172,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
},
},
assertErr: func(t require.TestingT, err error, vals ...interface{}) {
require.ErrorAs(t, err, &trace.BadParameterError{})
assert.ErrorAs(t, err, new(*trace.BadParameterError))
},
},
{
Expand All @@ -155,7 +183,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
},
},
assertErr: func(t require.TestingT, err error, vals ...interface{}) {
require.ErrorAs(t, err, &trace.BadParameterError{})
assert.ErrorAs(t, err, new(*trace.BadParameterError))
},
},
{
Expand All @@ -167,7 +195,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
AllowPasswordless: NewBoolOption(true),
},
assertErr: func(t require.TestingT, err error, vals ...interface{}) {
require.ErrorAs(t, err, &trace.BadParameterError{})
assert.ErrorAs(t, err, new(*trace.BadParameterError))
},
},
{
Expand All @@ -179,7 +207,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
AllowHeadless: NewBoolOption(true),
},
assertErr: func(t require.TestingT, err error, vals ...interface{}) {
require.ErrorAs(t, err, &trace.BadParameterError{})
assert.ErrorAs(t, err, new(*trace.BadParameterError))
},
},
{
Expand All @@ -191,7 +219,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
AllowLocalAuth: NewBoolOption(true),
},
assertErr: func(t require.TestingT, err error, vals ...interface{}) {
require.ErrorAs(t, err, &trace.BadParameterError{})
assert.ErrorAs(t, err, new(*trace.BadParameterError))
},
},
} {
Expand All @@ -200,7 +228,7 @@ func TestAuthPreference_CheckAndSetDefaults(t *testing.T) {
if tt.assertErr != nil {
tt.assertErr(t, err)
} else {
require.NoError(t, err)
assert.NoError(t, err)
}

if tt.assertAuthPref != nil {
Expand Down
2 changes: 1 addition & 1 deletion api/types/second_factor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ func legacySecondFactorFromSecondFactors(secondFactors []SecondFactorType) const
hasWebAuthn := slices.Contains(secondFactors, SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN)

switch {
case (hasOTP && hasWebAuthn):
case hasOTP && hasWebAuthn:
return constants.SecondFactorOn
case hasWebAuthn:
return constants.SecondFactorWebauthn
Expand Down
49 changes: 43 additions & 6 deletions api/types/second_factor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ package types
import (
"testing"

"github.com/stretchr/testify/require"
"github.com/stretchr/testify/assert"

"github.com/gravitational/teleport/api/constants"
)

// TestEncodeDecodeSecondFactorType tests encoding/decoding of the SecondFactorType.
func TestEncodeDecodeSecondFactorType(t *testing.T) {
for _, tt := range []struct {
secondFactorType SecondFactorType
Expand All @@ -42,16 +43,52 @@ func TestEncodeDecodeSecondFactorType(t *testing.T) {
t.Run(tt.secondFactorType.String(), func(t *testing.T) {
t.Run("encode", func(t *testing.T) {
encoded, err := tt.secondFactorType.encode()
require.NoError(t, err)
require.Equal(t, tt.encoded, encoded)
assert.NoError(t, err)
assert.Equal(t, tt.encoded, encoded)
})

t.Run("decode", func(t *testing.T) {
var decoded SecondFactorType
err := decoded.decode(tt.encoded)
require.NoError(t, err)
require.Equal(t, tt.secondFactorType, decoded)
assert.NoError(t, err)
assert.Equal(t, tt.secondFactorType, decoded)
})
})
}
}

func TestLegacySecondFactorsFromLegacySecondFactor(t *testing.T) {
for _, tt := range []struct {
sf constants.SecondFactorType
sfs []SecondFactorType
}{
{
sf: "",
sfs: nil,
},
{
sf: constants.SecondFactorOff,
sfs: nil,
},
{
sf: constants.SecondFactorOptional,
sfs: []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN, SecondFactorType_SECOND_FACTOR_TYPE_OTP},
},
{
sf: constants.SecondFactorOn,
sfs: []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN, SecondFactorType_SECOND_FACTOR_TYPE_OTP},
},
{
sf: constants.SecondFactorOTP,
sfs: []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_OTP},
},
{
sf: constants.SecondFactorWebauthn,
sfs: []SecondFactorType{SecondFactorType_SECOND_FACTOR_TYPE_WEBAUTHN},
},
} {
t.Run(string(tt.sf), func(t *testing.T) {
assert.Equal(t, tt.sfs, secondFactorsFromLegacySecondFactor(tt.sf))
})
}
}

0 comments on commit 07c1201

Please sign in to comment.