fix: URI components are not encoded correctly (#1640, #1641)

* encode all URI components with encodeResourceID function * only constants doesn't need to be encoded * in case ICM is fixed (ICM 12) the project can change it to single encoding at one place * plus sign must not be encoded plus comment why plus sign must not be encoded * migration note for encodeResourceID and ICM 11 vs ICM 12 handling
intershop · May 3, 2024 · 3e7c0ae · 3e7c0ae
1 parent c8cce73
commit 3e7c0ae
Show file tree

Hide file tree

Showing 20 changed files with 269 additions and 142 deletions.
diff --git a/docs/guides/migrations.md b/docs/guides/migrations.md
@@ -21,6 +21,12 @@ A few Stylelint rules were changed and the `.scss` files were adapted.
 In addition empty comments in `.scss` files are no longer allowed and removed.
 In migration projects either keep the Stylelint rules with the old settings or migrate all your styling files accordingly running `npm run format`.
 
+The method [`encodeResourceID`](../../src/app/core/utils/url-resource-ids.ts) is now used to encode all dynamic resource IDs in any REST API call.
+This was previously only done for the login but is now consistently used for all resource IDs.
+For ICM 7.10 and ICM 11 this is done by default with a duplicated `encodeURIComponent` encoding.
+For ICM 12 and newer this needs to be changed to a single `encodeURIComponent` with additional `+` character handling.
+The ICM 12 handling is already prepared in [`encodeResourceID`](../../src/app/core/utils/url-resource-ids.ts) and needs to be activated in the source code if the Intershop PWA is used together with ICM 12.
+
 ## From 5.0 to 5.1
 
 The OrderListComponent is strictly presentational, components using it have to supply the data.

diff --git a/projects/organization-management/src/app/services/cost-centers/cost-centers.service.spec.ts b/projects/organization-management/src/app/services/cost-centers/cost-centers.service.spec.ts
@@ -7,12 +7,15 @@ import { CostCenterBase, CostCenterBuyer } from 'ish-core/models/cost-center/cos
 import { Customer } from 'ish-core/models/customer/customer.model';
 import { ApiService } from 'ish-core/services/api/api.service';
 import { getLoggedInCustomer } from 'ish-core/store/customer/user';
+import { encodeResourceID } from 'ish-core/utils/url-resource-ids';
 
 import { CostCentersService } from './cost-centers.service';
 
 describe('Cost Centers Service', () => {
   let apiService: ApiService;
   let costCentersService: CostCentersService;
+  const millersEMail = 'pmiller@test.intershop.de';
+  const millersCostcenterEndpoint = `customers/4711/costcenters/123/buyers/${encodeResourceID(millersEMail)}`;
 
   beforeEach(() => {
     apiService = mock(ApiService);
@@ -107,23 +110,19 @@ describe('Cost Centers Service', () => {
   });
 
   it('should call the updateCostCenterBuyer of the customer API for updating a cost center buyer', done => {
-    const costCenterBuyer = { login: 'pmiller@test.intershop.de' } as CostCenterBuyer;
+    const costCenterBuyer = { login: millersEMail } as CostCenterBuyer;
 
     costCentersService.updateCostCenterBuyer('123', costCenterBuyer).subscribe(() => {
       verify(apiService.patch(anything(), anything())).once();
-      expect(capture(apiService.patch).last()[0]).toMatchInlineSnapshot(
-        `"customers/4711/costcenters/123/buyers/pmiller%2540test.intershop.de"`
-      );
+      expect(capture(apiService.patch).last()[0]).toMatchInlineSnapshot(`"${millersCostcenterEndpoint}"`);
       done();
     });
   });
 
   it('should call the deleteCostCenterBuyer of the customer API for deleting a cost center buyer', done => {
-    costCentersService.deleteCostCenterBuyer('123', 'pmiller@test.intershop.de').subscribe(() => {
+    costCentersService.deleteCostCenterBuyer('123', millersEMail).subscribe(() => {
       verify(apiService.delete(anything())).once();
-      expect(capture(apiService.delete).last()[0]).toMatchInlineSnapshot(
-        `"customers/4711/costcenters/123/buyers/pmiller%2540test.intershop.de"`
-      );
+      expect(capture(apiService.delete).last()[0]).toMatchInlineSnapshot(`"${millersCostcenterEndpoint}"`);
       done();
     });
   });

diff --git a/projects/organization-management/src/app/services/cost-centers/cost-centers.service.ts b/projects/organization-management/src/app/services/cost-centers/cost-centers.service.ts
@@ -25,7 +25,7 @@ export class CostCentersService {
   getCostCenters(): Observable<CostCenter[]> {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
-        this.apiService.get(`customers/${customer.customerNo}/costcenters`).pipe(
+        this.apiService.get(`customers/${encodeResourceID(customer.customerNo)}/costcenters`).pipe(
           this.apiService.resolveLinks<CostCenterData>(),
           map(ccData => ccData.map(CostCenterMapper.fromData))
         )
@@ -47,7 +47,9 @@ export class CostCentersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService
-          .get<CostCenterData>(`customers/${customer.customerNo}/costcenters/${costCenterId}`)
+          .get<CostCenterData>(
+            `customers/${encodeResourceID(customer.customerNo)}/costcenters/${encodeResourceID(costCenterId)}`
+          )
           .pipe(map(CostCenterMapper.fromData))
       )
     );
@@ -67,7 +69,7 @@ export class CostCentersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService
-          .post<CostCenterData>(`customers/${customer.customerNo}/costcenters`, costCenter)
+          .post<CostCenterData>(`customers/${encodeResourceID(customer.customerNo)}/costcenters`, costCenter)
           .pipe(concatMap(() => this.getCostCenter(costCenter.costCenterId)))
       )
     );
@@ -87,7 +89,10 @@ export class CostCentersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService
-          .patch<CostCenterData>(`customers/${customer.customerNo}/costcenters/${costCenter.id}`, costCenter)
+          .patch<CostCenterData>(
+            `customers/${encodeResourceID(customer.customerNo)}/costcenters/${encodeResourceID(costCenter.id)}`,
+            costCenter
+          )
           .pipe(map(CostCenterMapper.fromData))
       )
     );
@@ -104,7 +109,9 @@ export class CostCentersService {
     }
 
     return this.currentCustomer$.pipe(
-      switchMap(customer => this.apiService.delete(`customers/${customer.customerNo}/costcenters/${id}`))
+      switchMap(customer =>
+        this.apiService.delete(`customers/${encodeResourceID(customer.customerNo)}/costcenters/${encodeResourceID(id)}`)
+      )
     );
   }
 
@@ -127,7 +134,10 @@ export class CostCentersService {
       concatMap(customer =>
         forkJoin(
           buyers.map(buyer =>
-            this.apiService.post(`customers/${customer.customerNo}/costcenters/${costCenterId}/buyers`, buyer)
+            this.apiService.post(
+              `customers/${encodeResourceID(customer.customerNo)}/costcenters/${encodeResourceID(costCenterId)}/buyers`,
+              buyer
+            )
           )
         ).pipe(concatMap(() => this.getCostCenter(costCenterId)))
       )
@@ -153,7 +163,9 @@ export class CostCentersService {
       switchMap(customer =>
         this.apiService
           .patch(
-            `customers/${customer.customerNo}/costcenters/${costCenterId}/buyers/${encodeResourceID(buyer.login)}`,
+            `customers/${encodeResourceID(customer.customerNo)}/costcenters/${encodeResourceID(
+              costCenterId
+            )}/buyers/${encodeResourceID(buyer.login)}`,
             buyer
           )
           .pipe(concatMap(() => this.getCostCenter(costCenterId)))
@@ -179,7 +191,11 @@ export class CostCentersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService
-          .delete(`customers/${customer.customerNo}/costcenters/${costCenterId}/buyers/${encodeResourceID(login)}`)
+          .delete(
+            `customers/${encodeResourceID(customer.customerNo)}/costcenters/${encodeResourceID(
+              costCenterId
+            )}/buyers/${encodeResourceID(login)}`
+          )
           .pipe(concatMap(() => this.getCostCenter(costCenterId)))
       )
     );

diff --git a/projects/organization-management/src/app/services/users/users.service.spec.ts b/projects/organization-management/src/app/services/users/users.service.spec.ts
@@ -7,6 +7,7 @@ import { AppFacade } from 'ish-core/facades/app.facade';
 import { Customer } from 'ish-core/models/customer/customer.model';
 import { ApiService } from 'ish-core/services/api/api.service';
 import { getLoggedInCustomer } from 'ish-core/store/customer/user';
+import { encodeResourceID } from 'ish-core/utils/url-resource-ids';
 
 import { B2bRoleData } from '../../models/b2b-role/b2b-role.interface';
 import { B2bUser } from '../../models/b2b-user/b2b-user.model';
@@ -18,6 +19,8 @@ describe('Users Service', () => {
   let apiService: ApiService;
   let usersService: UsersService;
   let appFacade: AppFacade;
+  const millersEMail = 'pmiller@test.intershop.de';
+  const millersEndpoint = `customers/4711/users/${encodeResourceID(millersEMail)}`;
 
   beforeEach(() => {
     apiService = mock(ApiService);
@@ -59,31 +62,31 @@ describe('Users Service', () => {
   });
 
   it('should call the getUser of customer API when fetching user', done => {
-    usersService.getUser('pmiller@test.intershop.de').subscribe(() => {
+    usersService.getUser(millersEMail).subscribe(() => {
       verify(apiService.get(anything())).once();
       expect(capture(apiService.get).last()).toMatchInlineSnapshot(`
         [
-          "customers/4711/users/pmiller%2540test.intershop.de",
+          "${millersEndpoint}",
         ]
       `);
       done();
     });
   });
 
   it('should call delete method of customer API when delete user', done => {
-    usersService.deleteUser('pmiller@test.intershop.de').subscribe(() => {
+    usersService.deleteUser(millersEMail).subscribe(() => {
       verify(apiService.delete(anything())).once();
       expect(capture(apiService.delete).last()).toMatchInlineSnapshot(`
         [
-          "customers/4711/users/pmiller%2540test.intershop.de",
+          "${millersEndpoint}",
         ]
       `);
       done();
     });
   });
 
   it('should call the addUser for creating a new b2b user', done => {
-    const user = { login: 'pmiller@test.intershop.de' } as B2bUser;
+    const user = { login: millersEMail } as B2bUser;
 
     usersService.addUser(user).subscribe(() => {
       verify(apiService.post(anything(), anything())).once();
@@ -93,21 +96,19 @@ describe('Users Service', () => {
   });
 
   it('should call the updateUser for updating a b2b user', done => {
-    const user = { login: 'pmiller@test.intershop.de' } as B2bUser;
+    const user = { login: millersEMail } as B2bUser;
 
     usersService.updateUser(user).subscribe(() => {
       verify(apiService.put(anything(), anything())).once();
-      expect(capture(apiService.put).last()[0]).toMatchInlineSnapshot(
-        `"customers/4711/users/pmiller%2540test.intershop.de"`
-      );
+      expect(capture(apiService.put).last()[0]).toMatchInlineSnapshot(`"${millersEndpoint}"`);
       done();
     });
   });
 
   it('should put the roles onto user when calling setUserRoles', done => {
     when(apiService.put(anyString(), anything())).thenReturn(of({ userRoles: [{ roleID: 'BUYER' }] as B2bRoleData[] }));
 
-    usersService.setUserRoles('pmiller@test.intershop.de', []).subscribe(data => {
+    usersService.setUserRoles(millersEMail, []).subscribe(data => {
       expect(data).toMatchInlineSnapshot(`
         [
           "BUYER",
@@ -116,7 +117,7 @@ describe('Users Service', () => {
       verify(apiService.put(anything(), anything())).once();
       expect(capture(apiService.put).last()).toMatchInlineSnapshot(`
         [
-          "customers/4711/users/pmiller%2540test.intershop.de/roles",
+          "${millersEndpoint}/roles",
           {
             "userRoles": [],
           },
@@ -130,7 +131,7 @@ describe('Users Service', () => {
     when(apiService.put(anyString(), anything())).thenReturn(of({}));
 
     usersService
-      .setUserBudget('pmiller@test.intershop.de', {
+      .setUserBudget(millersEMail, {
         orderSpentLimit: undefined,
         budget: { value: 2000, currency: 'USD' },
         budgetPeriod: 'monthly',
@@ -140,7 +141,7 @@ describe('Users Service', () => {
         verify(apiService.put(anything(), anything())).once();
         expect(capture(apiService.put).last()).toMatchInlineSnapshot(`
           [
-            "customers/4711/users/pmiller%2540test.intershop.de/budgets",
+            "${millersEndpoint}/budgets",
             {
               "budget": {
                 "currency": "USD",

diff --git a/projects/organization-management/src/app/services/users/users.service.ts b/projects/organization-management/src/app/services/users/users.service.ts
@@ -37,7 +37,7 @@ export class UsersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService
-          .get(`customers/${customer.customerNo}/users`)
+          .get(`customers/${encodeResourceID(customer.customerNo)}/users`)
           .pipe(unpackEnvelope(), map(B2bUserMapper.fromListData))
       )
     );
@@ -53,7 +53,7 @@ export class UsersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService
-          .get(`customers/${customer.customerNo}/users/${encodeResourceID(login)}`)
+          .get(`customers/${encodeResourceID(customer.customerNo)}/users/${encodeResourceID(login)}`)
           .pipe(map(B2bUserMapper.fromData))
       )
     );
@@ -74,7 +74,7 @@ export class UsersService {
       withLatestFrom(this.appFacade.currentLocale$),
       switchMap(([customer, currentLocale]) =>
         this.apiService
-          .post<B2bUser>(`customers/${customer.customerNo}/users`, {
+          .post<B2bUser>(`customers/${encodeResourceID(customer.customerNo)}/users`, {
             elements: [
               {
                 ...customer,
@@ -120,7 +120,7 @@ export class UsersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService
-          .put(`customers/${customer.customerNo}/users/${encodeResourceID(user.login)}`, {
+          .put(`customers/${encodeResourceID(customer.customerNo)}/users/${encodeResourceID(user.login)}`, {
             ...customer,
             ...user,
             preferredInvoiceToAddress: { urn: user.preferredInvoiceToAddressUrn },
@@ -147,14 +147,16 @@ export class UsersService {
     }
 
     return this.currentCustomer$.pipe(
-      switchMap(customer => this.apiService.delete(`customers/${customer.customerNo}/users/${encodeResourceID(login)}`))
+      switchMap(customer =>
+        this.apiService.delete(`customers/${encodeResourceID(customer.customerNo)}/users/${encodeResourceID(login)}`)
+      )
     );
   }
 
   getAvailableRoles(): Observable<B2bRole[]> {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
-        this.apiService.get(`customers/${customer.customerNo}/roles`).pipe(
+        this.apiService.get(`customers/${encodeResourceID(customer.customerNo)}/roles`).pipe(
           unpackEnvelope<B2bRoleData>('userRoles'),
           map(data => this.b2bRoleMapper.fromData(data))
         )
@@ -171,7 +173,9 @@ export class UsersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService
-          .put(`customers/${customer.customerNo}/users/${encodeResourceID(login)}/roles`, { userRoles })
+          .put(`customers/${encodeResourceID(customer.customerNo)}/users/${encodeResourceID(login)}/roles`, {
+            userRoles,
+          })
           .pipe(
             unpackEnvelope<B2bRoleData>('userRoles'),
             map(data => data.map(r => r.roleID))
@@ -195,7 +199,7 @@ export class UsersService {
     return this.currentCustomer$.pipe(
       switchMap(customer =>
         this.apiService.put<UserBudget>(
-          `customers/${customer.customerNo}/users/${encodeResourceID(login)}/budgets`,
+          `customers/${encodeResourceID(customer.customerNo)}/users/${encodeResourceID(login)}/budgets`,
           budget
         )
       )

diff --git a/src/app/core/services/address/address.service.ts b/src/app/core/services/address/address.service.ts
@@ -7,6 +7,7 @@ import { AddressMapper } from 'ish-core/models/address/address.mapper';
 import { Address } from 'ish-core/models/address/address.model';
 import { Link } from 'ish-core/models/link/link.model';
 import { ApiService, unpackEnvelope } from 'ish-core/services/api/api.service';
+import { encodeResourceID } from 'ish-core/utils/url-resource-ids';
 
 /**
  * The Address Service handles the interaction with the REST API concerning addresses.
@@ -25,7 +26,7 @@ export class AddressService {
     return this.appFacade.customerRestResource$.pipe(
       first(),
       concatMap(restResource =>
-        this.apiService.get(`${restResource}/${customerId}/addresses`).pipe(
+        this.apiService.get(`${restResource}/${encodeResourceID(customerId)}/addresses`).pipe(
           unpackEnvelope<Link>(),
           this.apiService.resolveLinks<Address>(),
           map(addressesData => addressesData.map(AddressMapper.fromData))
@@ -51,7 +52,7 @@ export class AddressService {
       first(),
       concatMap(restResource =>
         this.apiService
-          .post(`${restResource}/${customerId}/addresses`, customerAddress)
+          .post(`${restResource}/${encodeResourceID(customerId)}/addresses`, customerAddress)
           .pipe(this.apiService.resolveLink<Address>(), map(AddressMapper.fromData))
       )
     );
@@ -73,7 +74,10 @@ export class AddressService {
       first(),
       concatMap(restResource =>
         this.apiService
-          .put(`${restResource}/${customerId}/addresses/${address.id}`, customerAddress)
+          .put(
+            `${restResource}/${encodeResourceID(customerId)}/addresses/${encodeResourceID(address.id)}`,
+            customerAddress
+          )
           .pipe(map(AddressMapper.fromData))
       )
     );
@@ -90,7 +94,9 @@ export class AddressService {
     return this.appFacade.customerRestResource$.pipe(
       first(),
       concatMap(restResource =>
-        this.apiService.delete(`${restResource}/${customerId}/addresses/${addressId}`).pipe(map(() => addressId))
+        this.apiService
+          .delete(`${restResource}/${encodeResourceID(customerId)}/addresses/${encodeResourceID(addressId)}`)
+          .pipe(map(() => addressId))
       )
     );
   }