From 928209dc980dadef16061cde83c3dca9384a7ff0 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Thu, 3 Aug 2017 00:30:41 +0200 Subject: [PATCH] Fix redirection after authentication and error page when accessing restricted pages --- example/nginx/nginx.conf | 1 + src/server/lib/routes/verify/get.ts | 6 ++- src/server/views/errors/401.pug | 2 +- src/server/views/errors/403.pug | 2 +- test/features/redirection.feature | 20 +++++++- .../step_definitions/authentication.ts | 22 ++++---- .../features/step_definitions/restrictions.ts | 4 +- test/features/support/world.ts | 33 ++++++++---- test/unit/server/routes/verify/get.test.ts | 50 ++++++++++--------- 9 files changed, 89 insertions(+), 51 deletions(-) diff --git a/example/nginx/nginx.conf b/example/nginx/nginx.conf index c9688f8f3..a91df4ce6 100644 --- a/example/nginx/nginx.conf +++ b/example/nginx/nginx.conf @@ -61,6 +61,7 @@ http { ssl_certificate_key /etc/ssl/server.key; error_page 401 = @error401; + error_page 403 = https://auth.test.local:8080/error/403; location @error401 { return 302 https://auth.test.local:8080; } diff --git a/src/server/lib/routes/verify/get.ts b/src/server/lib/routes/verify/get.ts index 1424bb30a..59d06387b 100644 --- a/src/server/lib/routes/verify/get.ts +++ b/src/server/lib/routes/verify/get.ts @@ -39,13 +39,15 @@ function verify_filter(req: express.Request, res: express.Response): BluebirdPro }); } -export default function (req: express.Request, res: express.Response) { +export default function (req: express.Request, res: express.Response): BluebirdPromise { const logger = ServerVariablesHandler.getLogger(req.app); - verify_filter(req, res) + return verify_filter(req, res) .then(function () { res.status(204); res.send(); + return BluebirdPromise.resolve(); }) + .catch(exceptions.DomainAccessDenied, ErrorReplies.replyWithError403(res, logger)) .catch(ErrorReplies.replyWithError401(res, logger)); } diff --git a/src/server/views/errors/401.pug b/src/server/views/errors/401.pug index 3cb97413f..b0dfae0e9 100644 --- a/src/server/views/errors/401.pug +++ b/src/server/views/errors/401.pug @@ -8,4 +8,4 @@ block form-header block content -

You are either not authorized or not logged in.

+

Please log in to access this resource.

diff --git a/src/server/views/errors/403.pug b/src/server/views/errors/403.pug index b2dc8be65..605f6f389 100644 --- a/src/server/views/errors/403.pug +++ b/src/server/views/errors/403.pug @@ -8,4 +8,4 @@ block form-header block content -

You are either not authorized or not logged in.

+

You are not authorized to access this resource.

diff --git a/test/features/redirection.feature b/test/features/redirection.feature index 7b8f24acb..eaafa7d04 100644 --- a/test/features/redirection.feature +++ b/test/features/redirection.feature @@ -1,7 +1,23 @@ -Feature: User is redirected to authelia when he is not authenticated +Feature: User is correctly redirected correctly - Scenario: User is redirected to authelia + Scenario: User is redirected to authelia when he is not authenticated Given I'm on https://home.test.local:8080 When I click on the link to secret.test.local Then I'm redirected to "https://auth.test.local:8080/" + Scenario: User is redirected to home page after several authentication tries + Given I'm on https://auth.test.local:8080/ + And I login with user "john" and password "password" + And I register a TOTP secret called "Sec0" + And I visit "https://public.test.local:8080/secret.html" + When I login with user "john" and password "badpassword" + And I clear field "username" + And I login with user "john" and password "password" + And I use "Sec0" as TOTP token handle + And I click on "TOTP" + Then I'm redirected to "https://public.test.local:8080/secret.html" + + Scenario: User Harry does not have access to https://secret.test.local:8080/secret.html and thus he must get an error 401 + When I register TOTP and login with user "harry" and password "password" + And I visit "https://secret.test.local:8080/secret.html" + Then I get an error 403 \ No newline at end of file diff --git a/test/features/step_definitions/authentication.ts b/test/features/step_definitions/authentication.ts index ae86f3f40..853a3f640 100644 --- a/test/features/step_definitions/authentication.ts +++ b/test/features/step_definitions/authentication.ts @@ -14,6 +14,10 @@ Cucumber.defineSupportCode(function ({ Given, When, Then }) { return this.setFieldTo(fieldName, content); }); + When("I clear field {stringInDoubleQuotes}", function (fieldName: string) { + return this.clearField(fieldName); + }); + When("I click on {stringInDoubleQuotes}", function (text: string) { return this.clickOnButton(text); }); @@ -55,20 +59,20 @@ Cucumber.defineSupportCode(function ({ Given, When, Then }) { return this.registerTotpAndSignin(username, password); }); - function hasAccessToSecret(link: string, driver: any) { - return driver.get(link) + function hasAccessToSecret(link: string, that: any) { + return that.driver.get(link) .then(function () { - return driver.findElement(seleniumWebdriver.By.tagName("body")).getText() + return that.driver.findElement(seleniumWebdriver.By.tagName("body")).getText() .then(function (body: string) { - Assert(body.indexOf("This is a very important secret!") > -1); + Assert(body.indexOf("This is a very important secret!") > -1, body); }); }); } - function hasNoAccessToSecret(link: string, driver: any) { - return driver.get(link) + function hasNoAccessToSecret(link: string, that: any) { + return that.driver.get(link) .then(function () { - return driver.wait(seleniumWebdriver.until.urlIs("https://auth.test.local:8080/")); + return that.getErrorPage(403); }); } @@ -76,7 +80,7 @@ Cucumber.defineSupportCode(function ({ Given, When, Then }) { const promises = []; for (let i = 0; i < dataTable.rows().length; i++) { const url = (dataTable.hashes() as any)[i].url; - promises.push(hasAccessToSecret(url, this.driver)); + promises.push(hasAccessToSecret(url, this)); } return Promise.all(promises); }); @@ -85,7 +89,7 @@ Cucumber.defineSupportCode(function ({ Given, When, Then }) { const promises = []; for (let i = 0; i < dataTable.rows().length; i++) { const url = (dataTable.hashes() as any)[i].url; - promises.push(hasNoAccessToSecret(url, this.driver)); + promises.push(hasNoAccessToSecret(url, this)); } return Promise.all(promises); }); diff --git a/test/features/step_definitions/restrictions.ts b/test/features/step_definitions/restrictions.ts index cf3ab289a..f319ffb8e 100644 --- a/test/features/step_definitions/restrictions.ts +++ b/test/features/step_definitions/restrictions.ts @@ -4,8 +4,6 @@ import Assert = require("assert"); Cucumber.defineSupportCode(function ({ Given, When, Then }) { Then("I get an error {number}", function (code: number) { - return this.driver - .findElement(seleniumWebdriver.By.tagName("h1")) - .findElement(seleniumWebdriver.By.xpath("//h1[contains(.,'Error " + code + "')]")); + return this.getErrorPage(code); }); }); \ No newline at end of file diff --git a/test/features/support/world.ts b/test/features/support/world.ts index 367b4df07..97bbf9f85 100644 --- a/test/features/support/world.ts +++ b/test/features/support/world.ts @@ -21,6 +21,16 @@ function CustomWorld() { .sendKeys(content); }; + this.clearField = function (fieldName: string) { + return this.driver.findElement(seleniumWebdriver.By.id(fieldName)).clear(); + }; + + this.getErrorPage = function (code: number) { + return this.driver + .findElement(seleniumWebdriver.By.tagName("h1")) + .findElement(seleniumWebdriver.By.xpath("//h1[contains(.,'Error " + code + "')]")); + }; + this.clickOnButton = function (buttonText: string) { return this.driver .findElement(seleniumWebdriver.By.tagName("button")) @@ -29,9 +39,11 @@ function CustomWorld() { }; this.loginWithUserPassword = function (username: string, password: string) { - return this.driver - .findElement(seleniumWebdriver.By.id("username")) - .sendKeys(username) + return that.driver.wait(seleniumWebdriver.until.elementLocated(seleniumWebdriver.By.id("username")), 4000) + .then(function () { + return that.driver.findElement(seleniumWebdriver.By.id("username")) + .sendKeys(username); + }) .then(function () { return that.driver.findElement(seleniumWebdriver.By.id("password")) .sendKeys(password); @@ -39,14 +51,14 @@ function CustomWorld() { .then(function () { return that.driver.findElement(seleniumWebdriver.By.tagName("button")) .click(); - }) - .then(function () { - return that.driver.wait(seleniumWebdriver.until.elementLocated(seleniumWebdriver.By.className("register-totp")), 4000); }); }; this.registerTotpSecret = function (totpSecretHandle: string) { - return this.driver.findElement(seleniumWebdriver.By.className("register-totp")).click() + return that.driver.wait(seleniumWebdriver.until.elementLocated(seleniumWebdriver.By.className("register-totp")), 4000) + .then(function () { + return that.driver.findElement(seleniumWebdriver.By.className("register-totp")).click(); + }) .then(function () { const notif = Fs.readFileSync("./notifications/notification.txt").toString(); const regexp = new RegExp(/Link: (.+)/); @@ -75,8 +87,11 @@ function CustomWorld() { }; this.useTotpToken = function (totpSecret: string) { - return this.driver.findElement(seleniumWebdriver.By.id("token")) - .sendKeys(totpSecret); + return that.driver.wait(seleniumWebdriver.until.elementLocated(seleniumWebdriver.By.className("register-totp")), 4000) + .then(function () { + return that.driver.findElement(seleniumWebdriver.By.id("token")) + .sendKeys(totpSecret); + }); }; this.registerTotpAndSignin = function (username: string, password: string) { diff --git a/test/unit/server/routes/verify/get.test.ts b/test/unit/server/routes/verify/get.test.ts index 8e2700d5e..65cb5ab32 100644 --- a/test/unit/server/routes/verify/get.test.ts +++ b/test/unit/server/routes/verify/get.test.ts @@ -24,6 +24,8 @@ describe("test authentication token verification", function () { req = ExpressMock.RequestMock(); res = ExpressMock.ResponseMock(); + req.session = {}; + AuthenticationSession.reset(req as any); req.headers = {}; req.headers.host = "secret.example.com"; const mocks = ServerVariablesMock.mock(req.app); @@ -32,7 +34,7 @@ describe("test authentication token verification", function () { mocks.accessController = accessController as any; }); - it("should be already authenticated", function (done) { + it("should be already authenticated", function () { req.session = {}; AuthenticationSession.reset(req as any); const authSession = AuthenticationSession.get(req as any); @@ -40,39 +42,34 @@ describe("test authentication token verification", function () { authSession.second_factor = true; authSession.userid = "myuser"; - res.send = sinon.spy(function () { - assert.equal(204, res.status.getCall(0).args[0]); - done(); - }); - - VerifyGet.default(req as express.Request, res as any); + return VerifyGet.default(req as express.Request, res as any) + .then(function () { + assert.equal(204, res.status.getCall(0).args[0]); + }); }); describe("given different cases of session", function () { function test_session(auth_session: AuthenticationSession.AuthenticationSession, status_code: number) { - return new BluebirdPromise(function (resolve, reject) { - req.session = {}; - req.session.auth_session = auth_session; - - res.send = sinon.spy(function () { + return VerifyGet.default(req as express.Request, res as any) + .then(function () { assert.equal(status_code, res.status.getCall(0).args[0]); - resolve(); }); - - VerifyGet.default(req as express.Request, res as any); - }); } - function test_unauthorized(auth_session: AuthenticationSession.AuthenticationSession) { + function test_non_authenticated_401(auth_session: AuthenticationSession.AuthenticationSession) { return test_session(auth_session, 401); } + function test_unauthorized_403(auth_session: AuthenticationSession.AuthenticationSession) { + return test_session(auth_session, 403); + } + function test_authorized(auth_session: AuthenticationSession.AuthenticationSession) { return test_session(auth_session, 204); } it("should not be authenticated when second factor is missing", function () { - return test_unauthorized({ + return test_non_authenticated_401({ userid: "user", first_factor: true, second_factor: false, @@ -82,7 +79,7 @@ describe("test authentication token verification", function () { }); it("should not be authenticated when first factor is missing", function () { - return test_unauthorized({ + return test_non_authenticated_401({ userid: "user", first_factor: false, second_factor: true, @@ -92,7 +89,7 @@ describe("test authentication token verification", function () { }); it("should not be authenticated when userid is missing", function () { - return test_unauthorized({ + return test_non_authenticated_401({ userid: undefined, first_factor: true, second_factor: false, @@ -102,26 +99,31 @@ describe("test authentication token verification", function () { }); it("should not be authenticated when first and second factor are missing", function () { - return test_unauthorized({ + return test_non_authenticated_401({ userid: "user", first_factor: false, second_factor: false, email: undefined, groups: [], - }); + }); }); it("should not be authenticated when session has not be initiated", function () { - return test_unauthorized(undefined); + return test_non_authenticated_401(undefined); }); it("should not be authenticated when domain is not allowed for user", function () { + const authSession = AuthenticationSession.get(req as any); + authSession.first_factor = true; + authSession.second_factor = true; + authSession.userid = "myuser"; + req.headers.host = "test.example.com"; accessController.isDomainAllowedForUser.returns(false); accessController.isDomainAllowedForUser.withArgs("test.example.com", "user", ["group1", "group2"]).returns(true); - return test_unauthorized({ + return test_unauthorized_403({ first_factor: true, second_factor: true, userid: "user",