Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Invalid CanActivate guard in cmsMapping #19265

Merged
merged 15 commits into from
Sep 27, 2024
Merged

Conversation

kpawelczak
Copy link
Contributor

@kpawelczak kpawelczak commented Sep 19, 2024

closes: CXSPA-7552

This change was made to break the reference to the canActivate array in the routing configuration. The previous implementation directly modified the route.canActivate array, leading to unintended side effects when the function was re-executed, particularly with lazy-loaded modules.

if (route?.canActivate?.length) {
  const canActivate = route.canActivate.map((guard: any) =>
    this.wrapCmsGuard(guard)
  );
  return {
    ...route,
    canActivate,
  };
}

Reason for the Change:
This change was implemented to avoid mutating the original route object, which could lead to issues in the following scenario:

  • Lazy-loaded module: The function is triggered each time a lazy-loaded module is loaded.

  • Logout and Re-login: After logging out and then logging in again, the routing function is triggered again.

  • Side effect: On the second execution (e.g., when navigating to "my-account/my-company"), the canActivate guards were already overwritten due to reference mutation in the previous run.

By breaking the reference and returning a new route object with a copied canActivate array, we make sure that each execution works with a fresh set of guards without mutating the original route object.

note:

  • spread operator was not enough to break the reference
  • JSON method breaks the object type and would require retyping the route object

@kpawelczak kpawelczak requested a review from a team as a code owner September 19, 2024 09:15
@github-actions github-actions bot marked this pull request as draft September 19, 2024 09:15
@kpawelczak kpawelczak marked this pull request as ready for review September 19, 2024 09:15
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@github-actions github-actions bot marked this pull request as draft September 19, 2024 09:24
@kpawelczak kpawelczak marked this pull request as ready for review September 19, 2024 09:27
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@github-actions github-actions bot marked this pull request as draft September 19, 2024 14:34
@kpawelczak kpawelczak marked this pull request as ready for review September 19, 2024 14:34
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@github-actions github-actions bot marked this pull request as draft September 19, 2024 14:37
@kpawelczak kpawelczak marked this pull request as ready for review September 19, 2024 14:38
Copy link
Contributor

Merge Checks Failed

Please push a commit to re-trigger the build. 
To push an empty commit you can use `git commit --allow-empty -m "Trigger Build"`

@github-actions github-actions bot marked this pull request as draft September 19, 2024 14:42
@kpawelczak kpawelczak marked this pull request as ready for review September 19, 2024 14:47
Copy link

cypress bot commented Sep 19, 2024

spartacus    Run #45036

Run Properties:  status check passed Passed #45036  •  git commit 438506d376 ℹ️: Merge 65dbcf10619cdc94f44c53a249add111e57bca4d into 267231983dddbeef941f6a31edbb...
Project spartacus
Branch Review bugfix/CXSPA-7552
Run status status check passed Passed #45036
Run duration 03m 36s
Commit git commit 438506d376 ℹ️: Merge 65dbcf10619cdc94f44c53a249add111e57bca4d into 267231983dddbeef941f6a31edbb...
Committer kpawelczak
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 125
View all changes introduced in this branch ↗︎

@github-actions github-actions bot marked this pull request as draft September 20, 2024 11:58
@kpawelczak kpawelczak marked this pull request as ready for review September 20, 2024 12:04
@github-actions github-actions bot marked this pull request as draft September 20, 2024 12:27
@kpawelczak kpawelczak marked this pull request as ready for review September 20, 2024 12:29
Platonn
Platonn previously approved these changes Sep 27, 2024
@github-actions github-actions bot marked this pull request as draft September 27, 2024 12:30
@kpawelczak kpawelczak marked this pull request as ready for review September 27, 2024 12:39
@github-actions github-actions bot marked this pull request as draft September 27, 2024 13:00
@kpawelczak kpawelczak marked this pull request as ready for review September 27, 2024 13:00
@github-actions github-actions bot marked this pull request as draft September 27, 2024 13:23
@kpawelczak kpawelczak marked this pull request as ready for review September 27, 2024 13:48
@kpawelczak kpawelczak merged commit 3cbda23 into develop Sep 27, 2024
28 checks passed
@kpawelczak kpawelczak deleted the bugfix/CXSPA-7552 branch September 27, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants