From f021719d8b2a9a1b21f0a359752851b5d5a603b6 Mon Sep 17 00:00:00 2001 From: Randall Morey Date: Thu, 6 May 2021 15:19:12 -0400 Subject: [PATCH 1/5] refactor: open and close oidc windows via window manager --- ui/admin/app/routes/scopes/scope/authenticate/method.js | 4 ++-- ui/admin/app/routes/scopes/scope/authenticate/method/oidc.js | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/ui/admin/app/routes/scopes/scope/authenticate/method.js b/ui/admin/app/routes/scopes/scope/authenticate/method.js index a7f6be207..d06bda35c 100644 --- a/ui/admin/app/routes/scopes/scope/authenticate/method.js +++ b/ui/admin/app/routes/scopes/scope/authenticate/method.js @@ -11,7 +11,7 @@ export default class ScopesScopeAuthenticateMethodRoute extends Route { @service session; @service notify; @service intl; - @service('browser/window') window; + @service windowManager; // =methods @@ -41,7 +41,7 @@ export default class ScopesScopeAuthenticateMethodRoute extends Route { * @param {string} url */ openExternalOIDCFlow(url) { - this.window.open(url); + this.windowManager.open(url); } // =actions diff --git a/ui/admin/app/routes/scopes/scope/authenticate/method/oidc.js b/ui/admin/app/routes/scopes/scope/authenticate/method/oidc.js index b6cb354c4..db24f3bbd 100644 --- a/ui/admin/app/routes/scopes/scope/authenticate/method/oidc.js +++ b/ui/admin/app/routes/scopes/scope/authenticate/method/oidc.js @@ -13,6 +13,7 @@ export default class ScopesScopeAuthenticateMethodOidcRoute extends Route { // =services @service session; + @service windowManager; // =attributes @@ -61,10 +62,12 @@ export default class ScopesScopeAuthenticateMethodOidcRoute extends Route { } /** - * When this route is deactivated (exited), stop polling for changes. + * When this route is deactivated (exited), stop polling for changes and close + * any windows opened via the window manager service. */ deactivate() { this.poller.cancelAll(); + this.windowManager.closeAll(); } /** From 9a909c0ec165a6ca7b0ef892b088230f46acc9a3 Mon Sep 17 00:00:00 2001 From: Randall Morey Date: Fri, 7 May 2021 13:21:18 -0400 Subject: [PATCH 2/5] =?UTF-8?q?chore:=20=F0=9F=A4=96=20retry=20oidc=20shou?= =?UTF-8?q?ld=20also=20restart=20with=20boundary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rather than just re-opening the external window --- .../app/templates/scopes/scope/authenticate/method/oidc.hbs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ui/admin/app/templates/scopes/scope/authenticate/method/oidc.hbs b/ui/admin/app/templates/scopes/scope/authenticate/method/oidc.hbs index dedca3ab3..7b543d888 100644 --- a/ui/admin/app/templates/scopes/scope/authenticate/method/oidc.hbs +++ b/ui/admin/app/templates/scopes/scope/authenticate/method/oidc.hbs @@ -8,9 +8,9 @@

{{t 'resources.auth-method.questions.no-see-window'}} {{t 'actions.retry'}} From da6b34b337ecc83e492a06460b963ec46eb7553a Mon Sep 17 00:00:00 2001 From: Randall Morey Date: Fri, 7 May 2021 14:32:37 -0400 Subject: [PATCH 3/5] =?UTF-8?q?feat:=20=F0=9F=8E=B8=20enable=20inline=20OI?= =?UTF-8?q?DC=20authentication?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../scopes/scope/authenticate/method.js | 14 ++----- .../scopes/scope/authenticate/method/oidc.hbs | 2 +- ui/admin/config/environment.js | 3 +- .../tests/acceptance/authentication-test.js | 38 ++++++++++++++++++- 4 files changed, 44 insertions(+), 13 deletions(-) diff --git a/ui/admin/app/routes/scopes/scope/authenticate/method.js b/ui/admin/app/routes/scopes/scope/authenticate/method.js index d06bda35c..9c42ca274 100644 --- a/ui/admin/app/routes/scopes/scope/authenticate/method.js +++ b/ui/admin/app/routes/scopes/scope/authenticate/method.js @@ -26,22 +26,16 @@ export default class ScopesScopeAuthenticateMethodRoute extends Route { } /** - * + * Kicks off the OIDC auth flow by opening a new window to the provider. + * @param {string} authenticatorName + * @param {object} options */ async startOIDCAuthentication(authenticatorName, options) { const oidc = getOwner(this).lookup(authenticatorName); // TODO: delegate this call from the session service so that we don't have // to look up the authenticator directly const json = await oidc.startAuthentication(options); - await this.openExternalOIDCFlow(json.attributes.auth_url); - } - - /** - * Opens the specified URL in a new tab or window. - * @param {string} url - */ - openExternalOIDCFlow(url) { - this.windowManager.open(url); + await this.windowManager.open(json.attributes.auth_url); } // =actions diff --git a/ui/admin/app/templates/scopes/scope/authenticate/method/oidc.hbs b/ui/admin/app/templates/scopes/scope/authenticate/method/oidc.hbs index 7b543d888..651b0a350 100644 --- a/ui/admin/app/templates/scopes/scope/authenticate/method/oidc.hbs +++ b/ui/admin/app/templates/scopes/scope/authenticate/method/oidc.hbs @@ -9,7 +9,7 @@ {{t 'resources.auth-method.questions.no-see-window'}} {{t 'actions.retry'}} diff --git a/ui/admin/config/environment.js b/ui/admin/config/environment.js index a0350eae9..14982afa1 100644 --- a/ui/admin/config/environment.js +++ b/ui/admin/config/environment.js @@ -149,8 +149,9 @@ module.exports = function (environment) { // Notification timeout should be 0 for fast tests ENV.notifyTimeout = 0; - // Session polling timeout should be short + // Polling timeouts should be short ENV.sessionPollingTimeoutSeconds = 0.25; + ENV.oidcPollingTimeoutSeconds = 0; ENV.enableConfirmService = false; } diff --git a/ui/admin/tests/acceptance/authentication-test.js b/ui/admin/tests/acceptance/authentication-test.js index 3578ae935..f42540f4f 100644 --- a/ui/admin/tests/acceptance/authentication-test.js +++ b/ui/admin/tests/acceptance/authentication-test.js @@ -12,12 +12,14 @@ import { import { setupApplicationTest } from 'ember-qunit'; import setupMirage from 'ember-cli-mirage/test-support/setup-mirage'; import { Response } from 'miragejs'; +//import { run, later } from '@ember/runloop'; import a11yAudit from 'ember-a11y-testing/test-support/audit'; import { currentSession, authenticateSession, invalidateSession, } from 'ember-simple-auth/test-support'; +import Service from '@ember/service'; module('Acceptance | authentication', function (hooks) { setupApplicationTest(hooks); @@ -31,12 +33,15 @@ module('Acceptance | authentication', function (hooks) { let orgScopeURL; let scope; let globalAuthMethod; - let authMethod; let globalAuthMethodID; + let authMethod; let authMethodID; + let authMethodOIDC; + let authMethodOIDCID; let globalAuthenticateURL; let authenticateURL; let authMethodAuthenticateURL; + let authMethodOIDCAuthenticateURL; let authMethodGlobalAuthenticateURL; let changePasswordURL; let orgsURL; @@ -72,15 +77,21 @@ module('Acceptance | authentication', function (hooks) { scope: orgScope, type: 'password', }); + authMethodOIDC = this.server.create('auth-method', { + scope: orgScope, + type: 'oidc', + }); orgScopeID = orgScope.id; globalAuthMethodID = globalAuthMethod.id; authMethodID = authMethod.id; + authMethodOIDCID = authMethodOIDC.id; scopesURL = `/scopes`; orgScopeURL = `/scopes/${orgScopeID}`; globalAuthenticateURL = `/scopes/global/authenticate`; authenticateURL = `/scopes/${orgScopeID}/authenticate`; authMethodGlobalAuthenticateURL = `/scopes/global/authenticate/${globalAuthMethodID}`; authMethodAuthenticateURL = `/scopes/${orgScopeID}/authenticate/${authMethodID}`; + authMethodOIDCAuthenticateURL = `/scopes/${orgScopeID}/authenticate/${authMethodOIDCID}`; changePasswordURL = `/account/change-password`; orgsURL = `/scopes/global/scopes`; orgEditURL = `/scopes/${orgScopeID}/edit`; @@ -105,6 +116,7 @@ module('Acceptance | authentication', function (hooks) { test('visiting auth methods authenticate route when there no methods shows a message', async function (assert) { assert.expect(2); authMethod.destroy(); + authMethodOIDC.destroy(); await visit(authenticateURL); await a11yAudit(); assert.equal(currentURL(), authenticateURL); @@ -350,4 +362,28 @@ module('Acceptance | authentication', function (hooks) { assert.notOk(getRootElement().classList.contains('rose-theme-light')); assert.notOk(getRootElement().classList.contains('rose-theme-dark')); }); + + // TODO: figure out a sustainable test strategy for polling routes + test('OIDC authentication opens a new window and closes it upon completion', async function (assert) { + assert.expect(3); + // Register mock window service + this.owner.register( + 'service:browser/window', + class extends Service { + open(/* url */) { + assert.ok(true, 'New window was opened.'); + return { + close() { + assert.ok(true, 'New window was closed.'); + assert.ok(currentSession().isAuthenticated); + }, + }; + } + } + ); + await visit(authMethodOIDCAuthenticateURL); + await click('form [type="submit"]'); + }); + + // TODO: test OIDC retry and cancel }); From cf5f41acbd3c972bbc4a4f48f61ac078e737e13d Mon Sep 17 00:00:00 2001 From: Randall Morey Date: Sat, 8 May 2021 07:49:53 -0400 Subject: [PATCH 4/5] =?UTF-8?q?fix:=20=F0=9F=90=9B=20handle=20null=20windo?= =?UTF-8?q?w=20in=20case=20of=20popup=20blocker?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ui/admin/app/services/window-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/admin/app/services/window-manager.js b/ui/admin/app/services/window-manager.js index 8c54be361..b9cdacf04 100644 --- a/ui/admin/app/services/window-manager.js +++ b/ui/admin/app/services/window-manager.js @@ -28,7 +28,7 @@ export default class WindowManagerService extends Service { */ open(url) { const newWindow = this.window.open(url); - this.add(newWindow); + if (newWindow) this.add(newWindow); } /** From 343f73bc21848766de8a75094d117ead883ba8f2 Mon Sep 17 00:00:00 2001 From: Randall Morey Date: Sat, 8 May 2021 07:50:16 -0400 Subject: [PATCH 5/5] style: set max width for SVG in dialog --- ui/admin/app/styles/app.scss | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ui/admin/app/styles/app.scss b/ui/admin/app/styles/app.scss index 4647973af..420258319 100644 --- a/ui/admin/app/styles/app.scss +++ b/ui/admin/app/styles/app.scss @@ -152,6 +152,12 @@ } } +.rose-dialog-cover { + svg { + max-width: 20rem; + } +} + .copyable { white-space: nowrap;