From a3c0029a997b5016bd770e5231dc600f46d9ce79 Mon Sep 17 00:00:00 2001 From: ccremer Date: Fri, 14 Apr 2023 16:32:50 +0200 Subject: [PATCH] BE members: Fix not being able to modify members --- cypress/e2e/billingentity-members.cy.ts | 99 +------------- cypress/fixtures/clusterrole.ts | 25 ---- .../billing-entity.component.html | 2 +- .../billingentity/billing-entity.component.ts | 6 +- .../billing-entity-members.component.html | 12 +- .../billing-entity-members.component.ts | 128 ++++-------------- .../store/cluster-role-collection.service.ts | 14 -- src/app/store/entity-metadata-map.ts | 6 - src/app/types/clusterRole.ts | 10 -- 9 files changed, 34 insertions(+), 268 deletions(-) delete mode 100644 cypress/fixtures/clusterrole.ts delete mode 100644 src/app/store/cluster-role-collection.service.ts delete mode 100644 src/app/types/clusterRole.ts diff --git a/cypress/e2e/billingentity-members.cy.ts b/cypress/e2e/billingentity-members.cy.ts index c5685cef..dd8bd582 100644 --- a/cypress/e2e/billingentity-members.cy.ts +++ b/cypress/e2e/billingentity-members.cy.ts @@ -3,10 +3,8 @@ import { BillingEntityPermissions } from '../../src/app/types/billing-entity'; import { ClusterRoleBinding, ClusterRoleBindingPermissions } from '../../src/app/types/clusterrole-binding'; import { billingEntityNxt } from '../fixtures/billingentities'; import { createClusterRoleBinding } from '../fixtures/clusterrole-binding'; -import { createClusterRole } from '../fixtures/clusterrole'; -import { ClusterRolePermissions } from '../../src/app/types/clusterRole'; -describe('billing entity edit members with existing roles', () => { +describe('billing entity edit members', () => { beforeEach(() => { cy.setupAuth(); window.localStorage.setItem('hideFirstTimeLoginDialog', 'true'); @@ -20,17 +18,9 @@ describe('billing entity edit members with existing roles', () => { cy.setPermission( { verb: 'list', ...BillingEntityPermissions }, { verb: 'get', ...BillingEntityPermissions, name: 'be-2345' }, - { verb: 'get', ...ClusterRoleBindingPermissions, name: 'billingentities-be-2345-viewer' }, - { verb: 'get', ...ClusterRoleBindingPermissions, name: 'billingentities-be-2345-admin' }, { verb: 'update', ...ClusterRoleBindingPermissions, name: 'billingentities-be-2345-viewer' }, { verb: 'update', ...ClusterRoleBindingPermissions, name: 'billingentities-be-2345-admin' } ); - cy.intercept('GET', 'appuio-api/apis/rbac.authorization.k8s.io/v1/clusterroles/billingentities-be-2345-admin', { - body: createClusterRole('be-2345', true), - }); - cy.intercept('GET', 'appuio-api/apis/rbac.authorization.k8s.io/v1/clusterroles/billingentities-be-2345-viewer', { - body: createClusterRole('be-2345', false), - }); }); it('add member', () => { @@ -290,90 +280,3 @@ describe('billing entity edit members with existing roles', () => { cy.get('p-message').contains('You are about to remove yourself as admin!'); }); }); - -describe('billing entity edit members without initial roles', () => { - beforeEach(() => { - cy.setupAuth(); - window.localStorage.setItem('hideFirstTimeLoginDialog', 'true'); - cy.disableCookieBanner(); - }); - beforeEach(() => { - // needed for initial getUser request - cy.intercept('GET', 'appuio-api/apis/appuio.io/v1/users/mig', { - body: createUser({ username: 'mig', defaultOrganizationRef: 'nxt' }), - }); - cy.setPermission( - { verb: 'list', ...BillingEntityPermissions }, - { verb: 'get', ...BillingEntityPermissions, name: 'be-2345' }, - { verb: 'get', ...ClusterRoleBindingPermissions, name: 'billingentities-be-2345-viewer' }, - { verb: 'update', ...ClusterRoleBindingPermissions, name: 'billingentities-be-2345-admin' }, - { verb: 'update', ...ClusterRoleBindingPermissions, name: 'billingentities-be-2345-viewer' }, - { verb: 'update', ...ClusterRolePermissions, name: 'billingentities-be-2345-admin' } - ); - cy.intercept('GET', 'appuio-api/apis/rbac.authorization.k8s.io/v1/clusterroles/billingentities-be-2345-admin', { - statusCode: 404, - }); - cy.intercept('GET', 'appuio-api/apis/rbac.authorization.k8s.io/v1/clusterroles/billingentities-be-2345-viewer', { - statusCode: 404, - }); - cy.intercept( - 'GET', - 'appuio-api/apis/rbac.authorization.k8s.io/v1/clusterrolebindings/billingentities-be-2345-viewer', - { - statusCode: 404, - } - ); - cy.intercept( - 'GET', - 'appuio-api/apis/rbac.authorization.k8s.io/v1/clusterrolebindings/billingentities-be-2345-admin', - { - statusCode: 404, - } - ); - }); - - it('add member', () => { - cy.intercept('GET', 'appuio-api/apis/billing.appuio.io/v1/billingentities/be-2345', { - body: billingEntityNxt, - }); - cy.intercept('POST', 'appuio-api/apis/rbac.authorization.k8s.io/v1/clusterroles', (req) => { - if (req.body.metadata.name.includes('admin')) { - expect(req.body.rules).to.have.length(2); - const rule = req.body.rules && req.body.rules[0]; - expect(rule && rule.resourceNames).to.include('billingentities-be-2345-admin'); - expect(rule && rule.verbs).to.eql(['*']); - - req.reply(createClusterRole('be-2345', true)); - return; - } - if (req.body.metadata.name.includes('viewer')) { - const rule = req.body.rules && req.body.rules[0]; - expect(rule && rule.resourceNames).to.include('billingentities-be-2345-viewer'); - expect(rule && rule.verbs).to.eql(['get', 'watch']); - - req.reply(createClusterRole('be-2345', false)); - return; - } - }).as('createRole'); - - cy.intercept('POST', 'appuio-api/apis/rbac.authorization.k8s.io/v1/clusterrolebindings', (req) => { - expect(req.body.subjects).to.have.length(1); - const subject = req.body.subjects && req.body.subjects[0]; - expect(subject && subject.name).to.eq('appuio#crc'); - - if (req.body.metadata.name.includes('admin')) { - req.reply(createClusterRoleBinding({ name: 'billingentities-be-2345-admin', users: ['appuio#crc'] })); - } - if (req.body.metadata.name.includes('viewer')) { - req.reply(createClusterRoleBinding({ name: 'billingentities-be-2345-viewer', users: ['appuio#crc'] })); - } - }).as('createRoleBinding'); - - cy.visit('/billingentities/be-2345/members'); - cy.get('.text-3xl').should('contain.text', 'be-2345 Members'); - cy.get('[data-cy="name-input-0"]').type('crc'); - cy.get('p-multiselect').eq(0).click().contains('billingentities-be-2345-admin').click(); - cy.get('button[type=submit]').click(); - cy.wait(['@createRole', '@createRoleBinding']); - }); -}); diff --git a/cypress/fixtures/clusterrole.ts b/cypress/fixtures/clusterrole.ts deleted file mode 100644 index dc4558a7..00000000 --- a/cypress/fixtures/clusterrole.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { ClusterRole } from '../../src/app/types/clusterRole'; - -export function createClusterRole(beName: string, admin: boolean): ClusterRole { - return { - apiVersion: 'rbac.authorization.k8s.io/v1', - kind: 'ClusterRole', - metadata: { - name: `billingentities-${beName}-${admin ? 'admin' : 'viewer'}`, - }, - rules: [ - { - verbs: admin ? ['get', 'update', 'create', 'watch', 'patch', 'delete'] : ['get', 'watch'], - apiGroups: ['rbac.authorization.k8s.io'], - resources: ['clusterrolebindings'], - resourceNames: [`billingentities-${beName}-${admin ? 'admin' : 'viewer'}`], - }, - { - verbs: ['get'], - apiGroups: ['rbac.appuio.io'], - resources: ['billingentities'], - resourceNames: [beName], - }, - ], - }; -} diff --git a/src/app/billingentity/billing-entity.component.html b/src/app/billingentity/billing-entity.component.html index c60f1ee7..df1dfea6 100644 --- a/src/app/billingentity/billing-entity.component.html +++ b/src/app/billingentity/billing-entity.component.html @@ -39,7 +39,7 @@

Billing

{{ p.billingEntity.metadata.name }}
- diff --git a/src/app/billingentity/billing-entity.component.ts b/src/app/billingentity/billing-entity.component.ts index 8d47d531..0a29386c 100644 --- a/src/app/billingentity/billing-entity.component.ts +++ b/src/app/billingentity/billing-entity.component.ts @@ -57,10 +57,10 @@ export class BillingEntityComponent implements OnInit { this.billingEntityService.canEditMembers(`billingentities-${be.metadata.name}-admin`), this.billingEntityService.canEditBilling(be.metadata.name), ]).pipe( - map(([billingEntity, canViewMembers, canEdit]) => { + map(([billingEntity, canEditMembers, canEdit]) => { return { billingEntity, - canViewMembers, + canEditMembers, canEdit, } satisfies BillingModel; }) @@ -91,5 +91,5 @@ interface ViewModel { interface BillingModel { billingEntity: BillingEntity; canEdit: boolean; - canViewMembers: boolean; + canEditMembers: boolean; } diff --git a/src/app/billingentity/billingentity-members/billing-entity-members.component.html b/src/app/billingentity/billingentity-members/billing-entity-members.component.html index 71b80cc2..a0f35132 100644 --- a/src/app/billingentity/billingentity-members/billing-entity-members.component.html +++ b/src/app/billingentity/billingentity-members/billing-entity-members.component.html @@ -1,9 +1,9 @@ - - + +
- {{ payload.billingEntity.metadata.name }} + {{ vm.billingEntity.metadata.name }} Members
@@ -12,7 +12,7 @@
-
+
Roles
- diff --git a/src/app/billingentity/billingentity-members/billing-entity-members.component.ts b/src/app/billingentity/billingentity-members/billing-entity-members.component.ts index 8078bbba..ad37de6f 100644 --- a/src/app/billingentity/billingentity-members/billing-entity-members.component.ts +++ b/src/app/billingentity/billingentity-members/billing-entity-members.component.ts @@ -2,7 +2,7 @@ import { ChangeDetectionStrategy, Component, OnDestroy, OnInit } from '@angular/ import { ActivatedRoute, Router } from '@angular/router'; import { BillingEntityCollectionService } from '../../store/billingentity-collection.service'; import { BillingEntity } from '../../types/billing-entity'; -import { catchError, forkJoin, map, Observable, Subscription, take } from 'rxjs'; +import { forkJoin, map, Observable, Subscription, take } from 'rxjs'; import { faClose, faSave, faWarning } from '@fortawesome/free-solid-svg-icons'; import { FormArray, FormControl, FormGroup, Validators } from '@angular/forms'; import { ClusterRoleBinding } from '../../types/clusterrole-binding'; @@ -11,20 +11,8 @@ import { MessageService } from 'primeng/api'; import { UserCollectionService } from '../../store/user-collection.service'; import { User } from 'src/app/types/user'; import { switchMap } from 'rxjs/operators'; -import { defaultIfNotFound } from '../../store/kubernetes-collection.service'; -import { ClusterRoleCollectionService } from '../../store/cluster-role-collection.service'; -import { ClusterRole } from '../../types/clusterRole'; -import { KubeObject } from '../../types/entity'; import { NavigationService } from '../../shared/navigation.service'; -interface Payload { - billingEntity: BillingEntity; - viewerRole: ClusterRole; - adminRole: ClusterRole; - adminBinding: ClusterRoleBinding; - viewerBinding: ClusterRoleBinding; -} - @Component({ selector: 'app-billingentity-members', templateUrl: './billing-entity-members.component.html', @@ -32,7 +20,7 @@ interface Payload { changeDetection: ChangeDetectionStrategy.OnPush, }) export class BillingEntityMembersComponent implements OnInit, OnDestroy { - payload$?: Observable; + viewModel$?: Observable; faWarning = faWarning; faClose = faClose; @@ -57,8 +45,7 @@ export class BillingEntityMembersComponent implements OnInit, OnDestroy { private router: Router, private navigationService: NavigationService, private billingService: BillingEntityCollectionService, - private roleService: ClusterRoleCollectionService, - public rolebindingService: ClusterRolebindingCollectionService, + public roleBindingService: ClusterRolebindingCollectionService, private messageService: MessageService, private userService: UserCollectionService ) {} @@ -76,59 +63,40 @@ export class BillingEntityMembersComponent implements OnInit, OnDestroy { // -> Check if we're allowed to do something and error handling // Second, we enrich the pipe with more entities, based on whether we're allowed to. // Last, map the results to the payload and render the UI based on those. - this.payload$ = forkJoin([ + this.viewModel$ = forkJoin([ this.billingService.canViewBilling(beName), - this.billingService.canEditMembers(viewerClusterRoleBindingName), this.billingService.canEditMembers(adminClusterRoleBindingName), ]).pipe( - switchMap(([canViewBE, canEditViewer, canEditAdmins]) => { + switchMap(([canViewBE, canEditAdmins]) => { if (!canViewBE) { this.messageService.add({ severity: 'error', summary: `You don't have permissions to view Billing ${beName}.`, }); - this.router.navigateByUrl('/home'); + void this.router.navigateByUrl('/home'); } - if (!canEditViewer || !canEditAdmins) { + if (!canEditAdmins) { this.messageService.add({ severity: 'error', summary: `You don't have enough permissions to edit members of Billing ${beName}.`, }); - this.router.navigateByUrl('/home'); + void this.router.navigateByUrl('/home'); } return forkJoin([ this.billingService.getByKeyMemoized(beName), - this.rolebindingService - .getByKeyMemoized(adminClusterRoleBindingName) - .pipe(catchError(defaultIfNotFound(this.newRoleBinding(adminClusterRoleBindingName))), take(1)), - this.rolebindingService - .getByKeyMemoized(viewerClusterRoleBindingName) - .pipe(catchError(defaultIfNotFound(this.newRoleBinding(viewerClusterRoleBindingName))), take(1)), + this.roleBindingService.getByKeyMemoized(adminClusterRoleBindingName), + this.roleBindingService.getByKeyMemoized(viewerClusterRoleBindingName), this.userService.currentUser$.pipe(take(1)), - this.createClusterRole$( - adminClusterRoleBindingName, - beName, - ['*'], - [adminClusterRoleBindingName, viewerClusterRoleBindingName] - ), - this.createClusterRole$( - viewerClusterRoleBindingName, - beName, - ['get', 'watch'], - [viewerClusterRoleBindingName] - ), ]); }), - map(([billingEntity, adminBinding, viewerBinding, currentUser, adminRole, viewerRole]) => { + map(([billingEntity, adminBinding, viewerBinding, currentUser]) => { this.currentUser = currentUser; const payload = { billingEntity, adminBinding, - adminRole, viewerBinding, - viewerRole, - } satisfies Payload; + } satisfies ViewModel; this.initForm(payload); return payload; }) @@ -139,7 +107,7 @@ export class BillingEntityMembersComponent implements OnInit, OnDestroy { return this.form?.get('userRefs') as FormArray; } - initForm(payload: Payload): void { + initForm(payload: ViewModel): void { const userRoles = this.mapRolesToUsers([payload.viewerBinding, payload.adminBinding]); const members: FormGroup[] = []; @@ -214,28 +182,22 @@ export class BillingEntityMembersComponent implements OnInit, OnDestroy { }); } - save(payload: Payload): void { + save(payload: ViewModel): void { if (!this.form) { return; } const bindingClones = [payload.adminBinding, payload.viewerBinding].map((binding) => structuredClone(binding)); - const roleClones = [payload.adminRole, payload.viewerRole].map((role) => structuredClone(role)); this.mapUsersToRoles(bindingClones, this.form.getRawValue().userRefs); - const upsert$: Observable[] = []; - bindingClones.forEach((binding) => { - upsert$.push(this.rolebindingService.upsert(binding)); - }); - roleClones.forEach((role) => { - upsert$.push(this.roleService.upsert(role)); - }); - forkJoin(upsert$).subscribe({ + const update$ = bindingClones.map((binding) => this.roleBindingService.update(binding)); + forkJoin(update$).subscribe({ next: () => { this.messageService.add({ severity: 'success', summary: $localize`Successfully saved`, }); - void this.router.navigate([this.navigationService.previousLocation()], { relativeTo: this.route }); + const previous = this.navigationService.previousRoute('..'); + void this.router.navigate([previous.path], { relativeTo: this.route }); }, error: (error) => { this.messageService.add({ @@ -266,54 +228,10 @@ export class BillingEntityMembersComponent implements OnInit, OnDestroy { ngOnDestroy(): void { this.subscriptions.forEach((sub) => sub.unsubscribe()); } +} - private newRoleBinding(clusterRoleBindingName: string): ClusterRoleBinding { - return { - apiVersion: 'rbac.authorization.k8s.io/v1', - kind: 'ClusterRoleBinding', - metadata: { - name: clusterRoleBindingName, - }, - roleRef: { - name: clusterRoleBindingName, - kind: 'ClusterRole', - apiGroup: 'rbac.authorization.k8s.io', - }, - }; - } - - private createClusterRole$( - bindingName: string, - beName: string, - verbs: string[], - resourceNames: string[] - ): Observable { - return this.roleService - .getByKeyMemoized(bindingName) - .pipe(catchError(defaultIfNotFound(this.newRole(bindingName, beName, verbs, resourceNames))), take(1)); - } - - private newRole(roleName: string, beName: string, verbs: string[], resourceNames: string[]): ClusterRole { - return { - apiVersion: 'rbac.authorization.k8s.io/v1', - kind: 'ClusterRole', - metadata: { - name: roleName, - }, - rules: [ - { - verbs: verbs, - apiGroups: ['rbac.authorization.k8s.io'], - resources: ['clusterrolebindings'], - resourceNames: resourceNames, - }, - { - verbs: ['get'], - apiGroups: ['rbac.appuio.io'], - resources: ['billingentities'], - resourceNames: [beName], - }, - ], - }; - } +interface ViewModel { + billingEntity: BillingEntity; + adminBinding: ClusterRoleBinding; + viewerBinding: ClusterRoleBinding; } diff --git a/src/app/store/cluster-role-collection.service.ts b/src/app/store/cluster-role-collection.service.ts deleted file mode 100644 index 98fcf04e..00000000 --- a/src/app/store/cluster-role-collection.service.ts +++ /dev/null @@ -1,14 +0,0 @@ -import { Injectable } from '@angular/core'; -import { KubernetesCollectionService } from './kubernetes-collection.service'; -import { ClusterRole } from '../types/clusterRole'; -import { EntityCollectionServiceElementsFactory } from '@ngrx/data'; -import { clusterroleEntityKey } from './entity-metadata-map'; - -@Injectable({ - providedIn: 'root', -}) -export class ClusterRoleCollectionService extends KubernetesCollectionService { - constructor(elementsFactory: EntityCollectionServiceElementsFactory) { - super(clusterroleEntityKey, elementsFactory); - } -} diff --git a/src/app/store/entity-metadata-map.ts b/src/app/store/entity-metadata-map.ts index 98720671..c2072b47 100644 --- a/src/app/store/entity-metadata-map.ts +++ b/src/app/store/entity-metadata-map.ts @@ -8,7 +8,6 @@ import { Team } from '../types/team'; import { User } from '../types/user'; import { Zone } from '../types/zone'; import { ClusterRoleBinding } from '../types/clusterrole-binding'; -import { ClusterRole } from '../types/clusterRole'; import { Invitation, InvitationRedeemRequest } from '../types/invitation'; export const organizationEntityKey = 'organization.appuio.io/v1/organizations'; @@ -16,7 +15,6 @@ export const organizationMembersEntityKey = 'appuio.io/v1/organizationmembers'; export const selfSubjectAccessReviewEntityKey = 'authorization.k8s.io/v1/selfsubjectaccessreviews'; export const billingEntityEntityKey = 'billing.appuio.io/v1/billingentities'; export const rolebindingEntityKey = 'rbac.authorization.k8s.io/v1/rolebindings'; -export const clusterroleEntityKey = 'rbac.authorization.k8s.io/v1/clusterroles'; export const clusterrolebindingEntityKey = 'rbac.authorization.k8s.io/v1/clusterrolebindings'; export const teamEntityKey = 'appuio.io/v1/teams'; export const userEntityKey = 'appuio.io/v1/users'; @@ -51,10 +49,6 @@ export const entityMetadataMap: EntityMetadataMap = { sortComparer: (a: BillingEntity, b: BillingEntity) => a.metadata.name.localeCompare(b.metadata.name, undefined, { sensitivity: 'base' }), }, - ClusterRole: { - entityName: clusterroleEntityKey, - selectId: (crb: ClusterRole) => crb.metadata.name, // cluster-scoped - }, RoleBinding: { entityName: rolebindingEntityKey, selectId: (rb: RoleBinding) => `${rb.metadata.namespace}/${rb.metadata.name}`, diff --git a/src/app/types/clusterRole.ts b/src/app/types/clusterRole.ts deleted file mode 100644 index 6c6a1c1c..00000000 --- a/src/app/types/clusterRole.ts +++ /dev/null @@ -1,10 +0,0 @@ -import { KubeObject } from './entity'; -import { Rule } from './rule'; - -export const ClusterRolePermissions = { group: 'rbac.authorization.k8s.io', resource: 'clusterroles' }; - -export interface ClusterRole extends KubeObject { - apiVersion: 'rbac.authorization.k8s.io/v1'; - kind: 'ClusterRole'; - rules: Rule[]; -}