From 6b78240d39f33f67970c816ff6ef19578fab9e16 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Wed, 1 Nov 2017 14:24:18 +0100 Subject: [PATCH] Fix endpoints redirection on errors From this commit on, api endpoints reply with a 401 error code and non api endpoints redirect to /error/40X. This commit also fixes missing restrictions on /loggedin (the "already logged in page). This was not a security issue, though. The change also makes error pages automatically redirect the user after few seconds based on the referrer or the default_redirection_url if provided in the configuration. Warning: The old /verify endpoint of the REST API has moved to /api/verify. You will need to update your nginx configuration to take this change into account. --- example/nginx/nginx.conf | 10 +- server/src/lib/ErrorReplies.ts | 15 +- server/src/lib/IdentityCheckMiddleware.ts | 73 ++++--- server/src/lib/routes/error/401/get.ts | 11 +- server/src/lib/routes/error/403/get.ts | 11 +- server/src/lib/routes/error/redirector.ts | 13 ++ server/src/lib/web_server/RestApi.ts | 25 ++- server/src/views/errors/401.pug | 7 +- server/src/views/errors/403.pug | 7 +- server/test/IdentityCheckMiddleware.test.ts | 179 +++++++++++------- server/test/mocks/express.ts | 2 + server/test/routes/errors/401/get.test.ts | 58 +++++- server/test/routes/errors/403/get.test.ts | 58 +++++- server/test/routes/firstfactor/post.test.ts | 1 + .../identity/PasswordResetHandler.test.ts | 1 + .../test/routes/password-reset/post.test.ts | 1 + .../secondfactor/totp/sign/post.test.ts | 1 + .../secondfactor/u2f/register/post.test.ts | 1 + .../u2f/register_request/get.test.ts | 1 + .../routes/secondfactor/u2f/sign/post.test.ts | 1 + .../secondfactor/u2f/sign_request/get.test.ts | 1 + server/test/routes/verify/get.test.ts | 5 +- shared/api.ts | 4 +- test/features/redirection.feature | 21 +- test/features/restrictions.feature | 1 + 25 files changed, 364 insertions(+), 144 deletions(-) create mode 100644 server/src/lib/routes/error/redirector.ts diff --git a/example/nginx/nginx.conf b/example/nginx/nginx.conf index d4e54f61f..685138bb5 100644 --- a/example/nginx/nginx.conf +++ b/example/nginx/nginx.conf @@ -86,7 +86,7 @@ http { proxy_set_header Host $http_host; proxy_set_header Content-Length ""; - proxy_pass http://authelia/verify; + proxy_pass http://authelia/api/verify; } location / { @@ -143,7 +143,7 @@ http { proxy_set_header Host $http_host; proxy_set_header Content-Length ""; - proxy_pass http://authelia/verify; + proxy_pass http://authelia/api/verify; } location / { @@ -183,7 +183,7 @@ http { proxy_set_header Host $http_host; proxy_set_header Content-Length ""; - proxy_pass http://authelia/verify; + proxy_pass http://authelia/api/verify; } location / { @@ -223,7 +223,7 @@ http { proxy_set_header Host $http_host; proxy_set_header Content-Length ""; - proxy_pass http://authelia/verify; + proxy_pass http://authelia/api/verify; } location / { @@ -263,7 +263,7 @@ http { proxy_set_header Host $http_host; proxy_set_header Content-Length ""; - proxy_pass http://authelia/verify; + proxy_pass http://authelia/api/verify; } location / { diff --git a/server/src/lib/ErrorReplies.ts b/server/src/lib/ErrorReplies.ts index cd6a4599e..dc74abbe0 100644 --- a/server/src/lib/ErrorReplies.ts +++ b/server/src/lib/ErrorReplies.ts @@ -5,10 +5,17 @@ import { IRequestLogger } from "./logging/IRequestLogger"; function replyWithError(req: express.Request, res: express.Response, code: number, logger: IRequestLogger, body?: Object): (err: Error) => void { return function (err: Error): void { - logger.error(req, "Reply with error %d: %s", code, err.message); - logger.debug(req, "%s", err.stack); - res.status(code); - res.send(body); + if (req.originalUrl.startsWith("/api/") || code == 200) { + logger.error(req, "Reply with error %d: %s", code, err.message); + logger.debug(req, "%s", err.stack); + res.status(code); + res.send(body); + } + else { + logger.error(req, "Redirect to error %d: %s", code, err.message); + logger.debug(req, "%s", err.stack); + res.redirect("/error/" + code); + } }; } diff --git a/server/src/lib/IdentityCheckMiddleware.ts b/server/src/lib/IdentityCheckMiddleware.ts index 662ddab6c..4ecbed863 100644 --- a/server/src/lib/IdentityCheckMiddleware.ts +++ b/server/src/lib/IdentityCheckMiddleware.ts @@ -7,69 +7,84 @@ import Exceptions = require("./Exceptions"); import fs = require("fs"); import ejs = require("ejs"); import { IUserDataStore } from "./storage/IUserDataStore"; -import express = require("express"); +import Express = require("express"); import ErrorReplies = require("./ErrorReplies"); import { AuthenticationSessionHandler } from "./AuthenticationSessionHandler"; import { AuthenticationSession } from "../../types/AuthenticationSession"; import { ServerVariables } from "./ServerVariables"; import Identity = require("../../types/Identity"); -import { IdentityValidationDocument } from "./storage/IdentityValidationDocument"; +import { IdentityValidationDocument } + from "./storage/IdentityValidationDocument"; const filePath = __dirname + "/../resources/email-template.ejs"; const email_template = fs.readFileSync(filePath, "utf8"); -// IdentityValidator allows user to go through a identity validation process in two steps: +// IdentityValidator allows user to go through a identity validation process +// in two steps: // - Request an operation to be performed (password reset, registration). // - Confirm operation with email. export interface IdentityValidable { challenge(): string; - preValidationInit(req: express.Request): BluebirdPromise; - postValidationInit(req: express.Request): BluebirdPromise; + preValidationInit(req: Express.Request): BluebirdPromise; + postValidationInit(req: Express.Request): BluebirdPromise; // Serves a page after identity check request - preValidationResponse(req: express.Request, res: express.Response): void; + preValidationResponse(req: Express.Request, res: Express.Response): void; // Serves the page if identity validated - postValidationResponse(req: express.Request, res: express.Response): void; + postValidationResponse(req: Express.Request, res: Express.Response): void; mailSubject(): string; } -function createAndSaveToken(userid: string, challenge: string, userDataStore: IUserDataStore) +function createAndSaveToken(userid: string, challenge: string, + userDataStore: IUserDataStore) : BluebirdPromise { const five_minutes = 4 * 60 * 1000; const token = randomstring.generate({ length: 64 }); const that = this; - return userDataStore.produceIdentityValidationToken(userid, token, challenge, five_minutes) + return userDataStore.produceIdentityValidationToken(userid, token, challenge, + five_minutes) .then(function () { return BluebirdPromise.resolve(token); }); } -function consumeToken(token: string, challenge: string, userDataStore: IUserDataStore) +function consumeToken(token: string, challenge: string, + userDataStore: IUserDataStore) : BluebirdPromise { return userDataStore.consumeIdentityValidationToken(token, challenge); } -export function register(app: express.Application, pre_validation_endpoint: string, - post_validation_endpoint: string, handler: IdentityValidable, vars: ServerVariables) { - app.get(pre_validation_endpoint, get_start_validation(handler, post_validation_endpoint, vars)); - app.get(post_validation_endpoint, get_finish_validation(handler, vars)); +export function register(app: Express.Application, + pre_validation_endpoint: string, + post_validation_endpoint: string, + handler: IdentityValidable, + vars: ServerVariables) { + + app.get(pre_validation_endpoint, + get_start_validation(handler, post_validation_endpoint, vars)); + app.get(post_validation_endpoint, + get_finish_validation(handler, vars)); } -function checkIdentityToken(req: express.Request, identityToken: string): BluebirdPromise { +function checkIdentityToken(req: Express.Request, identityToken: string) + : BluebirdPromise { if (!identityToken) - return BluebirdPromise.reject(new Exceptions.AccessDeniedError("No identity token provided")); + return BluebirdPromise.reject( + new Exceptions.AccessDeniedError("No identity token provided")); return BluebirdPromise.resolve(); } export function get_finish_validation(handler: IdentityValidable, vars: ServerVariables) - : express.RequestHandler { - return function (req: express.Request, res: express.Response): BluebirdPromise { + : Express.RequestHandler { + return function (req: Express.Request, res: Express.Response) + : BluebirdPromise { let authSession: AuthenticationSession; - const identityToken = objectPath.get(req, "query.identity_token"); + const identityToken = objectPath.get( + req, "query.identity_token"); vars.logger.debug(req, "Identity token provided is %s", identityToken); return checkIdentityToken(req, identityToken) @@ -78,7 +93,8 @@ export function get_finish_validation(handler: IdentityValidable, return handler.postValidationInit(req); }) .then(function () { - return consumeToken(identityToken, handler.challenge(), vars.userDataStore); + return consumeToken(identityToken, handler.challenge(), + vars.userDataStore); }) .then(function (doc: IdentityValidationDocument) { authSession.identity_check = { @@ -95,8 +111,9 @@ export function get_finish_validation(handler: IdentityValidable, export function get_start_validation(handler: IdentityValidable, postValidationEndpoint: string, vars: ServerVariables) - : express.RequestHandler { - return function (req: express.Request, res: express.Response): BluebirdPromise { + : Express.RequestHandler { + return function (req: Express.Request, res: Express.Response) + : BluebirdPromise { let identity: Identity.Identity; return handler.preValidationInit(req) @@ -104,20 +121,24 @@ export function get_start_validation(handler: IdentityValidable, identity = id; const email = identity.email; const userid = identity.userid; - vars.logger.info(req, "Start identity validation of user \"%s\"", userid); + vars.logger.info(req, "Start identity validation of user \"%s\"", + userid); if (!(email && userid)) return BluebirdPromise.reject(new Exceptions.IdentityError( "Missing user id or email address")); - return createAndSaveToken(userid, handler.challenge(), vars.userDataStore); + return createAndSaveToken(userid, handler.challenge(), + vars.userDataStore); }) .then(function (token: string) { const host = req.get("Host"); const link_url = util.format("https://%s%s?identity_token=%s", host, postValidationEndpoint, token); - vars.logger.info(req, "Notification sent to user \"%s\"", identity.userid); - return vars.notifier.notify(identity.email, handler.mailSubject(), link_url); + vars.logger.info(req, "Notification sent to user \"%s\"", + identity.userid); + return vars.notifier.notify(identity.email, handler.mailSubject(), + link_url); }) .then(function () { handler.preValidationResponse(req, res); diff --git a/server/src/lib/routes/error/401/get.ts b/server/src/lib/routes/error/401/get.ts index 6dce84ee2..ca4a3963d 100644 --- a/server/src/lib/routes/error/401/get.ts +++ b/server/src/lib/routes/error/401/get.ts @@ -1,8 +1,15 @@ import BluebirdPromise = require("bluebird"); import express = require("express"); +import redirector from "../redirector"; +import { ServerVariables } from "../../../ServerVariables"; -export default function (req: express.Request, res: express.Response): BluebirdPromise { - res.render("errors/401"); +export default function (vars: ServerVariables) { + return function (req: express.Request, res: express.Response): BluebirdPromise { + const redirectionUrl = redirector(req, vars); + res.render("errors/401", { + redirection_url: redirectionUrl + }); return BluebirdPromise.resolve(); + }; } diff --git a/server/src/lib/routes/error/403/get.ts b/server/src/lib/routes/error/403/get.ts index 5893ecd01..3ab0319e5 100644 --- a/server/src/lib/routes/error/403/get.ts +++ b/server/src/lib/routes/error/403/get.ts @@ -1,8 +1,15 @@ import BluebirdPromise = require("bluebird"); import express = require("express"); +import redirector from "../redirector"; +import { ServerVariables } from "../../../ServerVariables"; -export default function (req: express.Request, res: express.Response): BluebirdPromise { - res.render("errors/403"); +export default function (vars: ServerVariables) { + return function (req: express.Request, res: express.Response): BluebirdPromise { + const redirectionUrl = redirector(req, vars); + res.render("errors/403", { + redirection_url: redirectionUrl + }); return BluebirdPromise.resolve(); + }; } \ No newline at end of file diff --git a/server/src/lib/routes/error/redirector.ts b/server/src/lib/routes/error/redirector.ts new file mode 100644 index 000000000..b1a3ccc11 --- /dev/null +++ b/server/src/lib/routes/error/redirector.ts @@ -0,0 +1,13 @@ +import Express = require("express"); +import { ServerVariables } from "../../ServerVariables"; + +export default function (req: Express.Request, vars: ServerVariables): string { + let redirectionUrl: string; + + if (req.headers && req.headers["referer"]) + redirectionUrl = "" + req.headers["referer"]; + else if (vars.config.default_redirection_url) + redirectionUrl = vars.config.default_redirection_url; + + return redirectionUrl; +} \ No newline at end of file diff --git a/server/src/lib/web_server/RestApi.ts b/server/src/lib/web_server/RestApi.ts index 833b9591c..ed2319971 100644 --- a/server/src/lib/web_server/RestApi.ts +++ b/server/src/lib/web_server/RestApi.ts @@ -101,17 +101,21 @@ function setupU2f(app: Express.Application, vars: ServerVariables) { } function setupResetPassword(app: Express.Application, vars: ServerVariables) { - IdentityCheckMiddleware.register(app, Endpoints.RESET_PASSWORD_IDENTITY_START_GET, + IdentityCheckMiddleware.register(app, + Endpoints.RESET_PASSWORD_IDENTITY_START_GET, Endpoints.RESET_PASSWORD_IDENTITY_FINISH_GET, - new ResetPasswordIdentityHandler(vars.logger, vars.ldapEmailsRetriever), vars); + new ResetPasswordIdentityHandler(vars.logger, vars.ldapEmailsRetriever), + vars); - app.get(Endpoints.RESET_PASSWORD_REQUEST_GET, ResetPasswordRequestPost.default); - app.post(Endpoints.RESET_PASSWORD_FORM_POST, ResetPasswordFormPost.default(vars)); + app.get(Endpoints.RESET_PASSWORD_REQUEST_GET, + ResetPasswordRequestPost.default); + app.post(Endpoints.RESET_PASSWORD_FORM_POST, + ResetPasswordFormPost.default(vars)); } -function setupErrors(app: Express.Application) { - app.get(Endpoints.ERROR_401_GET, Error401Get.default); - app.get(Endpoints.ERROR_403_GET, Error403Get.default); +function setupErrors(app: Express.Application, vars: ServerVariables) { + app.get(Endpoints.ERROR_401_GET, Error401Get.default(vars)); + app.get(Endpoints.ERROR_403_GET, Error403Get.default(vars)); app.get(Endpoints.ERROR_404_GET, Error404Get.default); } @@ -124,6 +128,7 @@ export class RestApi { vars.config.authentication_methods), RequireValidatedFirstFactor.middleware(vars.logger), SecondFactorGet.default(vars)); + app.get(Endpoints.LOGOUT_GET, LogoutGet.default); app.get(Endpoints.VERIFY_GET, VerifyGet.default(vars)); @@ -132,8 +137,10 @@ export class RestApi { setupTotp(app, vars); setupU2f(app, vars); setupResetPassword(app, vars); - setupErrors(app); + setupErrors(app, vars); - app.get(Endpoints.LOGGED_IN, LoggedIn.default(vars)); + app.get(Endpoints.LOGGED_IN, + RequireValidatedFirstFactor.middleware(vars.logger), + LoggedIn.default(vars)); } } diff --git a/server/src/views/errors/401.pug b/server/src/views/errors/401.pug index 052b4dc57..b7a222ad0 100644 --- a/server/src/views/errors/401.pug +++ b/server/src/views/errors/401.pug @@ -8,4 +8,9 @@ block form-header block content img(class="header-img" src="/img/warning.png" alt="warning") - p Please log in to access this resource. + if redirection_url + p You are not authorized to access this resource.

+ | Please click here if you are not + | redirected in few seconds. + else + p You are not authorized to access this resource. \ No newline at end of file diff --git a/server/src/views/errors/403.pug b/server/src/views/errors/403.pug index b0b77b606..f4b5ca8a2 100644 --- a/server/src/views/errors/403.pug +++ b/server/src/views/errors/403.pug @@ -8,4 +8,9 @@ block form-header block content img(class="header-img" src="/img/warning.png" alt="warning") - p You are not authorized to access this resource. + if redirection_url + p You don't have enough privileges to access this resource.

+ | Please click here if you are not + | redirected in few seconds. + else + p You don't have enough privileges to access this resource. diff --git a/server/test/IdentityCheckMiddleware.test.ts b/server/test/IdentityCheckMiddleware.test.ts index 1f43a6542..9ed4f47de 100644 --- a/server/test/IdentityCheckMiddleware.test.ts +++ b/server/test/IdentityCheckMiddleware.test.ts @@ -1,7 +1,8 @@ import sinon = require("sinon"); import IdentityValidator = require("../src/lib/IdentityCheckMiddleware"); -import { AuthenticationSessionHandler } from "../src/lib/AuthenticationSessionHandler"; +import { AuthenticationSessionHandler } + from "../src/lib/AuthenticationSessionHandler"; import { AuthenticationSession } from "../types/AuthenticationSession"; import { UserDataStore } from "../src/lib/storage/UserDataStore"; import exceptions = require("../src/lib/Exceptions"); @@ -13,7 +14,8 @@ import ExpressMock = require("./mocks/express"); import NotifierMock = require("./mocks/Notifier"); import IdentityValidatorMock = require("./mocks/IdentityValidator"); import { RequestLoggerStub } from "./mocks/RequestLoggerStub"; -import { ServerVariablesMock, ServerVariablesMockBuilder } from "./mocks/ServerVariablesMockBuilder"; +import { ServerVariablesMock, ServerVariablesMockBuilder } + from "./mocks/ServerVariablesMockBuilder"; describe("test identity check process", function () { @@ -36,17 +38,18 @@ describe("test identity check process", function () { identityValidable = IdentityValidatorMock.IdentityValidableMock(); - req.headers = {}; - req.session = {}; + req.originalUrl = "/non-api/xxx"; req.session = {}; req.query = {}; req.app = {}; mocks.notifier.notifyStub.returns(BluebirdPromise.resolve()); - mocks.userDataStore.produceIdentityValidationTokenStub.returns(Promise.resolve()); - mocks.userDataStore.consumeIdentityValidationTokenStub.returns(Promise.resolve({ userId: "user" })); + mocks.userDataStore.produceIdentityValidationTokenStub + .returns(Promise.resolve()); + mocks.userDataStore.consumeIdentityValidationTokenStub + .returns(Promise.resolve({ userId: "user" })); app = express(); app_get = sinon.stub(app, "get"); @@ -58,112 +61,142 @@ describe("test identity check process", function () { app_post.restore(); }); - describe("test start GET", test_start_get_handler); - describe("test finish GET", test_finish_get_handler); + describe("test start GET", function () { + it("should redirect to error 401 if pre validation initialization \ +throws a first factor error", function () { + identityValidable.preValidationInit.returns(BluebirdPromise.reject( + new exceptions.FirstFactorValidationError( + "Error during prevalidation"))); + const callback = IdentityValidator.get_start_validation( + identityValidable, "/endpoint", vars); - function test_start_get_handler() { - it("should send 401 if pre validation initialization throws a first factor error", function () { - identityValidable.preValidationInit.returns(BluebirdPromise.reject(new exceptions.FirstFactorValidationError("Error during prevalidation"))); - const callback = IdentityValidator.get_start_validation(identityValidable, "/endpoint", vars); - - return callback(req as any, res as any, undefined) - .then(function () { return BluebirdPromise.reject("Should fail"); }) - .catch(function () { - Assert.equal(res.status.getCall(0).args[0], 401); - }); - }); + return callback(req as any, res as any, undefined) + .then(function () { return BluebirdPromise.reject("Should fail"); }) + .catch(function () { + Assert(res.redirect.calledWith("/error/401")); + }); + }); it("should send 401 if email is missing in provided identity", function () { const identity = { userid: "abc" }; - identityValidable.preValidationInit.returns(BluebirdPromise.resolve(identity)); - const callback = IdentityValidator.get_start_validation(identityValidable, "/endpoint", vars); + identityValidable.preValidationInit + .returns(BluebirdPromise.resolve(identity)); + const callback = IdentityValidator + .get_start_validation(identityValidable, "/endpoint", vars); return callback(req as any, res as any, undefined) - .then(function () { return BluebirdPromise.reject("Should fail"); }) + .then(function () { + return BluebirdPromise.reject("Should fail"); + }) .catch(function () { - Assert.equal(res.status.getCall(0).args[0], 401); + Assert(res.redirect.calledWith("/error/401")); }); }); - it("should send 401 if userid is missing in provided identity", function () { - const endpoint = "/protected"; - const identity = { email: "abc@example.com" }; + it("should send 401 if userid is missing in provided identity", + function () { + const endpoint = "/protected"; + const identity = { email: "abc@example.com" }; - identityValidable.preValidationInit.returns(BluebirdPromise.resolve(identity)); - const callback = IdentityValidator.get_start_validation(identityValidable, "/endpoint", vars); + identityValidable.preValidationInit + .returns(BluebirdPromise.resolve(identity)); + const callback = IdentityValidator + .get_start_validation(identityValidable, "/endpoint", vars); - return callback(req as any, res as any, undefined) - .then(function () { return BluebirdPromise.reject(new Error("It should fail")); }) - .catch(function (err: Error) { - Assert.equal(res.status.getCall(0).args[0], 401); - return BluebirdPromise.resolve(); - }); - }); + return callback(req as any, res as any, undefined) + .then(function () { + return BluebirdPromise.reject(new Error("It should fail")); + }) + .catch(function (err: Error) { + Assert(res.redirect.calledWith("/error/401")); + }); + }); it("should issue a token, send an email and return 204", function () { const endpoint = "/protected"; const identity = { userid: "user", email: "abc@example.com" }; req.get = sinon.stub().withArgs("Host").returns("localhost"); - identityValidable.preValidationInit.returns(BluebirdPromise.resolve(identity)); - const callback = IdentityValidator.get_start_validation(identityValidable, "/finish_endpoint", vars); + identityValidable.preValidationInit + .returns(BluebirdPromise.resolve(identity)); + const callback = IdentityValidator + .get_start_validation(identityValidable, "/finish_endpoint", vars); return callback(req as any, res as any, undefined) .then(function () { Assert(mocks.notifier.notifyStub.calledOnce); - Assert(mocks.userDataStore.produceIdentityValidationTokenStub.calledOnce); - Assert.equal(mocks.userDataStore.produceIdentityValidationTokenStub.getCall(0).args[0], "user"); - Assert.equal(mocks.userDataStore.produceIdentityValidationTokenStub.getCall(0).args[3], 240000); + Assert(mocks.userDataStore.produceIdentityValidationTokenStub + .calledOnce); + Assert.equal(mocks.userDataStore.produceIdentityValidationTokenStub + .getCall(0).args[0], "user"); + Assert.equal(mocks.userDataStore.produceIdentityValidationTokenStub + .getCall(0).args[3], 240000); }); }); - } + }); - function test_finish_get_handler() { + + + describe("test finish GET", function () { it("should send 401 if no identity_token is provided", function () { - const callback = IdentityValidator.get_finish_validation(identityValidable, vars); + const callback = IdentityValidator + .get_finish_validation(identityValidable, vars); return callback(req as any, res as any, undefined) - .then(function () { return BluebirdPromise.reject("Should fail"); }) + .then(function () { + return BluebirdPromise.reject("Should fail"); + }) .catch(function () { - Assert.equal(res.status.getCall(0).args[0], 401); + Assert(res.redirect.calledWith("/error/401")); }); }); - it("should call postValidation if identity_token is provided and still valid", function () { - req.query.identity_token = "token"; + it("should call postValidation if identity_token is provided and still \ +valid", function () { + req.query.identity_token = "token"; - const callback = IdentityValidator.get_finish_validation(identityValidable, vars); - return callback(req as any, res as any, undefined); - }); + const callback = IdentityValidator + .get_finish_validation(identityValidable, vars); + return callback(req as any, res as any, undefined); + }); - it("should return 401 if identity_token is provided but invalid", function () { - req.query.identity_token = "token"; + it("should return 401 if identity_token is provided but invalid", + function () { + req.query.identity_token = "token"; - mocks.userDataStore.consumeIdentityValidationTokenStub.returns(BluebirdPromise.reject(new Error("Invalid token"))); + mocks.userDataStore.consumeIdentityValidationTokenStub + .returns(BluebirdPromise.reject(new Error("Invalid token"))); - const callback = IdentityValidator.get_finish_validation(identityValidable, vars); - return callback(req as any, res as any, undefined) - .then(function () { return BluebirdPromise.reject("Should fail"); }) - .catch(function () { - Assert.equal(res.status.getCall(0).args[0], 401); - }); - }); + const callback = IdentityValidator + .get_finish_validation(identityValidable, vars); + return callback(req as any, res as any, undefined) + .then(function () { + return BluebirdPromise.reject("Should fail"); + }) + .catch(function () { + Assert(res.redirect.calledWith("/error/401")); + }); + }); - it("should set the identity_check session object even if session does not exist yet", function () { - req.query.identity_token = "token"; + it("should set the identity_check session object even if session does \ +not exist yet", function () { + req.query.identity_token = "token"; - req.session = {}; - const authSession: AuthenticationSession = AuthenticationSessionHandler.get(req as any, vars.logger); - const callback = IdentityValidator.get_finish_validation(identityValidable, vars); + req.session = {}; + const authSession = + AuthenticationSessionHandler.get(req as any, vars.logger); + const callback = IdentityValidator + .get_finish_validation(identityValidable, vars); - return callback(req as any, res as any, undefined) - .then(function () { return BluebirdPromise.reject("Should fail"); }) - .catch(function () { - Assert.equal(authSession.identity_check.userid, "user"); - return BluebirdPromise.resolve(); - }); - }); - } + return callback(req as any, res as any, undefined) + .then(function () { + return BluebirdPromise.reject("Should fail"); + }) + .catch(function () { + Assert.equal(authSession.identity_check.userid, "user"); + }); + }); + }); }); diff --git a/server/test/mocks/express.ts b/server/test/mocks/express.ts index c60240fa5..48f15d7e1 100644 --- a/server/test/mocks/express.ts +++ b/server/test/mocks/express.ts @@ -9,6 +9,7 @@ export interface RequestMock { headers?: any; get?: any; query?: any; + originalUrl: string; } export interface ResponseMock { @@ -51,6 +52,7 @@ export interface ResponseMock { export function RequestMock(): RequestMock { return { + originalUrl: "/non-api/xxx", app: { get: sinon.stub() }, diff --git a/server/test/routes/errors/401/get.test.ts b/server/test/routes/errors/401/get.test.ts index 721622856..36766f172 100644 --- a/server/test/routes/errors/401/get.test.ts +++ b/server/test/routes/errors/401/get.test.ts @@ -2,18 +2,60 @@ import Sinon = require("sinon"); import Express = require("express"); import Assert = require("assert"); import Get401 from "../../../../src/lib/routes/error/401/get"; +import { ServerVariables } from "../../../../src/lib/ServerVariables"; +import { ServerVariablesMockBuilder, ServerVariablesMock } + from "../../../mocks/ServerVariablesMockBuilder"; describe("Server error 401", function () { - it("should render the page", function () { - const req = {} as Express.Request; - const res = { - render: Sinon.stub() - }; + let vars: ServerVariables; + let mocks: ServerVariablesMock; + let req: any; + let res: any; + let renderSpy: Sinon.SinonSpy; - return Get401(req, res as any) + beforeEach(function () { + const s = ServerVariablesMockBuilder.build(); + vars = s.variables; + mocks = s.mocks; + + renderSpy = Sinon.spy(); + req = { + headers: {} + }; + res = { + render: renderSpy + }; + }); + + it("should set redirection url to the default redirection url", function () { + vars.config.default_redirection_url = "http://default-redirection"; + return Get401(vars)(req, res as any) .then(function () { - Assert(res.render.calledOnce); - Assert(res.render.calledWith("errors/401")); + Assert(renderSpy.calledOnce); + Assert(renderSpy.calledWithExactly("errors/401", { + redirection_url: "http://default-redirection" + })); + }); + }); + + it("should set redirection url to the referer", function () { + req.headers["referer"] = "http://redirection"; + return Get401(vars)(req, res as any) + .then(function () { + Assert(renderSpy.calledOnce); + Assert(renderSpy.calledWithExactly("errors/401", { + redirection_url: "http://redirection" + })); + }); + }); + + it("should render without redirecting the user", function () { + return Get401(vars)(req, res as any) + .then(function () { + Assert(renderSpy.calledOnce); + Assert(renderSpy.calledWithExactly("errors/401", { + redirection_url: undefined + })); }); }); }); \ No newline at end of file diff --git a/server/test/routes/errors/403/get.test.ts b/server/test/routes/errors/403/get.test.ts index 09f141e3f..376acb6a9 100644 --- a/server/test/routes/errors/403/get.test.ts +++ b/server/test/routes/errors/403/get.test.ts @@ -2,18 +2,60 @@ import Sinon = require("sinon"); import Express = require("express"); import Assert = require("assert"); import Get403 from "../../../../src/lib/routes/error/403/get"; +import { ServerVariables } from "../../../../src/lib/ServerVariables"; +import { ServerVariablesMockBuilder, ServerVariablesMock } + from "../../../mocks/ServerVariablesMockBuilder"; describe("Server error 403", function () { - it("should render the page", function () { - const req = {} as Express.Request; - const res = { - render: Sinon.stub() - }; + let vars: ServerVariables; + let mocks: ServerVariablesMock; + let req: any; + let res: any; + let renderSpy: Sinon.SinonSpy; - return Get403(req, res as any) + beforeEach(function () { + const s = ServerVariablesMockBuilder.build(); + vars = s.variables; + mocks = s.mocks; + + renderSpy = Sinon.spy(); + req = { + headers: {} + }; + res = { + render: renderSpy + }; + }); + + it("should set redirection url to the default redirection url", function () { + vars.config.default_redirection_url = "http://default-redirection"; + return Get403(vars)(req, res as any) .then(function () { - Assert(res.render.calledOnce); - Assert(res.render.calledWith("errors/403")); + Assert(renderSpy.calledOnce); + Assert(renderSpy.calledWithExactly("errors/403", { + redirection_url: "http://default-redirection" + })); + }); + }); + + it("should set redirection url to the referer", function () { + req.headers["referer"] = "http://redirection"; + return Get403(vars)(req, res as any) + .then(function () { + Assert(renderSpy.calledOnce); + Assert(renderSpy.calledWithExactly("errors/403", { + redirection_url: "http://redirection" + })); + }); + }); + + it("should render without redirecting the user", function () { + return Get403(vars)(req, res as any) + .then(function () { + Assert(renderSpy.calledOnce); + Assert(renderSpy.calledWithExactly("errors/403", { + redirection_url: undefined + })); }); }); }); \ No newline at end of file diff --git a/server/test/routes/firstfactor/post.test.ts b/server/test/routes/firstfactor/post.test.ts index 55f57ef49..6f8ba43cb 100644 --- a/server/test/routes/firstfactor/post.test.ts +++ b/server/test/routes/firstfactor/post.test.ts @@ -34,6 +34,7 @@ describe("test the first factor validation route", function () { mocks.regulator.markStub.returns(BluebirdPromise.resolve()); req = { + originalUrl: "/api/firstfactor", body: { username: "username", password: "password" diff --git a/server/test/routes/password-reset/identity/PasswordResetHandler.test.ts b/server/test/routes/password-reset/identity/PasswordResetHandler.test.ts index e2bd14e10..eeaf693fa 100644 --- a/server/test/routes/password-reset/identity/PasswordResetHandler.test.ts +++ b/server/test/routes/password-reset/identity/PasswordResetHandler.test.ts @@ -18,6 +18,7 @@ describe("test reset password identity check", function () { beforeEach(function () { req = { + originalUrl: "/non-api/xxx", query: { userid: "user" }, diff --git a/server/test/routes/password-reset/post.test.ts b/server/test/routes/password-reset/post.test.ts index 31055b44c..9b86e7a4e 100644 --- a/server/test/routes/password-reset/post.test.ts +++ b/server/test/routes/password-reset/post.test.ts @@ -20,6 +20,7 @@ describe("test reset password route", function () { beforeEach(function () { req = { + originalUrl: "/api/password-reset", body: { userid: "user" }, diff --git a/server/test/routes/secondfactor/totp/sign/post.test.ts b/server/test/routes/secondfactor/totp/sign/post.test.ts index d6b3d9ce5..ddf1d26cf 100644 --- a/server/test/routes/secondfactor/totp/sign/post.test.ts +++ b/server/test/routes/secondfactor/totp/sign/post.test.ts @@ -25,6 +25,7 @@ describe("test totp route", function () { mocks = s.mocks; const app_get = Sinon.stub(); req = { + originalUrl: "/api/totp-register", app: {}, body: { token: "abc" diff --git a/server/test/routes/secondfactor/u2f/register/post.test.ts b/server/test/routes/secondfactor/u2f/register/post.test.ts index 88fc5ad4d..59ee8a761 100644 --- a/server/test/routes/secondfactor/u2f/register/post.test.ts +++ b/server/test/routes/secondfactor/u2f/register/post.test.ts @@ -20,6 +20,7 @@ describe("test u2f routes: register", function () { beforeEach(function () { req = ExpressMock.RequestMock(); + req.originalUrl = "/api/xxxx"; req.app = {}; req.session = { auth: { diff --git a/server/test/routes/secondfactor/u2f/register_request/get.test.ts b/server/test/routes/secondfactor/u2f/register_request/get.test.ts index c2f5dcb49..9617e24bb 100644 --- a/server/test/routes/secondfactor/u2f/register_request/get.test.ts +++ b/server/test/routes/secondfactor/u2f/register_request/get.test.ts @@ -16,6 +16,7 @@ describe("test u2f routes: register_request", function () { beforeEach(function () { req = ExpressMock.RequestMock(); + req.originalUrl = "/api/xxxx"; req.app = {}; req.session = { auth: { diff --git a/server/test/routes/secondfactor/u2f/sign/post.test.ts b/server/test/routes/secondfactor/u2f/sign/post.test.ts index e84e30a79..942d9dbfa 100644 --- a/server/test/routes/secondfactor/u2f/sign/post.test.ts +++ b/server/test/routes/secondfactor/u2f/sign/post.test.ts @@ -20,6 +20,7 @@ describe("test u2f routes: sign", function () { beforeEach(function () { req = ExpressMock.RequestMock(); req.app = {}; + req.originalUrl = "/api/xxxx"; const s = ServerVariablesMockBuilder.build(); mocks = s.mocks; diff --git a/server/test/routes/secondfactor/u2f/sign_request/get.test.ts b/server/test/routes/secondfactor/u2f/sign_request/get.test.ts index 0bf2ab190..49e7cf11e 100644 --- a/server/test/routes/secondfactor/u2f/sign_request/get.test.ts +++ b/server/test/routes/secondfactor/u2f/sign_request/get.test.ts @@ -20,6 +20,7 @@ describe("test u2f routes: sign_request", function () { beforeEach(function () { req = ExpressMock.RequestMock(); + req.originalUrl = "/api/xxxx"; req.app = {}; req.session = { auth: { diff --git a/server/test/routes/verify/get.test.ts b/server/test/routes/verify/get.test.ts index f248ab3b7..b4684e30e 100644 --- a/server/test/routes/verify/get.test.ts +++ b/server/test/routes/verify/get.test.ts @@ -12,7 +12,7 @@ import ExpressMock = require("../../mocks/express"); import { ServerVariables } from "../../../src/lib/ServerVariables"; import { ServerVariablesMockBuilder, ServerVariablesMock } from "../../mocks/ServerVariablesMockBuilder"; -describe("test /verify endpoint", function () { +describe("test /api/verify endpoint", function () { let req: ExpressMock.RequestMock; let res: ExpressMock.ResponseMock; let mocks: ServerVariablesMock; @@ -22,6 +22,7 @@ describe("test /verify endpoint", function () { beforeEach(function () { req = ExpressMock.RequestMock(); res = ExpressMock.ResponseMock(); + req.originalUrl = "/api/xxxx"; req.query = { redirect: "http://redirect.url" }; @@ -174,7 +175,7 @@ describe("test /verify endpoint", function () { }); describe("inactivity period", function () { - it("should update last inactivity period on requests on /verify", function () { + it("should update last inactivity period on requests on /api/verify", function () { mocks.config.session.inactivity = 200000; mocks.accessController.isAccessAllowedMock.returns(true); const currentTime = new Date().getTime() - 1000; diff --git a/shared/api.ts b/shared/api.ts index 32ba220a7..ef257f2c8 100644 --- a/shared/api.ts +++ b/shared/api.ts @@ -264,7 +264,7 @@ export const FIRST_FACTOR_GET = "/"; export const SECOND_FACTOR_GET = "/secondfactor"; /** - * @api {get} /verify Verify user authentication + * @api {get} /api/verify Verify user authentication * @apiName VerifyAuthentication * @apiGroup Verification * @apiVersion 1.0.0 @@ -279,7 +279,7 @@ export const SECOND_FACTOR_GET = "/secondfactor"; * are set. Remote-User contains the user id of the currently logged in user and Remote-Groups * a comma separated list of assigned groups. */ -export const VERIFY_GET = "/verify"; +export const VERIFY_GET = "/api/verify"; /** * @api {get} /logout Serves logout page diff --git a/test/features/redirection.feature b/test/features/redirection.feature index 19890a464..d6fafe618 100644 --- a/test/features/redirection.feature +++ b/test/features/redirection.feature @@ -48,4 +48,23 @@ Feature: User is correctly redirected And I login with user "john" and password "password" And I use "REGISTERED" as TOTP token handle And I click on "Sign in" - Then I'm redirected to "https://home.test.local:8080/" \ No newline at end of file + Then I'm redirected to "https://home.test.local:8080/" + + + Scenario: User is redirected when hitting an error 401 + When I visit "https://auth.test.local:8080/secondfactor/u2f/identity/finish" + Then I'm redirected to "https://auth.test.local:8080/error/401" + And I sleep for 5 seconds + And I'm redirected to "https://home.test.local:8080/" + + @need-registered-user-harry + Scenario: User is redirected when hitting an error 403 + When I visit "https://auth.test.local:8080" + And I login with user "harry" and password "password" + And I use "REGISTERED" as TOTP token handle + And I click on "Sign in" + And I'm redirected to "https://home.test.local:8080/" + When I visit "https://admin.test.local:8080/secret.html" + Then I'm redirected to "https://auth.test.local:8080/error/403" + And I sleep for 5 seconds + And I'm redirected to "https://home.test.local:8080/" \ No newline at end of file diff --git a/test/features/restrictions.feature b/test/features/restrictions.feature index 4ff7a1362..ab72b6d82 100644 --- a/test/features/restrictions.feature +++ b/test/features/restrictions.feature @@ -8,6 +8,7 @@ Feature: Non authenticated users have no access to certain pages | https://auth.test.local:8080/secondfactor/u2f/identity/finish | 401 | GET | | https://auth.test.local:8080/secondfactor/totp/identity/start | 401 | GET | | https://auth.test.local:8080/secondfactor/totp/identity/finish | 401 | GET | + | https://auth.test.local:8080/loggedin | 401 | GET | | https://auth.test.local:8080/api/totp | 401 | POST | | https://auth.test.local:8080/api/u2f/sign_request | 401 | GET | | https://auth.test.local:8080/api/u2f/sign | 401 | POST |