From 72be2dd204a4c9ed3c63f59de2544858c5f25c50 Mon Sep 17 00:00:00 2001 From: John Cowen Date: Thu, 4 Nov 2021 12:10:28 +0000 Subject: [PATCH] ui: Ensure we check intention service prefix permissions for per service (#11409) Port of: Ensure we check intention service prefix permissions for per service (#11270) Previously, when showing some action buttons for 'per service intentions' we used a global 'can I do something with any intention' permission to decide whether to show a certain button or not. If a user has a token that does not have 'global' intention permissions, but does have intention permissions on one or more specific services (for example via service / service_prefix), this meant that we did not show them certain buttons required to create/edit the intentions for this specific service. This PR adds that extra permissions check so we now check the intentions permissions per service instead of using the 'global' "can I edit intentions" question/request. **Notes:** - If a HTML button is `disabled` this means tippy.js doesn't adopt the popover properly and subsequently hide it from the user, so aswell as just disabling the button so you can't active the popover, we also don't even put the popover on the page - If `ability.item` or `ability.item.Resources` are empty then assume no access **We don't try to disable service > right hand side intention actions here** Whether you can create intentions for a service depends on the _destination_ of the intention you would like to create. For the topology view going from the LHS to the center, this is straightforwards as we only need to know the permissions for the central service, as when you are going from the LHS to the center, the center is the _destination_. When going from the center to the RHS the _destination[s]_ are on the RHS. This means we need to know the permissions for potentially 1000s of services all in one go in order to know when to show a button or not. We can't realistically discover the permissions for service > RHS services as we'd have either make a HTTP request per right hand service, or potentially make an incredibly large POST request for all the potentially 1000s of services on the right hand side (more preferable to 1000s of HTTP requests). Therefore for the moment at least we keep the old functionality (thin client) for the middle to RHS here. If you do go to click on the button and you don't have permissions to update the intention you will still not be able to update it, only you won't know this until you click the button (at which point you'll get a UI visible 403 error) Note: We reversed the conditional here between 1.10 and 1.11 So this make 100% sense that the port is different here to 1.11 --- .changelog/11409.txt | 4 + .../app/abilities/service-instance.js | 11 +- .../consul-ui/app/abilities/service.js | 28 ++ .../consul/intention/form/index.hbs | 2 +- .../topology-metrics/down-lines/index.hbs | 4 + .../topology-metrics/popover/index.hbs | 272 +++++++++--------- .../topology-metrics/up-lines/index.hbs | 2 +- .../consul-ui/app/services/repository.js | 5 +- .../services/repository/service-instance.js | 9 +- .../app/templates/dc/intentions/edit.hbs | 6 +- .../app/templates/dc/services/show.hbs | 3 +- .../dc/services/show/intentions/edit.hbs | 3 + .../dc/services/show/intentions/index.hbs | 5 +- 13 files changed, 213 insertions(+), 141 deletions(-) create mode 100644 .changelog/11409.txt diff --git a/.changelog/11409.txt b/.changelog/11409.txt new file mode 100644 index 000000000000..21fe036d509e --- /dev/null +++ b/.changelog/11409.txt @@ -0,0 +1,4 @@ +```release-note:bug +ui: Ensure we check intention permissions for specific services when deciding +whether to show action buttons for per service intention actions +``` diff --git a/ui/packages/consul-ui/app/abilities/service-instance.js b/ui/packages/consul-ui/app/abilities/service-instance.js index 628a92c75e19..07a11a41a330 100644 --- a/ui/packages/consul-ui/app/abilities/service-instance.js +++ b/ui/packages/consul-ui/app/abilities/service-instance.js @@ -1,5 +1,14 @@ -import BaseAbility from './base'; +import BaseAbility, { ACCESS_READ, ACCESS_WRITE } from './base'; export default class ServiceInstanceAbility extends BaseAbility { resource = 'service'; + generateForSegment(segment) { + // When we ask for service-instances its almost like a request for a single service + // When we do that we also want to know if we can read/write intentions for services + // so here we add intentions read/write for the specific segment/service prefix + return super.generateForSegment(...arguments).concat([ + this.permissions.generate('intention', ACCESS_READ, segment), + this.permissions.generate('intention', ACCESS_WRITE, segment), + ]); + } } diff --git a/ui/packages/consul-ui/app/abilities/service.js b/ui/packages/consul-ui/app/abilities/service.js index 5e806bd6f76d..8244f0b3f836 100644 --- a/ui/packages/consul-ui/app/abilities/service.js +++ b/ui/packages/consul-ui/app/abilities/service.js @@ -6,4 +6,32 @@ export default class ServiceAbility extends BaseAbility { get isLinkable() { return this.item.InstanceCount > 0; } + + get canReadIntention() { + if (typeof this.item === 'undefined' || typeof this.item.Resources === 'undefined') { + return false; + } + const found = this.item.Resources.find( + item => item.Resource === 'intention' && item.Access === 'read' && item.Allow === true + ); + return typeof found !== 'undefined'; + } + + get canWriteIntention() { + if (typeof this.item === 'undefined' || typeof this.item.Resources === 'undefined') { + return false; + } + const found = this.item.Resources.find( + item => item.Resource === 'intention' && item.Access === 'write' && item.Allow === true + ); + return typeof found !== 'undefined'; + } + + get canCreateIntention() { + return this.canWriteIntention; + } + + get canUpdateIntention() { + return this.canWriteIntention; + } } diff --git a/ui/packages/consul-ui/app/components/consul/intention/form/index.hbs b/ui/packages/consul-ui/app/components/consul/intention/form/index.hbs index 1806e28c79c3..98dc57c23907 100644 --- a/ui/packages/consul-ui/app/components/consul/intention/form/index.hbs +++ b/ui/packages/consul-ui/app/components/consul/intention/form/index.hbs @@ -33,7 +33,7 @@ as |api|> {{#let api.data as |item|}} -{{#if (can 'write intention' item=item)}} +{{#if (not readonly)}} {{#let (changeset-get item 'Action') as |newAction|}} {{/if}} +{{#let (not (can 'update intention for service' item=@service.Service)) as |disabled|}} {{#each @items as |item|}} {{#if (or (not item.Intention.Allowed) item.Intention.HasPermissions)}} {{else if (and item.Intention.Allowed (not item.TransparentProxy) (eq item.Source 'specific-intention'))}} @@ -96,8 +98,10 @@ @service={{@service}} @position={{find-by 'id' (concat this.guid item.Namespace item.Name) this.iconPositions}} @item={{item}} + @disabled={{disabled}} @oncreate={{action @oncreate item @service}} /> {{/if}} {{/each}} +{{/let}} diff --git a/ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs b/ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs index ccd29d8eb2be..96f237cba482 100644 --- a/ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs +++ b/ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs @@ -2,133 +2,147 @@ class="topology-metrics-popover {{@type}}" ...attributes > - -{{#if (eq @type 'deny')}} - - <:header> -

- {{t "components.consul.topology-metrics.popover.deny.header"}} -

- - <:body> -

- {{#if @item.Intention.HasExact}} - {{t "components.consul.topology-metrics.popover.deny.body.isExact"}} - {{else}} - {{t "components.consul.topology-metrics.popover.deny.body.notExact"}} - {{/if}} -

- - <:actions as |Actions|> - - - - - - - - -{{else if (eq @type 'not-defined')}} - - <:header> -

- {{t "components.consul.topology-metrics.popover.not-defined.header"}} -

- - <:body> -

- {{t "components.consul.topology-metrics.popover.not-defined.body" downstream=@item.Name upstream=@service.Name }} -

- - <:actions as |Actions|> - - - {{t "components.consul.topology-metrics.popover.not-defined.action"}} - - - - - - - -{{else}} - - <:header> -

- {{t "components.consul.topology-metrics.popover.l7.header"}} -

- - <:body> -

- {{t "components.consul.topology-metrics.popover.l7.body"}} -

- - <:actions as |Actions|> - - - {{t "components.consul.topology-metrics.popover.l7.action"}} - - - - - - - -{{/if}} - +{{#let + (concat 'top:' @position.y 'px;left:' @position.x 'px;') + (if (eq @type 'deny') 'Add intention' 'View intention') +as |style label|}} + {{#if (not @disabled)}} + {{#if (eq @type 'deny')}} + + <:header> +

+ {{t "components.consul.topology-metrics.popover.deny.header"}} +

+ + <:body> +

+ {{#if @item.Intention.HasExact}} + {{t "components.consul.topology-metrics.popover.deny.body.isExact"}} + {{else}} + {{t "components.consul.topology-metrics.popover.deny.body.notExact"}} + {{/if}} +

+ + <:actions as |Actions|> + + + + + + + + + {{else if (eq @type 'not-defined')}} + + <:header> +

+ {{t "components.consul.topology-metrics.popover.not-defined.header"}} +

+ + <:body> +

+ {{t "components.consul.topology-metrics.popover.not-defined.body" downstream=@item.Name upstream=@service.Name }} +

+ + <:actions as |Actions|> + + + {{t "components.consul.topology-metrics.popover.not-defined.action"}} + + + + + + + + {{else}} + + <:header> +

+ {{t "components.consul.topology-metrics.popover.l7.header"}} +

+ + <:body> +

+ {{t "components.consul.topology-metrics.popover.l7.body"}} +

+ + <:actions as |Actions|> + + + {{t "components.consul.topology-metrics.popover.l7.action"}} + + + + + + + + {{/if}} + {{! TODO: Once we can conditionally add a modifier these two buttons }} + {{! should be replaced with a conditional modifer instead }} + + {{else}} + + {{/if}} +{{/let}} - - diff --git a/ui/packages/consul-ui/app/components/topology-metrics/up-lines/index.hbs b/ui/packages/consul-ui/app/components/topology-metrics/up-lines/index.hbs index 7f73d5c5654c..33b9e59e483e 100644 --- a/ui/packages/consul-ui/app/components/topology-metrics/up-lines/index.hbs +++ b/ui/packages/consul-ui/app/components/topology-metrics/up-lines/index.hbs @@ -82,13 +82,13 @@ {{/each}} {{/if}} - {{#each @items as |item|}} {{#if (or (not item.Intention.Allowed) item.Intention.HasPermissions)}} {{/if}} diff --git a/ui/packages/consul-ui/app/services/repository.js b/ui/packages/consul-ui/app/services/repository.js index 78920b2b8262..cf7ecd12ea75 100644 --- a/ui/packages/consul-ui/app/services/repository.js +++ b/ui/packages/consul-ui/app/services/repository.js @@ -79,9 +79,12 @@ export default class RepositoryService extends Service { throw e; } } - const item = await cb(); + const item = await cb(params.resources); // add the `Resource` information to the record/model so we can inspect // them in other places like templates etc + // TODO: We mostly use this to authorize single items but we do + // occasionally get an array back here e.g. service-instances, so we + // should make this fact more obvious if (get(item, 'Resources')) { set(item, 'Resources', params.resources); } diff --git a/ui/packages/consul-ui/app/services/repository/service-instance.js b/ui/packages/consul-ui/app/services/repository/service-instance.js index 25e65f762190..d69fc067def0 100644 --- a/ui/packages/consul-ui/app/services/repository/service-instance.js +++ b/ui/packages/consul-ui/app/services/repository/service-instance.js @@ -21,12 +21,15 @@ export default class ServiceInstanceService extends RepositoryService { params.index = configuration.cursor; params.uri = configuration.uri; } - const instances = await this.authorizeBySlug( - async () => this.query(params), + return this.authorizeBySlug( + async (resources) => { + const instances = await this.query(params); + set(instances, 'firstObject.Service.Resources', resources); + return instances; + }, ACCESS_READ, params ); - return instances; } @dataSource('/:partition/:ns/:dc/service-instance/:serviceId/:node/:id') diff --git a/ui/packages/consul-ui/app/templates/dc/intentions/edit.hbs b/ui/packages/consul-ui/app/templates/dc/intentions/edit.hbs index bc931c5ea38f..2139fd73186f 100644 --- a/ui/packages/consul-ui/app/templates/dc/intentions/edit.hbs +++ b/ui/packages/consul-ui/app/templates/dc/intentions/edit.hbs @@ -22,7 +22,8 @@ as |route|> {{#let loader.data -as |item|}} + (not (can "write intention" item=item)) +as |item readOnly|}}
    @@ -31,7 +32,7 @@ as |item|}}

    - {{#if (can "write intention" item=item)}} + {{#if (not readOnly)}} {{#if item.ID}} {{else}} @@ -44,6 +45,7 @@ as |item|}} diff --git a/ui/packages/consul-ui/app/templates/dc/services/show/intentions/edit.hbs b/ui/packages/consul-ui/app/templates/dc/services/show/intentions/edit.hbs index f3980e804021..44bbc9d03703 100644 --- a/ui/packages/consul-ui/app/templates/dc/services/show/intentions/edit.hbs +++ b/ui/packages/consul-ui/app/templates/dc/services/show/intentions/edit.hbs @@ -1,14 +1,17 @@ +{{#let (not (can "write intention for service" item=item.Service)) as |readOnly|}} +{{/let}} diff --git a/ui/packages/consul-ui/app/templates/dc/services/show/intentions/index.hbs b/ui/packages/consul-ui/app/templates/dc/services/show/intentions/index.hbs index a2a4b26250f3..87d2f96492dd 100644 --- a/ui/packages/consul-ui/app/templates/dc/services/show/intentions/index.hbs +++ b/ui/packages/consul-ui/app/templates/dc/services/show/intentions/index.hbs @@ -38,10 +38,11 @@ as |route|> ) api.data + route.model.item - as |sort filters items|}} + as |sort filters items item|}}
    - {{#if (can 'create intentions')}} + {{#if (can 'create intention for service' item=item.Service)}} Create