From 101a20e03e16672f793ff42368d3fdb23080797e Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Tue, 18 Oct 2022 16:07:12 +0200 Subject: [PATCH 1/5] Improve testability `env`-service --- ui/packages/consul-ui/app/services/env.js | 60 +++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/ui/packages/consul-ui/app/services/env.js b/ui/packages/consul-ui/app/services/env.js index bbd9a9c1e..97b54c94c 100644 --- a/ui/packages/consul-ui/app/services/env.js +++ b/ui/packages/consul-ui/app/services/env.js @@ -12,3 +12,63 @@ export default class EnvService extends Service { return env(key); } } +/** + * Stub class that can be used in testing when we want to test + * interactions with the EnvService. We can use `EnvStub.stubEnv` to setup + * an Env-Service that returns certain values we need to execute our tests. + * + * Example: + * + * ```js + * // some-test.js + * test('testing interaction with Env-service', async function(assert) { + * this.owner.register('service:env', class Stub extends EnvStub { + * . stubEnv = { + * CONSUL_ACLS_ENABLED: true + * } + * }) + * }) + * ``` + */ +export class EnvStub extends EnvService { + var(key) { + const { stubEnv } = this; + + const stubbed = stubEnv[key]; + + if (stubbed) { + return stubbed; + } else { + return super.var(...arguments); + } + } +} + +/** + * Helper function to allow stubbing out data that is accessed by the application + * based on the Env-service. You will need to call this before the env-service gets + * initialized because it overrides the env-service injection on the owner. + * + * Example: + * + * ```js + * test('test something env related', async function(assert) { + * setupTestEnv(this.owner, { + * CONSUL_ACLS_ENABLED: true + * }); + * + * // ... + * }) + * ``` + * + * @param {*} owner - the owner of the test instance (usually `this.owner`) + * @param {*} stubEnv - an object that holds the stubbed env-data + */ +export function setupTestEnv(owner, stubEnv) { + owner.register( + 'service:env', + class Stub extends EnvStub { + stubEnv = stubEnv; + } + ); +} From 12a923c658bdb97e9619c3765d036389f5a9b648 Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Tue, 18 Oct 2022 16:07:34 +0200 Subject: [PATCH 2/5] Add tests for TokenSecretID handling --- .../tests/acceptance/hcp-login-test.js | 173 ++++++++++++++++++ 1 file changed, 173 insertions(+) create mode 100644 ui/packages/consul-ui/tests/acceptance/hcp-login-test.js diff --git a/ui/packages/consul-ui/tests/acceptance/hcp-login-test.js b/ui/packages/consul-ui/tests/acceptance/hcp-login-test.js new file mode 100644 index 000000000..28e08aaef --- /dev/null +++ b/ui/packages/consul-ui/tests/acceptance/hcp-login-test.js @@ -0,0 +1,173 @@ +import { module, test } from 'qunit'; +import { visit } from '@ember/test-helpers'; +import { setupApplicationTest } from 'ember-qunit'; +import { setupTestEnv } from 'consul-ui/services/env'; +import TokenRepo from 'consul-ui/services/repository/token'; +import SettingsService from 'consul-ui/services/settings'; + +const TOKEN_SET_BY_HCP = 'token-set-by-hcp'; + +module('Acceptance | hcp login', function (hooks) { + setupApplicationTest(hooks); + + module('with `CONSUL_HTTP_TOKEN` not set', function () { + hooks.beforeEach(function () { + setupTestEnv(this.owner, { + CONSUL_ACLS_ENABLED: true, + }); + }); + + test('we do not call the token endpoint', async function (assert) { + this.owner.register( + 'service:repository/token', + class extends TokenRepo { + self() { + assert.step('token'); + + return super.self(...arguments); + } + } + ); + + await visit('/'); + + assert.verifySteps([], 'we do not try to fetch new token'); + }); + }); + + module('with `CONSUL_HTTP_TOKEN` set', function (hooks) { + hooks.beforeEach(function () { + setupTestEnv(this.owner, { + CONSUL_ACLS_ENABLED: true, + CONSUL_HTTP_TOKEN: TOKEN_SET_BY_HCP, + }); + }); + + test('when no token was persisted to settings', async function (assert) { + assert.expect(3); + + // stub out the settings service to not access local-storage directly + this.owner.register( + 'service:settings', + class extends SettingsService { + async findBySlug(slug) { + // make sure we don't find anything + if (slug === 'token') { + // we return an empty string if nothing is found + return Promise.resolve(''); + } else { + return super.findBySlug(...arguments); + } + } + } + ); + + // There's no way to hook into the api handlers like with mirage + // so we need to stub the repo methods + this.owner.register( + 'service:repository/token', + class extends TokenRepo { + self(params) { + const { secret } = params; + + assert.equal( + secret, + TOKEN_SET_BY_HCP, + 'we try to request token based on what HCP set for us' + ); + + assert.step('token'); + + return super.self(...arguments); + } + } + ); + + await visit('/'); + + assert.verifySteps(['token'], 'we try to call token endpoint to fetch new token'); + }); + + test('when we already persisted a token to settings and it is different to the secret HCP set for us', async function (assert) { + assert.expect(3); + + this.owner.register( + 'service:settings', + class extends SettingsService { + async findBySlug(slug) { + if (slug === 'token') { + return Promise.resolve({ + AccessorID: 'accessor', + SecretID: 'secret', + Namespace: 'default', + Partition: 'default', + }); + } else { + return super.findBySlug(...arguments); + } + } + } + ); + + this.owner.register( + 'service:repository/token', + class extends TokenRepo { + self(params) { + const { secret } = params; + + assert.equal( + secret, + TOKEN_SET_BY_HCP, + 'we try to request token based on what HCP set for us' + ); + + assert.step('token'); + + return super.self(...arguments); + } + } + ); + + await visit('/'); + + assert.verifySteps(['token'], 'we call token endpoint to fetch new token'); + }); + + test('when we already persisted a token to settings, but it is the same secret as HCP set for us', async function (assert) { + assert.expect(1); + + this.owner.register( + 'service:settings', + class extends SettingsService { + async findBySlug(slug) { + if (slug === 'token') { + return Promise.resolve({ + AccessorID: 'accessor', + SecretID: TOKEN_SET_BY_HCP, + Namespace: 'default', + Partition: 'default', + }); + } else { + return super.findBySlug(...arguments); + } + } + } + ); + + this.owner.register( + 'service:repository/token', + class extends TokenRepo { + self() { + assert.step('token'); + + return super.self(...arguments); + } + } + ); + + await visit('/'); + + assert.verifySteps([], 'we do not try to fetch new token'); + }); + }); +}); From 955949998f6617dc931721d970c76e30d076a244 Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Tue, 18 Oct 2022 16:07:48 +0200 Subject: [PATCH 3/5] Fix `TokenSecretID`-handling --- .../consul-ui/app/routes/application.js | 37 ++++++++++--------- 1 file changed, 20 insertions(+), 17 deletions(-) diff --git a/ui/packages/consul-ui/app/routes/application.js b/ui/packages/consul-ui/app/routes/application.js index dd81c3498..8f4838c95 100644 --- a/ui/packages/consul-ui/app/routes/application.js +++ b/ui/packages/consul-ui/app/routes/application.js @@ -16,23 +16,26 @@ export default class ApplicationRoute extends Route.extend(WithBlockingActions) async model() { if (this.env.var('CONSUL_ACLS_ENABLED')) { const secret = this.env.var('CONSUL_HTTP_TOKEN'); - const existing = await this.settings.findBySlug('token'); - if (!existing.AccessorID && secret) { - try { - const token = await this.tokenRepo.self({ - secret: secret, - dc: this.env.var('CONSUL_DATACENTER_LOCAL'), - }); - await this.settings.persist({ - token: { - AccessorID: token.AccessorID, - SecretID: token.SecretID, - Namespace: token.Namespace, - Partition: token.Partition, - }, - }); - } catch (e) { - runInDebug((_) => console.error(e)); + if (secret) { + const existing = await this.settings.findBySlug('token'); + + if (secret && secret !== existing.SecretID) { + try { + const token = await this.tokenRepo.self({ + secret: secret, + dc: this.env.var('CONSUL_DATACENTER_LOCAL'), + }); + await this.settings.persist({ + token: { + AccessorID: token.AccessorID, + SecretID: token.SecretID, + Namespace: token.Namespace, + Partition: token.Partition, + }, + }); + } catch (e) { + runInDebug((_) => console.error(e)); + } } } } From 163f8b905755ef8b94d0c4492ad5da72cdcecaa0 Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Tue, 18 Oct 2022 16:16:20 +0200 Subject: [PATCH 4/5] Encapsulate hcp related logic in service --- ui/packages/consul-ui/app/services/hcp.js | 37 +++++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 ui/packages/consul-ui/app/services/hcp.js diff --git a/ui/packages/consul-ui/app/services/hcp.js b/ui/packages/consul-ui/app/services/hcp.js new file mode 100644 index 000000000..f0168254d --- /dev/null +++ b/ui/packages/consul-ui/app/services/hcp.js @@ -0,0 +1,37 @@ +import Service, { inject as service } from '@ember/service'; +import { runInDebug } from '@ember/debug'; + +/** + * A service to encapsulate all logic that handles dealing with setting up consul + * core correctly when started via HCP. + */ +export default class HCPService extends Service { + @service('env') env; + @service('repository/token') tokenRepo; + @service('settings') settings; + + async updateTokenIfNecessary(secret) { + if (secret) { + const existing = await this.settings.findBySlug('token'); + + if (secret && secret !== existing.SecretID) { + try { + const token = await this.tokenRepo.self({ + secret: secret, + dc: this.env.var('CONSUL_DATACENTER_LOCAL'), + }); + await this.settings.persist({ + token: { + AccessorID: token.AccessorID, + SecretID: token.SecretID, + Namespace: token.Namespace, + Partition: token.Partition, + }, + }); + } catch (e) { + runInDebug((_) => console.error(e)); + } + } + } + } +} From e856a2c5c5026fff8deb3727645ce5f15268edc2 Mon Sep 17 00:00:00 2001 From: Michael Klein Date: Tue, 18 Oct 2022 16:16:42 +0200 Subject: [PATCH 5/5] Cleanup app boot by using hcp service --- .../consul-ui/app/routes/application.js | 28 ++----------------- 1 file changed, 2 insertions(+), 26 deletions(-) diff --git a/ui/packages/consul-ui/app/routes/application.js b/ui/packages/consul-ui/app/routes/application.js index 8f4838c95..e15642ef1 100644 --- a/ui/packages/consul-ui/app/routes/application.js +++ b/ui/packages/consul-ui/app/routes/application.js @@ -1,43 +1,19 @@ import Route from 'consul-ui/routing/route'; import { action } from '@ember/object'; import { inject as service } from '@ember/service'; -import { runInDebug } from '@ember/debug'; import WithBlockingActions from 'consul-ui/mixins/with-blocking-actions'; export default class ApplicationRoute extends Route.extend(WithBlockingActions) { @service('client/http') client; @service('env') env; - @service('repository/token') tokenRepo; - @service('settings') settings; + @service() hcp; data; async model() { if (this.env.var('CONSUL_ACLS_ENABLED')) { - const secret = this.env.var('CONSUL_HTTP_TOKEN'); - if (secret) { - const existing = await this.settings.findBySlug('token'); - - if (secret && secret !== existing.SecretID) { - try { - const token = await this.tokenRepo.self({ - secret: secret, - dc: this.env.var('CONSUL_DATACENTER_LOCAL'), - }); - await this.settings.persist({ - token: { - AccessorID: token.AccessorID, - SecretID: token.SecretID, - Namespace: token.Namespace, - Partition: token.Partition, - }, - }); - } catch (e) { - runInDebug((_) => console.error(e)); - } - } - } + await this.hcp.updateTokenIfNecessary(this.env.var('CONSUL_HTTP_TOKEN')); } return {}; }