Skip to content

Commit

Permalink
ui: Ensure we check intention service prefix permissions for per serv…
Browse files Browse the repository at this point in the history
…ice (#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
  • Loading branch information
johncowen committed Nov 4, 2021
1 parent 8f80553 commit 72be2dd
Show file tree
Hide file tree
Showing 13 changed files with 213 additions and 141 deletions.
4 changes: 4 additions & 0 deletions .changelog/11409.txt
Original file line number Diff line number Diff line change
@@ -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
```
11 changes: 10 additions & 1 deletion ui/packages/consul-ui/app/abilities/service-instance.js
Original file line number Diff line number Diff line change
@@ -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),
]);
}
}
28 changes: 28 additions & 0 deletions ui/packages/consul-ui/app/abilities/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ as |api|>

<BlockSlot @name="form">
{{#let api.data as |item|}}
{{#if (can 'write intention' item=item)}}
{{#if (not readonly)}}

{{#let (changeset-get item 'Action') as |newAction|}}
<ModalDialog
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,12 +82,14 @@
</svg>
{{/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)}}
<TopologyMetrics::Popover
@type={{if item.Intention.HasPermissions 'l7' 'deny'}}
@position={{find-by 'id' (concat this.guid item.Namespace item.Name) this.iconPositions}}
@item={{item}}
@disabled={{disabled}}
@oncreate={{action @oncreate item @service}}
/>
{{else if (and item.Intention.Allowed (not item.TransparentProxy) (eq item.Source 'specific-intention'))}}
Expand All @@ -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}}

272 changes: 143 additions & 129 deletions ui/packages/consul-ui/app/components/topology-metrics/popover/index.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -2,133 +2,147 @@
class="topology-metrics-popover {{@type}}"
...attributes
>

{{#if (eq @type 'deny')}}
<InformedAction
class="dangerous"
{{did-insert (set this 'popover')}}
>
<:header>
<h3>
{{t "components.consul.topology-metrics.popover.deny.header"}}
</h3>
</:header>
<:body>
<p>
{{#if @item.Intention.HasExact}}
{{t "components.consul.topology-metrics.popover.deny.body.isExact"}}
{{else}}
{{t "components.consul.topology-metrics.popover.deny.body.notExact"}}
{{/if}}
</p>
</:body>
<:actions as |Actions|>
<Actions.Action class="action">
<button
{{on "click" @oncreate}}
data-test-confirm
type="button"
>
{{#if @item.Intention.HasExact}}
{{t "components.consul.topology-metrics.popover.deny.action.isExact"}}
{{else}}
{{t "components.consul.topology-metrics.popover.deny.action.notExact"}}
{{/if}}
</button>
</Actions.Action>
<Actions.Action>
<button
{{on 'click' (fn (optional this.popoverController.hide))}}
class="cancel"
type="button"
>
Cancel
</button>
</Actions.Action>
</:actions>
</InformedAction>
{{else if (eq @type 'not-defined')}}
<InformedAction
class="warning documentation"
{{did-insert (set this 'popover')}}
>
<:header>
<h3>
{{t "components.consul.topology-metrics.popover.not-defined.header"}}
</h3>
</:header>
<:body>
<p>
{{t "components.consul.topology-metrics.popover.not-defined.body" downstream=@item.Name upstream=@service.Name }}
</p>
</:body>
<:actions as |Actions|>
<Actions.Action class="action">
<a href="{{env 'CONSUL_DOCS_URL'}}/connect/registration/service-registration#upstreams" rel="noopener noreferrer" target="_blank">
{{t "components.consul.topology-metrics.popover.not-defined.action"}}
</a>
</Actions.Action>
<Actions.Action>
<button
{{on 'click' (fn (optional this.popoverController.hide))}}
class="cancel"
type="button"
>
Close
</button>
</Actions.Action>
</:actions>
</InformedAction>
{{else}}
<InformedAction
class="info"
{{did-insert (set this 'popover')}}
>
<:header>
<h3>
{{t "components.consul.topology-metrics.popover.l7.header"}}
</h3>
</:header>
<:body>
<p>
{{t "components.consul.topology-metrics.popover.l7.body"}}
</p>
</:body>
<:actions as |Actions|>
<Actions.Action class="action">
<a href={{href-to 'dc.services.show.intentions.edit' (concat @item.Intention.ID)}}>
{{t "components.consul.topology-metrics.popover.l7.action"}}
</a>
</Actions.Action>
<Actions.Action>
<button
{{on 'click' (fn (optional this.popoverController.hide))}}
class="cancel"
type="button"
>
Close
</button>
</Actions.Action>
</:actions>
</InformedAction>
{{/if}}
<button
{{with-overlay
this.popover
options=(hash
theme="square-tail"
placement="bottom-start"
)
returns=(set this 'popoverController')
}}
{{on 'click' (fn (optional this.popoverController.show))}}
{{disabled (cannot 'update intention' item=item.Intention)}}
type="button"
style={{{concat 'top:' @position.y 'px;left:' @position.x 'px;'}}}
aria-label={{if (eq @type 'deny') 'Add intention' 'View intention'}}
data-test-action
>
</button>
{{#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')}}
<InformedAction
class="dangerous"
{{did-insert (set this 'popover')}}
>
<:header>
<h3>
{{t "components.consul.topology-metrics.popover.deny.header"}}
</h3>
</:header>
<:body>
<p>
{{#if @item.Intention.HasExact}}
{{t "components.consul.topology-metrics.popover.deny.body.isExact"}}
{{else}}
{{t "components.consul.topology-metrics.popover.deny.body.notExact"}}
{{/if}}
</p>
</:body>
<:actions as |Actions|>
<Actions.Action class="action">
<button
{{on "click" @oncreate}}
data-test-confirm
type="button"
>
{{#if @item.Intention.HasExact}}
{{t "components.consul.topology-metrics.popover.deny.action.isExact"}}
{{else}}
{{t "components.consul.topology-metrics.popover.deny.action.notExact"}}
{{/if}}
</button>
</Actions.Action>
<Actions.Action>
<button
{{on 'click' (fn (optional this.popoverController.hide))}}
class="cancel"
type="button"
>
Cancel
</button>
</Actions.Action>
</:actions>
</InformedAction>
{{else if (eq @type 'not-defined')}}
<InformedAction
class="warning documentation"
{{did-insert (set this 'popover')}}
>
<:header>
<h3>
{{t "components.consul.topology-metrics.popover.not-defined.header"}}
</h3>
</:header>
<:body>
<p>
{{t "components.consul.topology-metrics.popover.not-defined.body" downstream=@item.Name upstream=@service.Name }}
</p>
</:body>
<:actions as |Actions|>
<Actions.Action class="action">
<a href="{{env 'CONSUL_DOCS_URL'}}/connect/registration/service-registration#upstreams" rel="noopener noreferrer" target="_blank">
{{t "components.consul.topology-metrics.popover.not-defined.action"}}
</a>
</Actions.Action>
<Actions.Action>
<button
{{on 'click' (fn (optional this.popoverController.hide))}}
class="cancel"
type="button"
>
Close
</button>
</Actions.Action>
</:actions>
</InformedAction>
{{else}}
<InformedAction
class="info"
{{did-insert (set this 'popover')}}
>
<:header>
<h3>
{{t "components.consul.topology-metrics.popover.l7.header"}}
</h3>
</:header>
<:body>
<p>
{{t "components.consul.topology-metrics.popover.l7.body"}}
</p>
</:body>
<:actions as |Actions|>
<Actions.Action class="action">
<a href={{href-to 'dc.services.show.intentions.edit' (concat @item.Intention.ID)}}>
{{t "components.consul.topology-metrics.popover.l7.action"}}
</a>
</Actions.Action>
<Actions.Action>
<button
{{on 'click' (fn (optional this.popoverController.hide))}}
class="cancel"
type="button"
>
Close
</button>
</Actions.Action>
</:actions>
</InformedAction>
{{/if}}
{{! TODO: Once we can conditionally add a modifier these two buttons }}
{{! should be replaced with a conditional modifer instead }}
<button
{{with-overlay
this.popover
options=(hash
theme="square-tail"
placement="bottom-start"
)
returns=(set this 'popoverController')
}}
{{on 'click' (fn (optional this.popoverController.show))}}
type="button"
style={{{style}}}
aria-label={{label}}
data-test-action
>
</button>
{{else}}
<button
{{disabled true}}
type="button"
style={{{style}}}
aria-label={{label}}
data-test-action
>
</button>
{{/if}}
{{/let}}
</div>


Original file line number Diff line number Diff line change
Expand Up @@ -82,13 +82,13 @@
{{/each}}
</svg>
{{/if}}

{{#each @items as |item|}}
{{#if (or (not item.Intention.Allowed) item.Intention.HasPermissions)}}
<TopologyMetrics::Popover
@type={{if item.Intention.HasPermissions 'l7' 'deny'}}
@position={{find-by 'id' (concat this.guid item.Namespace item.Name) this.iconPositions}}
@item={{item}}
@disabled={{false}}
@oncreate={{action @oncreate @service item}}
/>
{{/if}}
Expand Down
5 changes: 4 additions & 1 deletion ui/packages/consul-ui/app/services/repository.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

0 comments on commit 72be2dd

Please sign in to comment.