From 751f8552b2887e283bba19cdf9c43513c90753b8 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Wed, 7 Nov 2018 15:57:41 +0000 Subject: [PATCH] UI: Removes success notification on faking a success response for `self` (#4906) In order to continue supporting the legacy ACL system, we replace the 500 error from a non-existent `self` endpoint with a response of a `null` `AccessorID` - which makes sense (a null AccessorID means old API) We then redirect the user to the old ACL pages which then gives a 403 if their token was wrong which then redirects them back to the login page. Due to the multiple redirects and not wanting to test the validity of the token before redirecting (thus calling the same API endpoint twice), it is not straightforwards to turn the 'faked' response from the `self` endpoint into an error (flash messages are 'lost' through multiple redirects). In order to make this a slightly better experience, you can now return a `false` during execution of an action requiring success/failure feedback, this essentially skips the notification, so if the action is 'successful' but you don't want to show the notification, you can. This resolves showing a successful notification when the `self` endpoint response is faked. The last part of the puzzle is to make sure that the global 403 catching error in the application Route also produces an erroneous notification. Please note this can only happen with a ui client using the new ACL system when communicating with a cluster using the old ACL system, and only when you enter the wrong token. Lastly, further acceptance tests have been added around this This commit also adds functionality to avoid any possible double notification messages, to avoid UI overlapping --- ui-v2/app/routes/application.js | 16 +++-- ui-v2/app/routes/dc/acls.js | 8 ++- ui-v2/app/services/feedback.js | 24 +++++--- ui-v2/app/utils/acls-status.js | 2 + .../dc/acls/tokens/login-errors.feature | 52 ++++++++++++++++ .../dc/acls/tokens/login-errors-steps.js | 10 +++ ui-v2/tests/steps.js | 3 + ui-v2/tests/unit/routes/application-test.js | 8 ++- ui-v2/yarn.lock | 61 ++++++++++++++++--- 9 files changed, 158 insertions(+), 26 deletions(-) create mode 100644 ui-v2/tests/acceptance/dc/acls/tokens/login-errors.feature create mode 100644 ui-v2/tests/acceptance/steps/dc/acls/tokens/login-errors-steps.js diff --git a/ui-v2/app/routes/application.js b/ui-v2/app/routes/application.js index 9c3de6c88..3b85e9cfb 100644 --- a/ui-v2/app/routes/application.js +++ b/ui-v2/app/routes/application.js @@ -3,11 +3,13 @@ import { inject as service } from '@ember/service'; import { hash } from 'rsvp'; import { get } from '@ember/object'; import { next } from '@ember/runloop'; +import { Promise } from 'rsvp'; +import WithBlockingActions from 'consul-ui/mixins/with-blocking-actions'; const $html = document.documentElement; const removeLoading = function() { return $html.classList.remove('ember-loading'); }; -export default Route.extend({ +export default Route.extend(WithBlockingActions, { init: function() { this._super(...arguments); }, @@ -61,11 +63,13 @@ export default Route.extend({ // if a token has not been sent at all, it just gives you a 200 with an empty dataset const model = this.modelFor('dc'); if (error.status === '403') { - return get(this, 'settings') - .delete('token') - .then(() => { - return this.transitionTo('dc.acls.tokens', model.dc.Name); - }); + return get(this, 'feedback').execute(() => { + return get(this, 'settings') + .delete('token') + .then(() => { + return Promise.reject(this.transitionTo('dc.acls.tokens', model.dc.Name)); + }); + }, 'authorize'); } if (error.status === '') { error.message = 'Error'; diff --git a/ui-v2/app/routes/dc/acls.js b/ui-v2/app/routes/dc/acls.js index cd32fd9c3..4f0162958 100644 --- a/ui-v2/app/routes/dc/acls.js +++ b/ui-v2/app/routes/dc/acls.js @@ -13,7 +13,7 @@ export default Route.extend(WithBlockingActions, { return get(this, 'repo') .self(secret, dc) .then(item => { - get(this, 'settings') + return get(this, 'settings') .persist({ token: { AccessorID: get(item, 'AccessorID'), @@ -25,7 +25,11 @@ export default Route.extend(WithBlockingActions, { // take the user to the legacy acls // otherwise just refresh the page if (get(item, 'token.AccessorID') === null) { - return this.transitionTo('dc.acls'); + // returning false for a feedback action means even though + // its successful, please skip this notification and don't display it + return this.transitionTo('dc.acls').then(function() { + return false; + }); } else { this.refresh(); } diff --git a/ui-v2/app/services/feedback.js b/ui-v2/app/services/feedback.js index ce841b7d2..494adab5d 100644 --- a/ui-v2/app/services/feedback.js +++ b/ui-v2/app/services/feedback.js @@ -25,17 +25,23 @@ export default Service.extend({ handle() //TODO: pass this through to getAction.. .then(item => { - // TODO right now the majority of `item` is a Transition - // but you can resolve an object - notify.add({ - ...notificationDefaults(), - type: getStatus(TYPE_SUCCESS), - // here.. - action: getAction(), - item: item, - }); + // returning exactly `false` for a feedback action means even though + // its successful, please skip this notification and don't display it + if (item !== false) { + notify.clearMessages(); + // TODO right now the majority of `item` is a Transition + // but you can resolve an object + notify.add({ + ...notificationDefaults(), + type: getStatus(TYPE_SUCCESS), + // here.. + action: getAction(), + item: item, + }); + } }) .catch(e => { + notify.clearMessages(); get(this, 'logger').execute(e); if (e.name === 'TransitionAborted') { notify.add({ diff --git a/ui-v2/app/utils/acls-status.js b/ui-v2/app/utils/acls-status.js index d6ba94991..9712e5820 100644 --- a/ui-v2/app/utils/acls-status.js +++ b/ui-v2/app/utils/acls-status.js @@ -28,6 +28,8 @@ export default function(isValidServerError, P = Promise) { if (isValidServerError(e)) { enable(true); authorize(false); + } else { + return P.reject(e); } break; case '403': diff --git a/ui-v2/tests/acceptance/dc/acls/tokens/login-errors.feature b/ui-v2/tests/acceptance/dc/acls/tokens/login-errors.feature new file mode 100644 index 000000000..a35f447f3 --- /dev/null +++ b/ui-v2/tests/acceptance/dc/acls/tokens/login-errors.feature @@ -0,0 +1,52 @@ +@setupApplicationTest +Feature: dc / acls / tokens / index: ACL Login Errors + + Scenario: I get any 500 error that is not the specific legacy token cluster one + Given 1 datacenter model with the value "dc-1" + Given the url "/v1/acl/tokens" responds with a 500 status + When I visit the tokens page for yaml + --- + dc: dc-1 + --- + Then the url should be /dc-1/acls/tokens + Then I see the text "500 (The backend responded with an error)" in "[data-test-error]" + Scenario: I get a 500 error from acl/tokens that is the specific legacy one + Given 1 datacenter model with the value "dc-1" + And the url "/v1/acl/tokens" responds with from yaml + --- + status: 500 + body: "rpc error making call: rpc: can't find method ACL.TokenRead" + --- + When I visit the tokens page for yaml + --- + dc: dc-1 + --- + Then the url should be /dc-1/acls/tokens + Then ".app-view" has the "unauthorized" class + Scenario: I get a 500 error from acl/token/self that is the specific legacy one + Given 1 datacenter model with the value "dc-1" + Given the url "/v1/acl/tokens" responds with from yaml + --- + status: 500 + body: "rpc error making call: rpc: can't find method ACL.TokenRead" + --- + And the url "/v1/acl/token/self" responds with from yaml + --- + status: 500 + body: "rpc error making call: rpc: can't find method ACL.TokenRead" + --- + And the url "/v1/acl/list" responds with a 403 status + When I visit the tokens page for yaml + --- + dc: dc-1 + --- + Then the url should be /dc-1/acls/tokens + Then ".app-view" has the "unauthorized" class + Then I fill in with yaml + --- + secret: something + --- + And I submit + Then ".app-view" has the "unauthorized" class + And "[data-notification]" has the "error" class + diff --git a/ui-v2/tests/acceptance/steps/dc/acls/tokens/login-errors-steps.js b/ui-v2/tests/acceptance/steps/dc/acls/tokens/login-errors-steps.js new file mode 100644 index 000000000..9bfbe9ac9 --- /dev/null +++ b/ui-v2/tests/acceptance/steps/dc/acls/tokens/login-errors-steps.js @@ -0,0 +1,10 @@ +import steps from '../../../steps'; + +// step definitions that are shared between features should be moved to the +// tests/acceptance/steps/steps.js file + +export default function(assert) { + return steps(assert).then('I should find a file', function() { + assert.ok(true, this.step); + }); +} diff --git a/ui-v2/tests/steps.js b/ui-v2/tests/steps.js index 8ecb194fb..859cfe830 100644 --- a/ui-v2/tests/steps.js +++ b/ui-v2/tests/steps.js @@ -84,6 +84,9 @@ export default function(assert) { .given(['the url "$url" responds with a $status status'], function(url, status) { return api.server.respondWithStatus(url.split('?')[0], parseInt(status)); }) + .given(['the url "$url" responds with from yaml\n$yaml'], function(url, data) { + api.server.respondWith(url.split('?')[0], data); + }) // interactions .when('I visit the $name page', function(name) { currentPage = pages[name]; diff --git a/ui-v2/tests/unit/routes/application-test.js b/ui-v2/tests/unit/routes/application-test.js index 13dd42bed..5474f3d76 100644 --- a/ui-v2/tests/unit/routes/application-test.js +++ b/ui-v2/tests/unit/routes/application-test.js @@ -2,7 +2,13 @@ import { moduleFor, test } from 'ember-qunit'; moduleFor('route:application', 'Unit | Route | application', { // Specify the other units that are required for this test. - needs: ['service:repository/dc', 'service:settings'], + needs: [ + 'service:repository/dc', + 'service:settings', + 'service:feedback', + 'service:flashMessages', + 'service:logger', + ], }); test('it exists', function(assert) { diff --git a/ui-v2/yarn.lock b/ui-v2/yarn.lock index 3402c1a04..1fe965b5d 100644 --- a/ui-v2/yarn.lock +++ b/ui-v2/yarn.lock @@ -610,8 +610,8 @@ resolved "https://registry.yarnpkg.com/@hashicorp/consul-api-double/-/consul-api-double-2.0.1.tgz#eaf2e3f230fbdd876c90b931fd4bb4d94aac10e2" "@hashicorp/ember-cli-api-double@^1.3.0": - version "1.6.1" - resolved "https://registry.yarnpkg.com/@hashicorp/ember-cli-api-double/-/ember-cli-api-double-1.6.1.tgz#c45efe045da971dcb0915f182cdc4a75929a7ade" + version "1.7.0" + resolved "https://registry.yarnpkg.com/@hashicorp/ember-cli-api-double/-/ember-cli-api-double-1.7.0.tgz#4fdab6152157dd82b999de030c593c87e0cdb8b7" dependencies: "@hashicorp/api-double" "^1.3.0" array-range "^1.0.1" @@ -2028,7 +2028,7 @@ body-parser@1.18.2: raw-body "2.3.2" type-is "~1.6.15" -body-parser@^1.18.3: +body-parser@1.18.3, body-parser@^1.18.3: version "1.18.3" resolved "https://registry.yarnpkg.com/body-parser/-/body-parser-1.18.3.tgz#5b292198ffdd553b3a0f20ded0592b956955c8b4" dependencies: @@ -5299,7 +5299,7 @@ expand-tilde@^2.0.0, expand-tilde@^2.0.2: dependencies: homedir-polyfill "^1.0.1" -express@^4.10.7, express@^4.12.3, express@^4.16.2: +express@^4.10.7, express@^4.12.3: version "4.16.3" resolved "https://registry.yarnpkg.com/express/-/express-4.16.3.tgz#6af8a502350db3246ecc4becf6b5a34d22f7ed53" dependencies: @@ -5334,6 +5334,41 @@ express@^4.10.7, express@^4.12.3, express@^4.16.2: utils-merge "1.0.1" vary "~1.1.2" +express@^4.16.2: + version "4.16.4" + resolved "https://registry.yarnpkg.com/express/-/express-4.16.4.tgz#fddef61926109e24c515ea97fd2f1bdbf62df12e" + dependencies: + accepts "~1.3.5" + array-flatten "1.1.1" + body-parser "1.18.3" + content-disposition "0.5.2" + content-type "~1.0.4" + cookie "0.3.1" + cookie-signature "1.0.6" + debug "2.6.9" + depd "~1.1.2" + encodeurl "~1.0.2" + escape-html "~1.0.3" + etag "~1.8.1" + finalhandler "1.1.1" + fresh "0.5.2" + merge-descriptors "1.0.1" + methods "~1.1.2" + on-finished "~2.3.0" + parseurl "~1.3.2" + path-to-regexp "0.1.7" + proxy-addr "~2.0.4" + qs "6.5.2" + range-parser "~1.2.0" + safe-buffer "5.1.2" + send "0.16.2" + serve-static "1.13.2" + setprototypeof "1.1.0" + statuses "~1.4.0" + type-is "~1.6.16" + utils-merge "1.0.1" + vary "~1.1.2" + extend-shallow@^2.0.1: version "2.0.1" resolved "https://registry.yarnpkg.com/extend-shallow/-/extend-shallow-2.0.1.tgz#51af7d614ad9a9f610ea1bafbb989d6b1c56890f" @@ -7833,7 +7868,7 @@ mdurl@^1.0.1: media-typer@0.3.0: version "0.3.0" - resolved "https://registry.yarnpkg.com/media-typer/-/media-typer-0.3.0.tgz#8710d7af0aa626f8fffa1ce00168545263255748" + resolved "http://registry.npmjs.org/media-typer/-/media-typer-0.3.0.tgz#8710d7af0aa626f8fffa1ce00168545263255748" mem@^1.1.0: version "1.1.0" @@ -7956,7 +7991,11 @@ mime-db@~1.36.0: version "1.36.0" resolved "https://registry.yarnpkg.com/mime-db/-/mime-db-1.36.0.tgz#5020478db3c7fe93aad7bbcc4dcf869c43363397" -mime-types@^2.1.12, mime-types@~2.1.17, mime-types@~2.1.18, mime-types@~2.1.19, mime-types@~2.1.7: +mime-db@~1.37.0: + version "1.37.0" + resolved "https://registry.yarnpkg.com/mime-db/-/mime-db-1.37.0.tgz#0b6a0ce6fdbe9576e25f1f2d2fde8830dc0ad0d8" + +mime-types@^2.1.12, mime-types@~2.1.17, mime-types@~2.1.19, mime-types@~2.1.7: version "2.1.20" resolved "https://registry.yarnpkg.com/mime-types/-/mime-types-2.1.20.tgz#930cb719d571e903738520f8470911548ca2cc19" dependencies: @@ -7968,6 +8007,12 @@ mime-types@^2.1.18: dependencies: mime-db "~1.33.0" +mime-types@~2.1.18: + version "2.1.21" + resolved "https://registry.yarnpkg.com/mime-types/-/mime-types-2.1.21.tgz#28995aa1ecb770742fe6ae7e58f9181c744b3f96" + dependencies: + mime-db "~1.37.0" + mime@1.4.1: version "1.4.1" resolved "https://registry.yarnpkg.com/mime/-/mime-1.4.1.tgz#121f9ebc49e3766f311a76e1fa1c8003c4b03aa6" @@ -8979,7 +9024,7 @@ promised-io@*: version "0.3.5" resolved "https://registry.yarnpkg.com/promised-io/-/promised-io-0.3.5.tgz#4ad217bb3658bcaae9946b17a8668ecd851e1356" -proxy-addr@~2.0.3: +proxy-addr@~2.0.3, proxy-addr@~2.0.4: version "2.0.4" resolved "https://registry.yarnpkg.com/proxy-addr/-/proxy-addr-2.0.4.tgz#ecfc733bf22ff8c6f407fa275327b9ab67e48b93" dependencies: @@ -9707,7 +9752,7 @@ safe-buffer@5.1.1: version "5.1.1" resolved "https://registry.yarnpkg.com/safe-buffer/-/safe-buffer-5.1.1.tgz#893312af69b2123def71f57889001671eeb2c853" -safe-buffer@^5.0.1, safe-buffer@^5.1.0, safe-buffer@^5.1.1, safe-buffer@^5.1.2, safe-buffer@~5.1.0, safe-buffer@~5.1.1: +safe-buffer@5.1.2, safe-buffer@^5.0.1, safe-buffer@^5.1.0, safe-buffer@^5.1.1, safe-buffer@^5.1.2, safe-buffer@~5.1.0, safe-buffer@~5.1.1: version "5.1.2" resolved "https://registry.yarnpkg.com/safe-buffer/-/safe-buffer-5.1.2.tgz#991ec69d296e0313747d59bdfd2b745c35f8828d"