[FIX] Redirect to default URL after 1FA when default policy is one_fa…

…ctor. (#611) * Redirect to default URL after 1FA when default policy is one_factor. User is now redirected to the default redirection URL after 1FA if the default policy is set to one_factor and there is no target URL or if the target URL is unsafe. Also, if the default policy is set to one_factor and the user is already authenticated, if she visits the login portal, the 'already authenticated' view is displayed with a logout button. This fixes #581. * Update users.yml * Fix permissions issue causing suite test failure
authelia · Feb 4, 2020 · d1d02d9 · d1d02d9
1 parent 9c9d851
commit d1d02d9
Show file tree

Hide file tree

Showing 37 changed files with 1,481 additions and 117 deletions.
diff --git a/cmd/authelia-scripts/cmd_suites.go b/cmd/authelia-scripts/cmd_suites.go
@@ -27,9 +27,11 @@ var ErrNoRunningSuite = errors.New("no running suite")
 var runningSuiteFile = ".suite"
 
 var headless bool
+var testPattern string
 
 func init() {
 	SuitesTestCmd.Flags().BoolVar(&headless, "headless", false, "Run tests in headless mode")
+	SuitesTestCmd.Flags().StringVar(&testPattern, "test", "", "The single test to run")
 }
 
 // SuitesListCmd Command for listing the available suites.
@@ -184,15 +186,14 @@ func testSuite(cmd *cobra.Command, args []string) {
 	}
 
 	// If suite(s) are provided as argument
-	if len(args) == 1 {
+	if len(args) >= 1 {
 		suiteArg := args[0]
 
 		if runningSuite != "" && suiteArg != runningSuite {
 			log.Fatal(errors.New("Running suite (" + runningSuite + ") is different than suite(s) to be tested (" + suiteArg + "). Shutdown running suite and retry"))
 		}
 
-		suiteNames := strings.Split(suiteArg, ",")
-		if err := runMultipleSuitesTests(suiteNames, runningSuite == ""); err != nil {
+		if err := runMultipleSuitesTests(strings.Split(suiteArg, ","), runningSuite == ""); err != nil {
 			log.Fatal(err)
 		}
 	} else {
@@ -239,7 +240,13 @@ func runSuiteTests(suiteName string, withEnv bool) error {
 	if suite.TestTimeout > 0 {
 		timeout = fmt.Sprintf("%ds", int64(suite.TestTimeout/time.Second))
 	}
-	testCmdLine := fmt.Sprintf("go test -count=1 -v ./internal/suites -timeout %s -run '^(Test%sSuite)$'", timeout, suiteName)
+	testCmdLine := fmt.Sprintf("go test -count=1 -v ./internal/suites -timeout %s ", timeout)
+
+	if testPattern != "" {
+		testCmdLine += fmt.Sprintf("-run '%s'", testPattern)
+	} else {
+		testCmdLine += fmt.Sprintf("-run '^(Test%sSuite)$'", suiteName)
+	}
 
 	log.Infof("Running tests of suite %s...", suiteName)
 	log.Debugf("Running tests with command: %s", testCmdLine)

diff --git a/cmd/authelia/main.go b/cmd/authelia/main.go
@@ -87,7 +87,7 @@ func startServer() {
 	}
 
 	clock := utils.RealClock{}
-	authorizer := authorization.NewAuthorizer(*config.AccessControl)
+	authorizer := authorization.NewAuthorizer(config.AccessControl)
 	sessionProvider := session.NewProvider(config.Session)
 	regulator := regulation.NewRegulator(config.Regulation, storageProvider, clock)
 

diff --git a/internal/authorization/authorizer.go b/internal/authorization/authorizer.go
@@ -160,7 +160,7 @@ func selectMatchingRules(rules []schema.ACLRule, subject Subject, object Object)
 	return selectMatchingObjectRules(matchingRules, object)
 }
 
-func policyToLevel(policy string) Level {
+func PolicyToLevel(policy string) Level {
 	switch policy {
 	case "bypass":
 		return Bypass
@@ -183,7 +183,7 @@ func (p *Authorizer) GetRequiredLevel(subject Subject, requestURL url.URL) Level
 	})
 
 	if len(matchingRules) > 0 {
-		return policyToLevel(matchingRules[0].Policy)
+		return PolicyToLevel(matchingRules[0].Policy)
 	}
-	return policyToLevel(p.configuration.DefaultPolicy)
+	return PolicyToLevel(p.configuration.DefaultPolicy)
 }
diff --git a/internal/authorization/authorizer_test.go b/internal/authorization/authorizer_test.go
@@ -255,6 +255,15 @@ func (s *AuthorizerSuite) TestShouldCheckResourceMatching() {
 	tester.CheckAuthorizations(s.T(), John, "https://resource.example.com/xyz/embedded/abc", Bypass)
 }
 
+func (s *AuthorizerSuite) TestPolicyToLevel() {
+	s.Assert().Equal(Bypass, PolicyToLevel("bypass"))
+	s.Assert().Equal(OneFactor, PolicyToLevel("one_factor"))
+	s.Assert().Equal(TwoFactor, PolicyToLevel("two_factor"))
+	s.Assert().Equal(Denied, PolicyToLevel("deny"))
+
+	s.Assert().Equal(Denied, PolicyToLevel("whatever"))
+}
+
 func TestRunSuite(t *testing.T) {
 	s := AuthorizerSuite{}
 	suite.Run(t, &s)

diff --git a/internal/configuration/schema/configuration.go b/internal/configuration/schema/configuration.go
@@ -15,10 +15,10 @@ type Configuration struct {
 	AuthenticationBackend AuthenticationBackendConfiguration `mapstructure:"authentication_backend"`
 	Session               SessionConfiguration               `mapstructure:"session"`
 
-	TOTP          *TOTPConfiguration          `mapstructure:"totp"`
-	DuoAPI        *DuoAPIConfiguration        `mapstructure:"duo_api"`
-	AccessControl *AccessControlConfiguration `mapstructure:"access_control"`
-	Regulation    *RegulationConfiguration    `mapstructure:"regulation"`
-	Storage       *StorageConfiguration       `mapstructure:"storage"`
-	Notifier      *NotifierConfiguration      `mapstructure:"notifier"`
+	TOTP          *TOTPConfiguration         `mapstructure:"totp"`
+	DuoAPI        *DuoAPIConfiguration       `mapstructure:"duo_api"`
+	AccessControl AccessControlConfiguration `mapstructure:"access_control"`
+	Regulation    *RegulationConfiguration   `mapstructure:"regulation"`
+	Storage       *StorageConfiguration      `mapstructure:"storage"`
+	Notifier      *NotifierConfiguration     `mapstructure:"notifier"`
 }
diff --git a/internal/configuration/validator/configuration.go b/internal/configuration/validator/configuration.go
@@ -49,5 +49,9 @@ func Validate(configuration *schema.Configuration, validator *schema.StructValid
 		ValidateNotifier(configuration.Notifier, validator)
 	}
 
+	if configuration.AccessControl.DefaultPolicy == "" {
+		configuration.AccessControl.DefaultPolicy = "deny"
+	}
+
 	ValidateSQLStorage(configuration.Storage, validator)
 }
diff --git a/internal/configuration/validator/configuration_test.go b/internal/configuration/validator/configuration_test.go
@@ -91,3 +91,13 @@ func TestShouldEnsureNotifierConfigIsProvided(t *testing.T) {
 	require.Len(t, validator.Errors(), 1)
 	assert.EqualError(t, validator.Errors()[0], "A notifier configuration must be provided")
 }
+
+func TestShouldAddDefaultAccessControl(t *testing.T) {
+	validator := schema.NewStructValidator()
+	config := newDefaultConfig()
+
+	Validate(&config, validator)
+	require.Len(t, validator.Errors(), 0)
+	assert.NotNil(t, config.AccessControl)
+	assert.Equal(t, "deny", config.AccessControl.DefaultPolicy)
+}
diff --git a/internal/handlers/handler_extended_configuration.go b/internal/handlers/handler_extended_configuration.go
@@ -2,11 +2,15 @@ package handlers
 
 import (
 	"github.com/authelia/authelia/internal/authentication"
+	"github.com/authelia/authelia/internal/authorization"
 	"github.com/authelia/authelia/internal/middlewares"
 )
 
 type ExtendedConfigurationBody struct {
 	AvailableMethods MethodList `json:"available_methods"`
+
+	// OneFactorDefaultPolicy is set if default policy is 'one_factor'
+	OneFactorDefaultPolicy bool `json:"one_factor_default_policy"`
 }
 
 // ExtendedConfigurationGet get the extended configuration accessible to authenticated users.
@@ -18,6 +22,10 @@ func ExtendedConfigurationGet(ctx *middlewares.AutheliaCtx) {
 		body.AvailableMethods = append(body.AvailableMethods, authentication.Push)
 	}
 
-	ctx.Logger.Debugf("Available methods are %s", body.AvailableMethods)
+	defaultPolicy := authorization.PolicyToLevel(ctx.Configuration.AccessControl.DefaultPolicy)
+	body.OneFactorDefaultPolicy = defaultPolicy == authorization.OneFactor
+	ctx.Logger.Tracef("Default policy set to one factor: %v", body.OneFactorDefaultPolicy)
+
+	ctx.Logger.Tracef("Available methods are %s", body.AvailableMethods)
 	ctx.SetJSONBody(body)
 }
diff --git a/internal/handlers/handler_firstfactor.go b/internal/handlers/handler_firstfactor.go
@@ -2,15 +2,12 @@ package handlers
 
 import (
 	"fmt"
-	"net/url"
 	"time"
 
 	"github.com/authelia/authelia/internal/authentication"
-	"github.com/authelia/authelia/internal/authorization"
 	"github.com/authelia/authelia/internal/middlewares"
 	"github.com/authelia/authelia/internal/regulation"
 	"github.com/authelia/authelia/internal/session"
-	"github.com/authelia/authelia/internal/utils"
 )
 
 // FirstFactorPost is the handler performing the first factory.
@@ -111,30 +108,5 @@ func FirstFactorPost(ctx *middlewares.AutheliaCtx) {
 		return
 	}
 
-	if bodyJSON.TargetURL != "" {
-		targetURL, err := url.ParseRequestURI(bodyJSON.TargetURL)
-		if err != nil {
-			ctx.Error(fmt.Errorf("Unable to parse target URL %s: %s", bodyJSON.TargetURL, err), authenticationFailedMessage)
-			return
-		}
-		requiredLevel := ctx.Providers.Authorizer.GetRequiredLevel(authorization.Subject{
-			Username: userSession.Username,
-			Groups:   userSession.Groups,
-			IP:       ctx.RemoteIP(),
-		}, *targetURL)
-
-		ctx.Logger.Debugf("Required level for the URL %s is %d", targetURL.String(), requiredLevel)
-
-		safeRedirection := utils.IsRedirectionSafe(*targetURL, ctx.Configuration.Session.Domain)
-
-		if safeRedirection && requiredLevel <= authorization.OneFactor {
-			ctx.Logger.Debugf("Redirection is safe, redirecting...")
-			response := redirectResponse{bodyJSON.TargetURL}
-			ctx.SetJSONBody(response)
-		} else {
-			ctx.ReplyOK()
-		}
-	} else {
-		ctx.ReplyOK()
-	}
+	Handle1FAResponse(ctx, bodyJSON.TargetURL, userSession.Username, userSession.Groups)
 }
diff --git a/internal/handlers/handler_firstfactor_test.go b/internal/handlers/handler_firstfactor_test.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/authelia/authelia/internal/authorization"
 	"github.com/authelia/authelia/internal/mocks"
 	"github.com/authelia/authelia/internal/models"
 
@@ -229,7 +230,76 @@ func (s *FirstFactorSuite) TestShouldAuthenticateUserWithRememberMeUnchecked() {
 	assert.Equal(s.T(), []string{"dev", "admins"}, session.Groups)
 }
 
+type FirstFactorRedirectionSuite struct {
+	suite.Suite
+
+	mock *mocks.MockAutheliaCtx
+}
+
+func (s *FirstFactorRedirectionSuite) SetupTest() {
+	s.mock = mocks.NewMockAutheliaCtx(s.T())
+	s.mock.Ctx.Configuration.DefaultRedirectionURL = "https://default.local"
+	s.mock.Ctx.Configuration.AccessControl.DefaultPolicy = "one_factor"
+	s.mock.Ctx.Providers.Authorizer = authorization.NewAuthorizer(
+		s.mock.Ctx.Configuration.AccessControl)
+
+	s.mock.UserProviderMock.
+		EXPECT().
+		CheckUserPassword(gomock.Eq("test"), gomock.Eq("hello")).
+		Return(true, nil)
+
+	s.mock.UserProviderMock.
+		EXPECT().
+		GetDetails(gomock.Eq("test")).
+		Return(&authentication.UserDetails{
+			Emails: []string{"test@example.com"},
+			Groups: []string{"dev", "admins"},
+		}, nil)
+
+	s.mock.StorageProviderMock.
+		EXPECT().
+		AppendAuthenticationLog(gomock.Any()).
+		Return(nil)
+}
+
+func (s *FirstFactorRedirectionSuite) TearDownTest() {
+	s.mock.Close()
+}
+
+// When the target url is unknown, default policy is to one_factor and default_redirect_url
+// is provided, the user should be redirected to the default url.
+func (s *FirstFactorRedirectionSuite) TestShouldRedirectUserToDefaultRedirectionURLWhenNoTargetURLProvided() {
+	s.mock.Ctx.Request.SetBodyString(`{
+		"username": "test",
+		"password": "hello",
+		"keepMeLoggedIn": false
+	}`)
+	FirstFactorPost(s.mock.Ctx)
+
+	// Respond with 200.
+	s.mock.Assert200OK(s.T(), redirectResponse{
+		Redirect: "https://default.local",
+	})
+}
+
+// When the target url is unsafe, default policy is set to one_factor and default_redirect_url
+// is provided, the user should be redirected to the default url.
+func (s *FirstFactorRedirectionSuite) TestShouldRedirectUserToDefaultRedirectionURLWhenURLIsUnsafe() {
+	s.mock.Ctx.Request.SetBodyString(`{
+		"username": "test",
+		"password": "hello",
+		"keepMeLoggedIn": false,
+		"targetURL": "http://notsafe.local"
+	}`)
+	FirstFactorPost(s.mock.Ctx)
+
+	// Respond with 200.
+	s.mock.Assert200OK(s.T(), redirectResponse{
+		Redirect: "https://default.local",
+	})
+}
+
 func TestFirstFactorSuite(t *testing.T) {
-	firstFactorSuite := new(FirstFactorSuite)
-	suite.Run(t, firstFactorSuite)
+	suite.Run(t, new(FirstFactorSuite))
+	suite.Run(t, new(FirstFactorRedirectionSuite))
 }
diff --git a/internal/handlers/handler_sign_duo.go b/internal/handlers/handler_sign_duo.go
@@ -51,6 +51,6 @@ func SecondFactorDuoPost(duoAPI duo.API) middlewares.RequestHandler {
 			return
 		}
 
-		HandleAuthResponse(ctx, requestBody.TargetURL)
+		Handle2FAResponse(ctx, requestBody.TargetURL)
 	}
 }
diff --git a/internal/handlers/handler_sign_totp.go b/internal/handlers/handler_sign_totp.go
@@ -40,6 +40,6 @@ func SecondFactorTOTPPost(totpVerifier TOTPVerifier) middlewares.RequestHandler
 			return
 		}
 
-		HandleAuthResponse(ctx, bodyJSON.TargetURL)
+		Handle2FAResponse(ctx, bodyJSON.TargetURL)
 	}
 }
diff --git a/internal/handlers/handler_sign_u2f_step2.go b/internal/handlers/handler_sign_u2f_step2.go
@@ -48,6 +48,6 @@ func SecondFactorU2FSignPost(u2fVerifier U2FVerifier) middlewares.RequestHandler
 			return
 		}
 
-		HandleAuthResponse(ctx, requestBody.TargetURL)
+		Handle2FAResponse(ctx, requestBody.TargetURL)
 	}
 }