From 267cf2921d5b37d71e7707cf06d302dba2d8bf61 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Sun, 8 Oct 2017 15:34:58 +0200 Subject: [PATCH 1/3] Adapt ACL configuration to make it more flexible Basically, the ACL configuration was very static and it was not allowed to remove 'any', 'groups', 'users'. The application crashed when those keys did not exist. After this fix, every key is optional and replaced by a default value for the app configuration to be complete and used by Authelia. Later, a configuration validator will be implemented to detect issues with configuration at startup. --- .../lib/configuration/ConfigurationAdapter.ts | 4 +- .../lib/configuration/adapters/ACLAdapter.ts | 42 +++++ .../access_control/AccessController.test.ts | 8 +- .../ConfigurationAdapter.test.ts | 85 ++++++--- .../configuration/adapters/ACLAdapter.test.ts | 162 ++++++++++++++++++ 5 files changed, 269 insertions(+), 32 deletions(-) create mode 100644 server/src/lib/configuration/adapters/ACLAdapter.ts create mode 100644 server/test/configuration/adapters/ACLAdapter.test.ts diff --git a/server/src/lib/configuration/ConfigurationAdapter.ts b/server/src/lib/configuration/ConfigurationAdapter.ts index 8931401e..56afdd8a 100644 --- a/server/src/lib/configuration/ConfigurationAdapter.ts +++ b/server/src/lib/configuration/ConfigurationAdapter.ts @@ -7,10 +7,10 @@ import { UserLdapConfiguration } from "./Configuration"; import Util = require("util"); +import { ACLAdapter } from "./adapters/ACLAdapter"; const LDAP_URL_ENV_VARIABLE = "LDAP_URL"; - function get_optional(config: object, path: string, default_value: T): T { let entry = default_value; if (ObjectPath.has(config, path)) { @@ -80,7 +80,7 @@ function adaptFromUserConfiguration(userConfiguration: UserConfiguration): AppCo }, logs_level: get_optional(userConfiguration, "logs_level", "info"), notifier: ObjectPath.get(userConfiguration, "notifier"), - access_control: ObjectPath.get(userConfiguration, "access_control"), + access_control: ACLAdapter.adapt(userConfiguration.access_control), regulation: userConfiguration.regulation }; } diff --git a/server/src/lib/configuration/adapters/ACLAdapter.ts b/server/src/lib/configuration/adapters/ACLAdapter.ts new file mode 100644 index 00000000..53e0801c --- /dev/null +++ b/server/src/lib/configuration/adapters/ACLAdapter.ts @@ -0,0 +1,42 @@ +import { ACLConfiguration } from "../Configuration"; + +function clone(obj: any): any { + return JSON.parse(JSON.stringify(obj)); +} + +const DEFAULT_POLICY = "deny"; + +function adaptDefaultPolicy(configuration: ACLConfiguration) { + if (!configuration.default_policy) + configuration.default_policy = DEFAULT_POLICY; + if (configuration.default_policy != "deny" && configuration.default_policy != "allow") + configuration.default_policy = DEFAULT_POLICY; +} + +function adaptAny(configuration: ACLConfiguration) { + if (!configuration.any || !(configuration.any.constructor === Array)) + configuration.any = []; +} + +function adaptGroups(configuration: ACLConfiguration) { + if (!configuration.groups || !(configuration.groups.constructor === Object)) + configuration.groups = {}; +} + +function adaptUsers(configuration: ACLConfiguration) { + if (!configuration.users || !(configuration.users.constructor === Object)) + configuration.users = {}; +} + +export class ACLAdapter { + static adapt(configuration: ACLConfiguration): ACLConfiguration { + if (!configuration) return; + + const newConfiguration: ACLConfiguration = clone(configuration); + adaptDefaultPolicy(newConfiguration); + adaptAny(newConfiguration); + adaptGroups(newConfiguration); + adaptUsers(newConfiguration); + return newConfiguration; + } +} \ No newline at end of file diff --git a/server/test/access_control/AccessController.test.ts b/server/test/access_control/AccessController.test.ts index aca3ee3e..d0df7a90 100644 --- a/server/test/access_control/AccessController.test.ts +++ b/server/test/access_control/AccessController.test.ts @@ -173,8 +173,8 @@ describe("test access control manager", function () { }); }); - describe("check all rules", function () { - it("should control access when all rules are defined", function () { + describe("check any rules", function () { + it("should control access when any rules are defined", function () { configuration.any = [{ domain: "home.example.com", policy: "allow", @@ -307,7 +307,7 @@ describe("test access control manager", function () { Assert(!accessController.isAccessAllowed("home.example.com", "/dev/bob", "john", ["dev"])); }); - it("should control access when allowed at all level and denied at user level", function () { + it("should control access when allowed at 'any' level and denied at user level", function () { configuration.any = [{ domain: "home.example.com", policy: "allow", @@ -323,7 +323,7 @@ describe("test access control manager", function () { Assert(!accessController.isAccessAllowed("home.example.com", "/dev/bob", "john", ["dev"])); }); - it("should control access when allowed at all level and denied at group level", function () { + it("should control access when allowed at 'any' level and denied at group level", function () { configuration.any = [{ domain: "home.example.com", policy: "allow", diff --git a/server/test/configuration/ConfigurationAdapter.test.ts b/server/test/configuration/ConfigurationAdapter.test.ts index 100a5e18..62f6f6e3 100644 --- a/server/test/configuration/ConfigurationAdapter.test.ts +++ b/server/test/configuration/ConfigurationAdapter.test.ts @@ -43,18 +43,20 @@ describe("test config adapter", function () { return yaml_config; } - it("should read the port from the yaml file", function () { - const yaml_config = build_yaml_config(); - yaml_config.port = 7070; - const config = ConfigurationAdapter.adapt(yaml_config); - Assert.equal(config.port, 7070); - }); + describe("port", function () { + it("should read the port from the yaml file", function () { + const yaml_config = build_yaml_config(); + yaml_config.port = 7070; + const config = ConfigurationAdapter.adapt(yaml_config); + Assert.equal(config.port, 7070); + }); - it("should default the port to 8080 if not provided", function () { - const yaml_config = build_yaml_config(); - delete yaml_config.port; - const config = ConfigurationAdapter.adapt(yaml_config); - Assert.equal(config.port, 8080); + it("should default the port to 8080 if not provided", function () { + const yaml_config = build_yaml_config(); + delete yaml_config.port; + const config = ConfigurationAdapter.adapt(yaml_config); + Assert.equal(config.port, 8080); + }); }); it("should get the session attributes", function () { @@ -94,20 +96,51 @@ describe("test config adapter", function () { }); }); - it("should get the access_control config", function () { - const yaml_config = build_yaml_config(); - yaml_config.access_control = { - default_policy: "deny", - any: [], - users: {}, - groups: {} - }; - const config = ConfigurationAdapter.adapt(yaml_config); - Assert.deepEqual(config.access_control, { - default_policy: "deny", - any: [], - users: {}, - groups: {} - } as ACLConfiguration); + describe("access_control", function() { + it("should adapt access_control when it is already ok", function () { + const yaml_config = build_yaml_config(); + yaml_config.access_control = { + default_policy: "deny", + any: [{ + domain: "public.example.com", + policy: "allow" + }], + users: { + "user": [{ + domain: "www.example.com", + policy: "allow" + }] + }, + groups: {} + }; + const config = ConfigurationAdapter.adapt(yaml_config); + Assert.deepEqual(config.access_control, { + default_policy: "deny", + any: [{ + domain: "public.example.com", + policy: "allow" + }], + users: { + "user": [{ + domain: "www.example.com", + policy: "allow" + }] + }, + groups: {} + } as ACLConfiguration); + }); + + + it("should adapt access_control when it is empty", function () { + const yaml_config = build_yaml_config(); + yaml_config.access_control = {} as any; + const config = ConfigurationAdapter.adapt(yaml_config); + Assert.deepEqual(config.access_control, { + default_policy: "deny", + any: [], + users: {}, + groups: {} + } as ACLConfiguration); + }); }); }); diff --git a/server/test/configuration/adapters/ACLAdapter.test.ts b/server/test/configuration/adapters/ACLAdapter.test.ts new file mode 100644 index 00000000..01bb3258 --- /dev/null +++ b/server/test/configuration/adapters/ACLAdapter.test.ts @@ -0,0 +1,162 @@ +import { ACLAdapter } from "../../../src/lib/configuration/adapters/ACLAdapter"; +import Assert = require("assert"); + +describe("test ACL configuration adapter", function () { + + describe("bad default_policy", function () { + it("should adapt a configuration missing default_policy", function () { + const userConfiguration: any = { + any: [], + groups: {}, + users: {} + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + + it("should adapt a configuration with bad default_policy value", function () { + const userConfiguration: any = { + default_policy: "anything", // it should be 'allow' or 'deny' + any: [], + groups: {}, + users: {} + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + + it("should adapt a configuration with bad default_policy type", function () { + const userConfiguration: any = { + default_policy: {}, // it should be 'allow' or 'deny' + any: [], + groups: {}, + users: {} + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + }); + + describe("bad any", function () { + it("should adapt a configuration missing any key", function () { + const userConfiguration: any = { + default_policy: "deny", + groups: {}, + users: {} + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + + it("should adapt a configuration with any not being an array", function () { + const userConfiguration: any = { + default_policy: "deny", + any: "abc", + groups: {}, + users: {} + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + }); + + describe("bad groups", function () { + it("should adapt a configuration missing groups key", function () { + const userConfiguration: any = { + default_policy: "deny", + any: [], + users: {} + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + + it("should adapt configuration with groups being of wrong type", function () { + const userConfiguration: any = { + default_policy: "deny", + any: [], + groups: [], + users: {} + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + }); + + describe("bad users", function () { + it("should adapt a configuration missing users key", function () { + const userConfiguration: any = { + default_policy: "deny", + any: [], + groups: {} + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + + it("should adapt a configuration with users being of wrong type", function () { + const userConfiguration: any = { + default_policy: "deny", + any: [], + groups: {}, + users: [] + }; + + const appConfiguration = ACLAdapter.adapt(userConfiguration); + Assert.deepStrictEqual(appConfiguration, { + default_policy: "deny", + any: [], + groups: {}, + users: {} + }); + }); + }); +}); \ No newline at end of file From 54c93fc9458cae09243a4aa6e0b95a5ad5be0298 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Sun, 8 Oct 2017 16:28:10 +0200 Subject: [PATCH 2/3] Fix randomness with integration tests The notification message pops up and hide after few seconds. Sometimes, chrome drivers tries to click on a button that moves due to the notification message animation and thus miss it. --- test/features/redirection.feature | 1 + test/features/step_definitions/authentication.ts | 9 +++++++++ test/features/step_definitions/notifications.ts | 2 +- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/test/features/redirection.feature b/test/features/redirection.feature index 2b5a6602..12fba93b 100644 --- a/test/features/redirection.feature +++ b/test/features/redirection.feature @@ -8,6 +8,7 @@ Feature: User is correctly redirected Scenario: User is redirected to home page after several authentication tries When I visit "https://public.test.local:8080/secret.html" And I login with user "john" and password "badpassword" + And I wait for notification to disappear And I clear field "username" And I login with user "john" and password "password" And I use "REGISTERED" as TOTP token handle diff --git a/test/features/step_definitions/authentication.ts b/test/features/step_definitions/authentication.ts index 140434e8..6899d596 100644 --- a/test/features/step_definitions/authentication.ts +++ b/test/features/step_definitions/authentication.ts @@ -11,6 +11,15 @@ Cucumber.defineSupportCode(function ({ Given, When, Then }) { return this.visit(link); }); + When("I wait for notification to disappear", function() { + const that = this; + const notificationEl = this.driver.findElement(seleniumWebdriver.By.className("notification")); + return this.driver.wait(seleniumWebdriver.until.elementIsVisible(notificationEl), 15000) + .then(function() { + return that.driver.wait(seleniumWebdriver.until.elementIsNotVisible(notificationEl), 15000); + }) + }) + When("I set field {stringInDoubleQuotes} to {stringInDoubleQuotes}", function (fieldName: string, content: string) { return this.setFieldTo(fieldName, content); }); diff --git a/test/features/step_definitions/notifications.ts b/test/features/step_definitions/notifications.ts index 3a068129..8916ba7e 100644 --- a/test/features/step_definitions/notifications.ts +++ b/test/features/step_definitions/notifications.ts @@ -10,7 +10,7 @@ Cucumber.defineSupportCode(function ({ Given, When, Then }) { function (notificationType: string, notificationMessage: string) { const that = this; const notificationEl = this.driver.findElement(seleniumWebdriver.By.className("notification")); - return this.driver.wait(seleniumWebdriver.until.elementIsVisible(notificationEl), 2000) + return this.driver.wait(seleniumWebdriver.until.elementIsVisible(notificationEl), 5000) .then(function () { return notificationEl.getText(); }) From b7a180af9bf330e7d47ac36c6157ba63a43910e6 Mon Sep 17 00:00:00 2001 From: Clement Michaud Date: Sun, 8 Oct 2017 17:13:29 +0200 Subject: [PATCH 3/3] Fix randomness in integration tests --- test/features/regulation.feature | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/features/regulation.feature b/test/features/regulation.feature index 54e40b79..f361c2dd 100644 --- a/test/features/regulation.feature +++ b/test/features/regulation.feature @@ -37,6 +37,4 @@ Feature: Authelia regulates authentication to avoid brute force And I click on "Sign in" And I use "REGISTERED" as TOTP token handle And I click on "TOTP" - Then I have access to: - | url | - | https://public.test.local:8080/secret.html | \ No newline at end of file + Then I'm redirected to "https://public.test.local:8080/secret.html" \ No newline at end of file