From 1ddabf498001430867bf68c55675162afb85115c Mon Sep 17 00:00:00 2001 From: Zhihe Li Date: Thu, 2 Nov 2023 13:42:51 -0400 Subject: [PATCH] =?UTF-8?q?feat:=20=F0=9F=8E=B8=20Add=20checks=20for=20can?= =?UTF-8?q?cel=20permissions=20in=20desktop=20sessions=20(#1980)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat: 🎸 Add checks for cancel permissions in desktop sessions * chore: 🤖 Move model hook from session to session.index --- addons/api/addon/abilities/session.js | 16 +- addons/api/addon/models/session.js | 6 +- addons/api/mirage/factories/session.js | 4 +- .../api/tests/unit/abilities/session-test.js | 81 ++++++++ .../rose/components/layout/_body-content.scss | 2 +- .../app/components/session/tabs/index.hbs | 179 +++++++++--------- .../routes/scopes/scope/projects/sessions.js | 86 +-------- .../scopes/scope/projects/sessions/index.js | 85 ++++++++- .../scopes/scope/projects/targets/target.js | 1 + .../scopes/scope/projects/sessions/index.hbs | 13 +- .../scope/projects/sessions/session.hbs | 16 +- .../projects/sessions/index-test.js | 54 +++++- .../projects/sessions/session-test.js | 28 ++- 13 files changed, 373 insertions(+), 198 deletions(-) diff --git a/addons/api/addon/abilities/session.js b/addons/api/addon/abilities/session.js index ba9e7301f..5a506c049 100644 --- a/addons/api/addon/abilities/session.js +++ b/addons/api/addon/abilities/session.js @@ -14,13 +14,25 @@ export default class SessionAbility extends ModelAbility { // =permissions /** - * Only "active" or "pending" sessions may be read. + * Only available sessions may be read. * @type {boolean} */ get canRead() { return ( - (this.model.isActive || this.model.isPending) && + this.model.isAvailable && (this.hasAuthorizedAction('read:self') || super.canRead) ); } + + /** + * Only available sessions may be canceled. + * @type {boolean} + */ + get canCancel() { + return ( + this.model.isAvailable && + (this.hasAuthorizedAction('cancel:self') || + this.hasAuthorizedAction('cancel')) + ); + } } diff --git a/addons/api/addon/models/session.js b/addons/api/addon/models/session.js index 6096a8745..6848ba2ef 100644 --- a/addons/api/addon/models/session.js +++ b/addons/api/addon/models/session.js @@ -4,7 +4,6 @@ */ import GeneratedSessionModel from '../generated/models/session'; -import { computed } from '@ember/object'; import { equal } from '@ember/object/computed'; import { tracked } from '@glimmer/tracking'; import { A } from '@ember/array'; @@ -127,7 +126,6 @@ export default class SessionModel extends GeneratedSessionModel { /** * @type {boolean} */ - @computed('isActive', 'isPending') get isAvailable() { return this.isActive || this.isPending; } @@ -136,7 +134,6 @@ export default class SessionModel extends GeneratedSessionModel { * The full proxy address and port if address exists, otherwise null. * @type {?string} */ - @computed('proxy_address', 'proxy_port') get proxy() { return this.proxy_address ? `${this.proxy_address}:${this.proxy_port}` @@ -148,6 +145,9 @@ export default class SessionModel extends GeneratedSessionModel { * @type {TargetModel} */ get target() { + if (!this.target_id) { + return null; + } return this.store.peekRecord('target', this.target_id); } diff --git a/addons/api/mirage/factories/session.js b/addons/api/mirage/factories/session.js index 48c681814..864d1494a 100644 --- a/addons/api/mirage/factories/session.js +++ b/addons/api/mirage/factories/session.js @@ -7,6 +7,7 @@ import factory from '../generated/factories/session'; import { trait } from 'miragejs'; import { faker } from '@faker-js/faker'; import generateId from '../helpers/id'; +import permissions from '../helpers/permissions'; const statusStrings = ['pending', 'active', 'canceling', 'terminated']; @@ -15,7 +16,8 @@ export const pickRandomStatusString = () => export default factory.extend({ id: () => generateId('ss_'), - + authorized_actions: () => + permissions.authorizedActionsFor('session') || ['read', 'cancel'], status: pickRandomStatusString(), /** diff --git a/addons/api/tests/unit/abilities/session-test.js b/addons/api/tests/unit/abilities/session-test.js index 2d2d2ab75..5cc2166d5 100644 --- a/addons/api/tests/unit/abilities/session-test.js +++ b/addons/api/tests/unit/abilities/session-test.js @@ -108,4 +108,85 @@ module('Unit | Ability | session', function (hooks) { assert.true(canService.can('read session', session)); }); + + test('can cancel a session that is active', function (assert) { + assert.expect(1); + + const session = store.createRecord('session', { + authorized_actions: ['cancel'], + status: STATUS_SESSION_ACTIVE, + }); + + assert.true(canService.can('cancel session', session)); + }); + + test('can cancel a session that has cancel:self', function (assert) { + assert.expect(1); + + const session = store.createRecord('session', { + authorized_actions: ['cancel:self'], + status: STATUS_SESSION_ACTIVE, + }); + + assert.true(canService.can('cancel session', session)); + }); + + test('can cancel a session that is pending', function (assert) { + assert.expect(1); + + const session = store.createRecord('session', { + authorized_actions: ['cancel'], + status: STATUS_SESSION_PENDING, + }); + + assert.true(canService.can('cancel session', session)); + }); + + test('cannot cancel a session that is canceling', function (assert) { + assert.expect(1); + + const session = store.createRecord('session', { + authorized_actions: ['cancel'], + status: STATUS_SESSION_CANCELING, + }); + + assert.false(canService.can('cancel session', session)); + }); + + test('cannot cancel a session that is terminated', function (assert) { + assert.expect(1); + + const session = store.createRecord('session', { + authorized_actions: ['cancel'], + status: STATUS_SESSION_TERMINATED, + }); + + assert.false(canService.can('cancel session', session)); + }); + + test('cannot cancel a session that is not authorized', function (assert) { + assert.expect(4); + + const activeSession = store.createRecord('session', { + authorized_actions: [], + status: STATUS_SESSION_ACTIVE, + }); + const pendingSession = store.createRecord('session', { + authorized_actions: [], + status: STATUS_SESSION_PENDING, + }); + const cancelingSession = store.createRecord('session', { + authorized_actions: [], + status: STATUS_SESSION_CANCELING, + }); + const terminatedSession = store.createRecord('session', { + authorized_actions: [], + status: STATUS_SESSION_TERMINATED, + }); + + assert.false(canService.can('cancel session', activeSession)); + assert.false(canService.can('cancel session', pendingSession)); + assert.false(canService.can('cancel session', cancelingSession)); + assert.false(canService.can('cancel session', terminatedSession)); + }); }); diff --git a/addons/rose/app/styles/rose/components/layout/_body-content.scss b/addons/rose/app/styles/rose/components/layout/_body-content.scss index 0195535af..74b4e57ac 100644 --- a/addons/rose/app/styles/rose/components/layout/_body-content.scss +++ b/addons/rose/app/styles/rose/components/layout/_body-content.scss @@ -8,7 +8,7 @@ .rose-layout-body-content { display: grid; grid-template-areas: 'body body sidebar'; - grid-template-columns: 1fr 1fr 300px; + grid-template-columns: 1fr 1fr fit-content(300px); grid-template-rows: auto 1fr; gap: 2rem; diff --git a/ui/desktop/app/components/session/tabs/index.hbs b/ui/desktop/app/components/session/tabs/index.hbs index b01005577..bfcc82975 100644 --- a/ui/desktop/app/components/session/tabs/index.hbs +++ b/ui/desktop/app/components/session/tabs/index.hbs @@ -34,98 +34,105 @@ {{/if}} - -
- {{#if @model.host}} -
- {{t 'resources.host.details'}} -
- - {{! Host name }} - {{#if @model.host.name}} - - {{@model.host.name}} - - {{/if}} - {{! Host description }} - {{#if @model.host.description}} - - {{@model.host.description}} - - {{/if}} - {{! Host address }} - {{#if @model.host.address}} - - {{@model.host.address}} - - {{/if}} - {{! Host ID }} - - - {{@model.host.id}} - - - - {{/if}} - {{#if @model.target}} -
- {{t 'resources.target.details'}} -
- - {{! Target name }} - - {{@model.target.name}} - - {{! Project name }} - - {{@model.target.project.displayName}} - - {{! Target description }} - {{#if @model.target.description}} - - {{@model.target.description}} - - {{/if}} - {{! Target type }} - - {{#if @model.target.isSSH}} - {{t 'resources.target.types.ssh'}} - {{else}} - {{t 'resources.target.types.tcp'}} + + {{#if (or @model.host @model.target)}} + +
+ {{#if @model.host}} +
+ {{t 'resources.host.details'}} +
+ + {{! Host name }} + {{#if @model.host.name}} + + {{@model.host.name}} + {{/if}} - - {{! Target address }} - {{#if @model.target.address}} + {{! Host description }} + {{#if @model.host.description}} + + {{@model.host.description}} + + {{/if}} + {{! Host address }} + {{#if @model.host.address}} + + {{@model.host.address}} + + {{/if}} + {{! Host ID }} + + + {{@model.host.id}} + + + + {{/if}} + {{#if @model.target}} +
+ {{t 'resources.target.details'}} +
+ + {{! Target name }} + + {{@model.target.name}} + + {{! Project name }} + + {{@model.target.project.displayName}} + + {{! Target description }} + {{#if @model.target.description}} + + {{@model.target.description}} + + {{/if}} + {{! Target type }} - {{@model.target.address}} + {{#if @model.target.isSSH}} + {{t 'resources.target.types.ssh'}} + {{else}} + {{t 'resources.target.types.tcp'}} + {{/if}} - {{/if}} - {{! Target ID }} - - - {{@model.target.id}} - - - - {{/if}} -
- + {{! Target address }} + {{#if @model.target.address}} + + {{@model.target.address}} + + {{/if}} + {{! Target ID }} + + + {{@model.target.id}} + + + + {{/if}} +
+ + {{/if}} diff --git a/ui/desktop/app/routes/scopes/scope/projects/sessions.js b/ui/desktop/app/routes/scopes/scope/projects/sessions.js index 7def33b5c..f15e08917 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/sessions.js +++ b/ui/desktop/app/routes/scopes/scope/projects/sessions.js @@ -7,7 +7,6 @@ import Route from '@ember/routing/route'; import { inject as service } from '@ember/service'; import { action } from '@ember/object'; import { notifySuccess, notifyError } from 'core/decorators/notify'; -import { resourceFilter } from 'core/decorators/resource-filter'; import { loading } from 'ember-loading'; export default class ScopesScopeProjectsSessionsRoute extends Route { @@ -16,23 +15,8 @@ export default class ScopesScopeProjectsSessionsRoute extends Route { @service store; @service ipc; @service session; - @service resourceFilterStore; @service router; - - // =attributes - - @resourceFilter({ - allowed: ['active', 'pending', 'canceling', 'terminated'], - defaultValue: ['active', 'pending', 'canceling'], - }) - status; - - @resourceFilter({ - allowed: (route) => route.modelFor('scopes.scope.projects'), - serialize: ({ id }) => id, - findBySerialized: ({ id }, value) => id === value, - }) - project; + @service can; // =methods @@ -43,45 +27,6 @@ export default class ScopesScopeProjectsSessionsRoute extends Route { if (!this.session.isAuthenticated) this.router.transitionTo('index'); } - /** - * Loads all sessions under current scope for the current user. - * - * NOTE: previously, sessions were filtered only with API filter queries. - * In an effort to offload processing from the controller, sessions - * are now filtered on the client by projects and status, - * while user_id filtering remains server side. - * - * @return {Promise{[SessionModel]}} - */ - async model() { - const { id: scope_id } = this.modelFor('scopes.scope'); - const queryOptions = { - scope_id, - recursive: true, - include_terminated: this.status?.includes('terminated'), - }; - - // Recursively query sessions within the current scope for the current user - let sessions = await this.resourceFilterStore.queryBy( - 'session', - { user_id: this.session.data.authenticated.user_id }, - queryOptions - ); - - // If project filters are selected, filter sessions by the selected projects - if (this.project?.length) { - const projectIDs = this.project.map((p) => p?.id); - sessions = sessions.filter((s) => projectIDs.includes(s?.scopeID)); - } - - // If status filters are selected, filter sessions by the selected statuses - if (this.status?.length) { - sessions = sessions.filter((s) => this.status.includes(s?.status)); - } - - return sessions; - } - // =actions /** @@ -93,10 +38,14 @@ export default class ScopesScopeProjectsSessionsRoute extends Route { @notifyError(({ message }) => message, { catch: true }) @notifySuccess('notifications.canceled-success') async cancelSession(session) { + let updatedSession = session; // fetch session from API to verify we have most up to date record - const updatedSession = await this.store.findRecord('session', session.id, { - reload: true, - }); + if (this.can.can('read session', session)) { + updatedSession = await this.store.findRecord('session', session.id, { + reload: true, + }); + } + await updatedSession.cancelSession(); await this.ipc.invoke('stop', { session_id: session.id }); @@ -108,25 +57,6 @@ export default class ScopesScopeProjectsSessionsRoute extends Route { } } - /** - * Sets the specified resource filter field to the specified value. - * @param {string} field - * @param value - */ - @action - filterBy(field, value) { - this[field] = value; - } - - /** - * Clears and filter selections. - */ - @action - clearAllFilters() { - this.status = []; - this.project = []; - } - /** * refreshes session data. */ diff --git a/ui/desktop/app/routes/scopes/scope/projects/sessions/index.js b/ui/desktop/app/routes/scopes/scope/projects/sessions/index.js index 8f251194e..a41b08761 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/sessions/index.js +++ b/ui/desktop/app/routes/scopes/scope/projects/sessions/index.js @@ -4,5 +4,88 @@ */ import Route from '@ember/routing/route'; +import { action } from '@ember/object'; +import { inject as service } from '@ember/service'; +import { resourceFilter } from 'core/decorators/resource-filter'; -export default class ScopesScopeProjectsSessionsIndexRoute extends Route {} +export default class ScopesScopeProjectsSessionsIndexRoute extends Route { + // =services + + @service session; + @service resourceFilterStore; + + // =attributes + + @resourceFilter({ + allowed: ['active', 'pending', 'canceling', 'terminated'], + defaultValue: ['active', 'pending', 'canceling'], + }) + status; + + @resourceFilter({ + allowed: (route) => route.modelFor('scopes.scope.projects'), + serialize: ({ id }) => id, + findBySerialized: ({ id }, value) => id === value, + }) + project; + + // =methods + + /** + * Loads all sessions under current scope for the current user. + * + * NOTE: previously, sessions were filtered only with API filter queries. + * In an effort to offload processing from the controller, sessions + * are now filtered on the client by projects and status, + * while user_id filtering remains server side. + * + * @return {Promise{[SessionModel]}} + */ + async model() { + const { id: scope_id } = this.modelFor('scopes.scope'); + const queryOptions = { + scope_id, + recursive: true, + include_terminated: this.status?.includes('terminated'), + }; + + // Recursively query sessions within the current scope for the current user + let sessions = await this.resourceFilterStore.queryBy( + 'session', + { user_id: this.session.data.authenticated.user_id }, + queryOptions + ); + + // If project filters are selected, filter sessions by the selected projects + if (this.project?.length) { + const projectIDs = this.project.map((p) => p?.id); + sessions = sessions.filter((s) => projectIDs.includes(s?.scopeID)); + } + + // If status filters are selected, filter sessions by the selected statuses + if (this.status?.length) { + sessions = sessions.filter((s) => this.status.includes(s?.status)); + } + + return sessions; + } + + /** + * Sets the specified resource filter field to the specified value. + * @param {string} field + * @param value + */ + @action + filterBy(field, value) { + this[field] = value; + } + + /** + * Clears and filter selections. + */ + @action + clearAllFilters() { + this.status = []; + this.project = []; + } +} diff --git a/ui/desktop/app/routes/scopes/scope/projects/targets/target.js b/ui/desktop/app/routes/scopes/scope/projects/targets/target.js index f74d07f3b..5d2d19954 100644 --- a/ui/desktop/app/routes/scopes/scope/projects/targets/target.js +++ b/ui/desktop/app/routes/scopes/scope/projects/targets/target.js @@ -178,6 +178,7 @@ export default class ScopesScopeProjectsTargetsTargetRoute extends Route { id: session_id, proxy_address: address, proxy_port: port, + target_id: model.id, }, ], }); diff --git a/ui/desktop/app/templates/scopes/scope/projects/sessions/index.hbs b/ui/desktop/app/templates/scopes/scope/projects/sessions/index.hbs index c035a5cff..e01c4a1ce 100644 --- a/ui/desktop/app/templates/scopes/scope/projects/sessions/index.hbs +++ b/ui/desktop/app/templates/scopes/scope/projects/sessions/index.hbs @@ -18,7 +18,7 @@ {{#let - (resource-filter 'scopes.scope.projects.sessions' 'status') + (resource-filter 'scopes.scope.projects.sessions.index' 'status') as |filter| }} @@ -201,11 +201,12 @@ - {{#if B.data.isAvailable}} + {{#if (can 'cancel session' B.data)}} {{/if}} @@ -214,7 +215,7 @@ {{else if (has-resource-filter-selections - 'scopes.scope.projects.sessions' 'status' 'project' + 'scopes.scope.projects.sessions.index' 'status' 'project' )}} diff --git a/ui/desktop/app/templates/scopes/scope/projects/sessions/session.hbs b/ui/desktop/app/templates/scopes/scope/projects/sessions/session.hbs index 949943d37..55177d90a 100644 --- a/ui/desktop/app/templates/scopes/scope/projects/sessions/session.hbs +++ b/ui/desktop/app/templates/scopes/scope/projects/sessions/session.hbs @@ -15,13 +15,15 @@ - + {{#if (can 'cancel session' @model)}} + + {{/if}} diff --git a/ui/desktop/tests/acceptance/projects/sessions/index-test.js b/ui/desktop/tests/acceptance/projects/sessions/index-test.js index 2d052ed74..4be5c71a1 100644 --- a/ui/desktop/tests/acceptance/projects/sessions/index-test.js +++ b/ui/desktop/tests/acceptance/projects/sessions/index-test.js @@ -191,14 +191,7 @@ module('Acceptance | projects | sessions', function (hooks) { test('can link to an active session with read:self permissions', async function (assert) { assert.expect(1); - this.server.schema.sessions.all().destroy(); - this.server.create('session', { - authorized_actions: ['read:self'], - scope: instances.scopes.project, - target: instances.target, - status: STATUS_SESSION_ACTIVE, - user: instances.user, - }); + instances.session.update({ authorized_actions: ['read:self'] }); await visit(urls.sessions); @@ -222,6 +215,19 @@ module('Acceptance | projects | sessions', function (hooks) { .isVisible(); }); + test('can link to session even without read permissions', async function (assert) { + assert.expect(1); + instances.session.update({ authorized_actions: [] }); + + await visit(urls.sessions); + + assert + .dom( + 'tbody tr:first-child td:first-child [data-test-session-detail-link]' + ) + .isVisible(); + }); + test('cannot link to a canceling session', async function (assert) { assert.expect(2); instances.session.update({ status: STATUS_SESSION_CANCELING }); @@ -255,6 +261,38 @@ module('Acceptance | projects | sessions', function (hooks) { .isVisible(); }); + test('can cancel an active session with cancel permissions', async function (assert) { + assert.expect(1); + + await visit(urls.sessions); + + assert + .dom('tbody tr:first-child [data-test-session-cancel-button]') + .isVisible(); + }); + + test('can cancel an active session with cancel:self permissions', async function (assert) { + assert.expect(1); + instances.session.update({ authorized_actions: ['cancel:self'] }); + + await visit(urls.sessions); + + assert + .dom('tbody tr:first-child [data-test-session-cancel-button]') + .isVisible(); + }); + + test('cannot click cancel button without cancel permissions', async function (assert) { + assert.expect(1); + instances.session.update({ authorized_actions: [] }); + + await visit(urls.sessions); + + assert + .dom('tbody tr:first-child [data-test-session-cancel-button]') + .isNotVisible(); + }); + test('cancelling a session shows success alert', async function (assert) { assert.expect(1); stubs.ipcService.withArgs('stop'); diff --git a/ui/desktop/tests/acceptance/projects/sessions/session-test.js b/ui/desktop/tests/acceptance/projects/sessions/session-test.js index 2e6491a37..bcbf6a98a 100644 --- a/ui/desktop/tests/acceptance/projects/sessions/session-test.js +++ b/ui/desktop/tests/acceptance/projects/sessions/session-test.js @@ -123,6 +123,11 @@ module('Acceptance | projects | sessions | session', function (hooks) { stubs.ipcService = sinon.stub(ipcService, 'invoke'); }); + hooks.afterEach(function () { + // reset onUncaughtException to original state + QUnit.onUncaughtException = originalUncaughtException; + }); + test('visiting session detail', async function (assert) { assert.expect(1); @@ -132,11 +137,6 @@ module('Acceptance | projects | sessions | session', function (hooks) { assert.strictEqual(currentURL(), urls.session); }); - hooks.afterEach(function () { - // reset onUncaughtException to original state - QUnit.onUncaughtException = originalUncaughtException; - }); - test('visiting session with no credentials', async function (assert) { assert.expect(4); stubs.ipcService.withArgs('cliExists').returns(true); @@ -199,6 +199,24 @@ module('Acceptance | projects | sessions | session', function (hooks) { assert.strictEqual(currentURL(), urls.session); }); + test('can cancel a session with cancel:self permissions', async function (assert) { + assert.expect(1); + instances.session.update({ authorized_actions: ['cancel:self'] }); + + await visit(urls.session); + + assert.dom('[data-test-session-detail-cancel-button]').isVisible(); + }); + + test('cannot cancel a session without cancel permissions', async function (assert) { + assert.expect(1); + instances.session.update({ authorized_actions: [] }); + + await visit(urls.session); + + assert.dom('[data-test-session-detail-cancel-button]').isNotVisible(); + }); + test('cancelling a session shows success alert', async function (assert) { assert.expect(1); stubs.ipcService.withArgs('stop');