handle device states in deny block, fix default device type (#2919) (#2924)

* handle device states in deny block, fix default device type

* fix tests

Co-authored-by: Caleb Doxsey <cdoxsey@pomerium.com>

authorize/check_response.go

@@ -39,15 +39,28 @@ func (a *Authorize) handleResultDenied(
 	ctx context.Context,
 	in *envoy_service_auth_v3.CheckRequest,
 	result *evaluator.Result,
+	isForwardAuthVerify bool,
+	reasons criteria.Reasons,
 ) (*envoy_service_auth_v3.CheckResponse, error) {
 	denyStatusCode := int32(http.StatusForbidden)
 	denyStatusText := http.StatusText(http.StatusForbidden)
 
 	switch {
-	case result.Deny.Reasons.Has(criteria.ReasonRouteNotFound):
+	case reasons.Has(criteria.ReasonUserUnauthenticated):
+		// when the user is unauthenticated it means they haven't
+		// logged in yet, so redirect to authenticate
+		return a.requireLoginResponse(ctx, in, isForwardAuthVerify)
+	case reasons.Has(criteria.ReasonDeviceUnauthenticated):
+		// when the user's device is unauthenticated it means they haven't
+		// registered a webauthn device yet, so redirect to the webauthn flow
+		return a.requireWebAuthnResponse(ctx, in, result, isForwardAuthVerify)
+	case reasons.Has(criteria.ReasonDeviceUnauthorized):
+		denyStatusCode = httputil.StatusDeviceUnauthorized
+		denyStatusText = httputil.DetailsText(httputil.StatusDeviceUnauthorized)
+	case reasons.Has(criteria.ReasonRouteNotFound):
 		denyStatusCode = http.StatusNotFound
 		denyStatusText = httputil.DetailsText(http.StatusNotFound)
-	case result.Deny.Reasons.Has(criteria.ReasonInvalidClientCertificate):
+	case reasons.Has(criteria.ReasonInvalidClientCertificate):
 		denyStatusCode = httputil.StatusInvalidClientCertificate
 		denyStatusText = httputil.DetailsText(httputil.StatusInvalidClientCertificate)
 	}
@@ -55,31 +68,6 @@ func (a *Authorize) handleResultDenied(
 	return a.deniedResponse(ctx, in, denyStatusCode, denyStatusText, nil)
 }
 
-func (a *Authorize) handleResultNotAllowed(
-	ctx context.Context,
-	in *envoy_service_auth_v3.CheckRequest,
-	result *evaluator.Result,
-	isForwardAuthVerify bool,
-) (*envoy_service_auth_v3.CheckResponse, error) {
-	switch {
-	case result.Allow.Reasons.Has(criteria.ReasonUserUnauthenticated):
-		// when the user is unauthenticated it means they haven't
-		// logged in yet, so redirect to authenticate
-		return a.requireLoginResponse(ctx, in, isForwardAuthVerify)
-	case result.Allow.Reasons.Has(criteria.ReasonDeviceUnauthenticated):
-		// when the user's device is unauthenticated it means they haven't
-		// registered a webauthn device yet, so redirect to the webauthn flow
-		return a.requireWebAuthnResponse(ctx, in, result, isForwardAuthVerify)
-	case result.Allow.Reasons.Has(criteria.ReasonDeviceUnauthorized):
-		return a.deniedResponse(ctx, in,
-			httputil.StatusDeviceUnauthorized,
-			httputil.DetailsText(httputil.StatusDeviceUnauthorized),
-			nil)
-	}
-
-	return a.deniedResponse(ctx, in, http.StatusForbidden, httputil.DetailsText(http.StatusForbidden), nil)
-}
-
 func (a *Authorize) okResponse(headers http.Header) *envoy_service_auth_v3.CheckResponse {
 	var requestHeaders []*envoy_config_core_v3.HeaderValueOption
 	for k, vs := range headers {
@@ -212,6 +200,8 @@ func (a *Authorize) requireWebAuthnResponse(
 
 	if deviceType, ok := result.Allow.AdditionalData["device_type"].(string); ok {
 		q.Set(urlutil.QueryDeviceType, deviceType)
+	} else if deviceType, ok := result.Deny.AdditionalData["device_type"].(string); ok {
+		q.Set(urlutil.QueryDeviceType, deviceType)
 	} else {
 		q.Set(urlutil.QueryDeviceType, webauthnutil.DefaultDeviceType)
 	}

authorize/grpc.go

@@ -72,16 +72,20 @@ func (a *Authorize) Check(ctx context.Context, in *envoy_service_auth_v3.CheckRe
 		a.logAuthorizeCheck(ctx, in, out, res, s, u)
 	}()
 
+	isForwardAuthVerify := isForwardAuth && hreq.URL.Path == "/verify"
+
+	// if there's a deny, the result is denied using the deny reasons.
 	if res.Deny.Value {
-		return a.handleResultDenied(ctx, in, res)
+		return a.handleResultDenied(ctx, in, res, isForwardAuthVerify, res.Deny.Reasons)
 	}
 
+	// if there's an allow, the result is allowed.
 	if res.Allow.Value {
 		return a.handleResultAllowed(ctx, in, res)
 	}
 
-	isForwardAuthVerify := isForwardAuth && hreq.URL.Path == "/verify"
+	// otherwise, the result is denied using the allow reasons.
-	return a.handleResultNotAllowed(ctx, in, res, isForwardAuthVerify)
+	return a.handleResultDenied(ctx, in, res, isForwardAuthVerify, res.Allow.Reasons)
 }
 
 func getForwardAuthURL(r *http.Request) *url.URL {

pkg/policy/criteria/device.go

@@ -8,6 +8,7 @@ import (
 	"github.com/pomerium/pomerium/pkg/policy/generator"
 	"github.com/pomerium/pomerium/pkg/policy/parser"
 	"github.com/pomerium/pomerium/pkg/policy/rules"
+	"github.com/pomerium/pomerium/pkg/webauthnutil"
 )
 
 const (
@@ -73,7 +74,7 @@ func (c deviceCriterion) GenerateRule(_ string, data parser.Value) (*ast.Rule, [
 		}...)
 	}
 
-	deviceType := "default"
+	deviceType := webauthnutil.DefaultDeviceType
 	if v, ok := obj[deviceOperatorType]; ok {
 		s, ok := v.(parser.String)
 		if !ok {

pkg/policy/criteria/device_test.go

@@ -27,7 +27,7 @@ allow:
         is: dc1
 `, []dataBrokerRecord{}, Input{Session: InputSession{ID: "s1"}})
 		require.NoError(t, err)
-		require.Equal(t, A{false, A{ReasonUserUnauthenticated}, M{"device_type": "default"}}, res["allow"])
+		require.Equal(t, A{false, A{ReasonUserUnauthenticated}, M{"device_type": "any"}}, res["allow"])
 		require.Equal(t, A{false, A{}}, res["deny"])
 	})
 	t.Run("no device credential", func(t *testing.T) {
@@ -37,10 +37,10 @@ allow:
     - device:
         is: dc1
 `, []dataBrokerRecord{
-			mkDeviceSession("s1", "default", "dc1"),
+			mkDeviceSession("s1", "any", "dc1"),
 		}, Input{Session: InputSession{ID: "s1"}})
 		require.NoError(t, err)
-		require.Equal(t, A{false, A{ReasonDeviceUnauthenticated}, M{"device_type": "default"}}, res["allow"])
+		require.Equal(t, A{false, A{ReasonDeviceUnauthenticated}, M{"device_type": "any"}}, res["allow"])
 		require.Equal(t, A{false, A{}}, res["deny"])
 	})
 	t.Run("allowed by is", func(t *testing.T) {
@@ -50,12 +50,12 @@ allow:
     - device:
         is: dc1
 `, []dataBrokerRecord{
-			mkDeviceSession("s1", "default", "dc1"),
+			mkDeviceSession("s1", "any", "dc1"),
 			&device.Credential{Id: "dc1", EnrollmentId: "de1"},
 			&device.Enrollment{Id: "de1"},
 		}, Input{Session: InputSession{ID: "s1"}})
 		require.NoError(t, err)
-		require.Equal(t, A{true, A{ReasonDeviceOK}, M{"device_type": "default"}}, res["allow"])
+		require.Equal(t, A{true, A{ReasonDeviceOK}, M{"device_type": "any"}}, res["allow"])
 		require.Equal(t, A{false, A{}}, res["deny"])
 	})
 	t.Run("not allowed by is", func(t *testing.T) {
@@ -65,14 +65,14 @@ allow:
     - device:
         is: dc2
 `, []dataBrokerRecord{
-			mkDeviceSession("s1", "default", "dc1"),
+			mkDeviceSession("s1", "any", "dc1"),
 			&device.Credential{Id: "dc1", EnrollmentId: "de1"},
 			&device.Enrollment{Id: "de1"},
 			&device.Credential{Id: "dc2", EnrollmentId: "de2"},
 			&device.Enrollment{Id: "de2"},
 		}, Input{Session: InputSession{ID: "s1"}})
 		require.NoError(t, err)
-		require.Equal(t, A{false, A{ReasonDeviceUnauthorized}, M{"device_type": "default"}}, res["allow"])
+		require.Equal(t, A{false, A{ReasonDeviceUnauthorized}, M{"device_type": "any"}}, res["allow"])
 		require.Equal(t, A{false, A{}}, res["deny"])
 	})
 	t.Run("allowed by approved", func(t *testing.T) {
@@ -82,12 +82,12 @@ allow:
     - device:
         approved: true
 `, []dataBrokerRecord{
-			mkDeviceSession("s1", "default", "dc1"),
+			mkDeviceSession("s1", "any", "dc1"),
 			&device.Credential{Id: "dc1", EnrollmentId: "de1"},
 			&device.Enrollment{Id: "de1", ApprovedBy: "u1"},
 		}, Input{Session: InputSession{ID: "s1"}})
 		require.NoError(t, err)
-		require.Equal(t, A{true, A{ReasonDeviceOK}, M{"device_type": "default"}}, res["allow"])
+		require.Equal(t, A{true, A{ReasonDeviceOK}, M{"device_type": "any"}}, res["allow"])
 		require.Equal(t, A{false, A{}}, res["deny"])
 	})
 	t.Run("not allowed by approved", func(t *testing.T) {
@@ -97,12 +97,12 @@ allow:
     - device:
         approved: true
 `, []dataBrokerRecord{
-			mkDeviceSession("s1", "default", "dc1"),
+			mkDeviceSession("s1", "any", "dc1"),
 			&device.Credential{Id: "dc1", EnrollmentId: "de1"},
 			&device.Enrollment{Id: "de1"},
 		}, Input{Session: InputSession{ID: "s1"}})
 		require.NoError(t, err)
-		require.Equal(t, A{false, A{ReasonDeviceUnauthorized}, M{"device_type": "default"}}, res["allow"])
+		require.Equal(t, A{false, A{ReasonDeviceUnauthorized}, M{"device_type": "any"}}, res["allow"])
 		require.Equal(t, A{false, A{}}, res["deny"])
 	})
 	t.Run("allowed by not approved", func(t *testing.T) {
@@ -112,12 +112,12 @@ allow:
     - device:
         approved: false
 `, []dataBrokerRecord{
-			mkDeviceSession("s1", "default", "dc1"),
+			mkDeviceSession("s1", "any", "dc1"),
 			&device.Credential{Id: "dc1", EnrollmentId: "de1"},
 			&device.Enrollment{Id: "de1"},
 		}, Input{Session: InputSession{ID: "s1"}})
 		require.NoError(t, err)
-		require.Equal(t, A{true, A{ReasonDeviceOK}, M{"device_type": "default"}}, res["allow"])
+		require.Equal(t, A{true, A{ReasonDeviceOK}, M{"device_type": "any"}}, res["allow"])
 		require.Equal(t, A{false, A{}}, res["deny"])
 	})
 	t.Run("not allowed by not approved", func(t *testing.T) {
@@ -127,12 +127,12 @@ allow:
     - device:
         approved: false
 `, []dataBrokerRecord{
-			mkDeviceSession("s1", "default", "dc1"),
+			mkDeviceSession("s1", "any", "dc1"),
 			&device.Credential{Id: "dc1", EnrollmentId: "de1"},
 			&device.Enrollment{Id: "de1", ApprovedBy: "u1"},
 		}, Input{Session: InputSession{ID: "s1"}})
 		require.NoError(t, err)
-		require.Equal(t, A{false, A{ReasonDeviceUnauthorized}, M{"device_type": "default"}}, res["allow"])
+		require.Equal(t, A{false, A{ReasonDeviceUnauthorized}, M{"device_type": "any"}}, res["allow"])
 		require.Equal(t, A{false, A{}}, res["deny"])
 	})
 	t.Run("allowed by type", func(t *testing.T) {