From 805b4f936bc0da41a00aac938bcd60605a1d25ac Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 11 Dec 2020 17:16:54 -0800 Subject: [PATCH] feat(core): Add schematic to fix invalid `Route` configs (#40067) `Route` configs with `redirectTo` as well as `canActivate` are not valid because the `canActivate` guards will never execute. Redirects are applied before activation. There is no error currently for these configs, but another commit will change this so that an error does appear in dev mode. This migration fixes the configs by removing the `canActivate` property. PR Close #40067 --- packages/core/schematics/BUILD.bazel | 1 + packages/core/schematics/migrations.json | 7 +- .../can-activate-with-redirect-to/BUILD.bazel | 18 ++++ .../can-activate-with-redirect-to/README.md | 23 +++++ .../can-activate-with-redirect-to/index.ts | 56 +++++++++++++ .../can-activate-with-redirect-to/util.ts | 62 ++++++++++++++ .../schematics/migrations/google3/BUILD.bazel | 1 + .../google3/canActivateWithRedirectToRule.ts | 33 ++++++++ packages/core/schematics/test/BUILD.bazel | 1 + ...n_activate_with_redirect_migration_spec.ts | 78 +++++++++++++++++ .../can_activate_with_redirect_rule_spec.ts | 83 +++++++++++++++++++ 11 files changed, 362 insertions(+), 1 deletion(-) create mode 100644 packages/core/schematics/migrations/can-activate-with-redirect-to/BUILD.bazel create mode 100644 packages/core/schematics/migrations/can-activate-with-redirect-to/README.md create mode 100644 packages/core/schematics/migrations/can-activate-with-redirect-to/index.ts create mode 100644 packages/core/schematics/migrations/can-activate-with-redirect-to/util.ts create mode 100644 packages/core/schematics/migrations/google3/canActivateWithRedirectToRule.ts create mode 100644 packages/core/schematics/test/can_activate_with_redirect_migration_spec.ts create mode 100644 packages/core/schematics/test/google3/can_activate_with_redirect_rule_spec.ts diff --git a/packages/core/schematics/BUILD.bazel b/packages/core/schematics/BUILD.bazel index b68ceb4eb9..4934255a82 100644 --- a/packages/core/schematics/BUILD.bazel +++ b/packages/core/schematics/BUILD.bazel @@ -11,6 +11,7 @@ pkg_npm( visibility = ["//packages/core:__pkg__"], deps = [ "//packages/core/schematics/migrations/abstract-control-parent", + "//packages/core/schematics/migrations/can-activate-with-redirect-to", "//packages/core/schematics/migrations/dynamic-queries", "//packages/core/schematics/migrations/initial-navigation", "//packages/core/schematics/migrations/missing-injectable", diff --git a/packages/core/schematics/migrations.json b/packages/core/schematics/migrations.json index fb469fa76a..9d6d557e64 100644 --- a/packages/core/schematics/migrations.json +++ b/packages/core/schematics/migrations.json @@ -79,6 +79,11 @@ "version": "11.0.0-beta", "description": "Updates the `initialNavigation` property for `RouterModule.forRoot`.", "factory": "./migrations/initial-navigation/index" + }, + "migration-v11.1-can-activate-with-redirect-to": { + "version": "11.1.0-beta", + "description": "Removes `canActivate` from a `Route` config when `redirectTo` is also present", + "factory": "./migrations/can-activate-with-redirect-to/index" } } -} +} \ No newline at end of file diff --git a/packages/core/schematics/migrations/can-activate-with-redirect-to/BUILD.bazel b/packages/core/schematics/migrations/can-activate-with-redirect-to/BUILD.bazel new file mode 100644 index 0000000000..3899066089 --- /dev/null +++ b/packages/core/schematics/migrations/can-activate-with-redirect-to/BUILD.bazel @@ -0,0 +1,18 @@ +load("//tools:defaults.bzl", "ts_library") + +ts_library( + name = "can-activate-with-redirect-to", + srcs = glob(["**/*.ts"]), + tsconfig = "//packages/core/schematics:tsconfig.json", + visibility = [ + "//packages/core/schematics:__pkg__", + "//packages/core/schematics/migrations/google3:__pkg__", + "//packages/core/schematics/test:__pkg__", + ], + deps = [ + "//packages/core/schematics/utils", + "@npm//@angular-devkit/schematics", + "@npm//@types/node", + "@npm//typescript", + ], +) diff --git a/packages/core/schematics/migrations/can-activate-with-redirect-to/README.md b/packages/core/schematics/migrations/can-activate-with-redirect-to/README.md new file mode 100644 index 0000000000..e640403ec8 --- /dev/null +++ b/packages/core/schematics/migrations/can-activate-with-redirect-to/README.md @@ -0,0 +1,23 @@ +## Router migration to remove canActivate property from Routes that also have redirectTo + +The activation stage of the router happens after redirects so any `canActivate` guards +will not be executed. This invalid configuration is now an error. This migration +removes `canActivate` from the `Route` to fix pre-existing invalid configurations. + +#### Before +```ts +import { Routes } from '@angular/router'; + +const routes: Routes = [ + {path: '', redirectTo: 'other', canActivate: [MyGuard]} +]; +``` + +#### After +```ts +import { Routes } from '@angular/router'; + +const routes: Routes = [ + {path: '', redirectTo: 'other'} +]; +``` diff --git a/packages/core/schematics/migrations/can-activate-with-redirect-to/index.ts b/packages/core/schematics/migrations/can-activate-with-redirect-to/index.ts new file mode 100644 index 0000000000..34578470f5 --- /dev/null +++ b/packages/core/schematics/migrations/can-activate-with-redirect-to/index.ts @@ -0,0 +1,56 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Rule, SchematicsException, Tree} from '@angular-devkit/schematics'; +import {relative} from 'path'; +import * as ts from 'typescript'; + +import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; +import {canMigrateFile, createMigrationProgram} from '../../utils/typescript/compiler_host'; +import {findLiteralsToMigrate, migrateLiteral} from './util'; + + +/** Migration that removes `canActivate` property from routes that also have `redirectTo`. */ +export default function(): Rule { + return (tree: Tree) => { + const {buildPaths, testPaths} = getProjectTsConfigPaths(tree); + const basePath = process.cwd(); + const allPaths = [...buildPaths, ...testPaths]; + + if (!allPaths.length) { + throw new SchematicsException( + 'Could not find any tsconfig file. Cannot migrate ' + + 'Router.navigateByUrl and Router.createUrlTree calls.'); + } + + for (const tsconfigPath of allPaths) { + runCanActivateWithRedirectToMigration(tree, tsconfigPath, basePath); + } + }; +} + +function runCanActivateWithRedirectToMigration(tree: Tree, tsconfigPath: string, basePath: string) { + const {program} = createMigrationProgram(tree, tsconfigPath, basePath); + const printer = ts.createPrinter(); + const sourceFiles = + program.getSourceFiles().filter(sourceFile => canMigrateFile(basePath, sourceFile, program)); + + sourceFiles.forEach(sourceFile => { + const literalsToMigrate = findLiteralsToMigrate(sourceFile); + const update = tree.beginUpdate(relative(basePath, sourceFile.fileName)); + + for (const literal of Array.from(literalsToMigrate)) { + const migratedNode = migrateLiteral(literal); + update.remove(literal.getStart(), literal.getWidth()); + update.insertRight( + literal.getStart(), printer.printNode(ts.EmitHint.Unspecified, migratedNode, sourceFile)); + } + + tree.commitUpdate(update); + }); +} diff --git a/packages/core/schematics/migrations/can-activate-with-redirect-to/util.ts b/packages/core/schematics/migrations/can-activate-with-redirect-to/util.ts new file mode 100644 index 0000000000..63a6684317 --- /dev/null +++ b/packages/core/schematics/migrations/can-activate-with-redirect-to/util.ts @@ -0,0 +1,62 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import * as ts from 'typescript'; + +const CAN_ACTIVATE = 'canActivate'; +const REDIRECT_TO = 'redirectTo'; + +export function migrateLiteral(node: ts.ObjectLiteralExpression): ts.ObjectLiteralExpression { + const propertiesToKeep: ts.ObjectLiteralElementLike[] = []; + node.properties.forEach(property => { + // Only look for regular and shorthand property assignments since resolving things + // like spread operators becomes too complicated for this migration. + if ((ts.isPropertyAssignment(property) || ts.isShorthandPropertyAssignment(property)) && + (ts.isStringLiteralLike(property.name) || ts.isNumericLiteral(property.name) || + ts.isIdentifier(property.name))) { + if (property.name.text !== CAN_ACTIVATE) { + propertiesToKeep.push(property); + } + } else { + propertiesToKeep.push(property); + } + }); + + return ts.createObjectLiteral(propertiesToKeep); +} + + +export function findLiteralsToMigrate(sourceFile: ts.SourceFile) { + const results = new Set(); + + sourceFile.forEachChild(function visitNode(node: ts.Node) { + if (!ts.isObjectLiteralExpression(node)) { + node.forEachChild(visitNode); + return; + } + if (hasProperty(node, REDIRECT_TO) && hasProperty(node, CAN_ACTIVATE)) { + results.add(node); + } + }); + + return results; +} + +function hasProperty(node: ts.ObjectLiteralExpression, propertyName: string): boolean { + for (const property of node.properties) { + // Only look for regular and shorthand property assignments since resolving things + // like spread operators becomes too complicated for this migration. + if ((ts.isPropertyAssignment(property) || ts.isShorthandPropertyAssignment(property)) && + (ts.isStringLiteralLike(property.name) || ts.isNumericLiteral(property.name) || + ts.isIdentifier(property.name)) && + property.name.text === propertyName) { + return true; + } + } + return false; +} \ No newline at end of file diff --git a/packages/core/schematics/migrations/google3/BUILD.bazel b/packages/core/schematics/migrations/google3/BUILD.bazel index 0eecceb175..1fcd8e91f9 100644 --- a/packages/core/schematics/migrations/google3/BUILD.bazel +++ b/packages/core/schematics/migrations/google3/BUILD.bazel @@ -6,6 +6,7 @@ ts_library( tsconfig = "//packages/core/schematics:tsconfig.json", visibility = ["//packages/core/schematics/test/google3:__pkg__"], deps = [ + "//packages/core/schematics/migrations/can-activate-with-redirect-to", "//packages/core/schematics/migrations/dynamic-queries", "//packages/core/schematics/migrations/initial-navigation", "//packages/core/schematics/migrations/initial-navigation/google3", diff --git a/packages/core/schematics/migrations/google3/canActivateWithRedirectToRule.ts b/packages/core/schematics/migrations/google3/canActivateWithRedirectToRule.ts new file mode 100644 index 0000000000..d920e136b4 --- /dev/null +++ b/packages/core/schematics/migrations/google3/canActivateWithRedirectToRule.ts @@ -0,0 +1,33 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {Replacement, RuleFailure, Rules} from 'tslint'; +import * as ts from 'typescript'; +import {findLiteralsToMigrate, migrateLiteral} from '../can-activate-with-redirect-to/util'; + + +/** TSLint rule that removes canActivate from Route configs that also have redirectTo. */ +export class Rule extends Rules.TypedRule { + applyWithProgram(sourceFile: ts.SourceFile, program: ts.Program): RuleFailure[] { + const failures: RuleFailure[] = []; + const printer = ts.createPrinter(); + const literalsToMigrate = findLiteralsToMigrate(sourceFile); + + for (const literal of Array.from(literalsToMigrate)) { + const migratedNode = migrateLiteral(literal); + failures.push(new RuleFailure( + sourceFile, literal.getStart(), literal.getEnd(), + 'canActivate cannot be used with redirectTo.', this.ruleName, + new Replacement( + literal.getStart(), literal.getWidth(), + printer.printNode(ts.EmitHint.Unspecified, migratedNode, sourceFile)))); + } + + return failures; + } +} diff --git a/packages/core/schematics/test/BUILD.bazel b/packages/core/schematics/test/BUILD.bazel index a26b22892f..c6d62041e8 100644 --- a/packages/core/schematics/test/BUILD.bazel +++ b/packages/core/schematics/test/BUILD.bazel @@ -9,6 +9,7 @@ ts_library( ], deps = [ "//packages/core/schematics/migrations/abstract-control-parent", + "//packages/core/schematics/migrations/can-activate-with-redirect-to", "//packages/core/schematics/migrations/dynamic-queries", "//packages/core/schematics/migrations/initial-navigation", "//packages/core/schematics/migrations/missing-injectable", diff --git a/packages/core/schematics/test/can_activate_with_redirect_migration_spec.ts b/packages/core/schematics/test/can_activate_with_redirect_migration_spec.ts new file mode 100644 index 0000000000..24c750b936 --- /dev/null +++ b/packages/core/schematics/test/can_activate_with_redirect_migration_spec.ts @@ -0,0 +1,78 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {getSystemPath, normalize, virtualFs} from '@angular-devkit/core'; +import {TempScopedNodeJsSyncHost} from '@angular-devkit/core/node/testing'; +import {HostTree} from '@angular-devkit/schematics'; +import {SchematicTestRunner, UnitTestTree} from '@angular-devkit/schematics/testing'; +import * as shx from 'shelljs'; + +describe('canActivate removal with redirectTo', () => { + let runner: SchematicTestRunner; + let host: TempScopedNodeJsSyncHost; + let tree: UnitTestTree; + let tmpDirPath: string; + let previousWorkingDir: string; + + beforeEach(() => { + runner = new SchematicTestRunner('test', require.resolve('../migrations.json')); + host = new TempScopedNodeJsSyncHost(); + tree = new UnitTestTree(new HostTree(host)); + + writeFile('/tsconfig.json', JSON.stringify({ + compilerOptions: { + lib: ['es2015'], + strictNullChecks: true, + }, + })); + writeFile('/angular.json', JSON.stringify({ + projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}} + })); + + previousWorkingDir = shx.pwd(); + tmpDirPath = getSystemPath(host.root); + + // Switch into the temporary directory path. This allows us to run + // the schematic against our custom unit test tree. + shx.cd(tmpDirPath); + }); + + afterEach(() => { + shx.cd(previousWorkingDir); + shx.rm('-r', tmpDirPath); + }); + + it('should not remove canActivate when redirectTo is not present', async () => { + writeFile('/index.ts', `const route = {path: '', canActivate: ['my_guard_token']}`); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toEqual(`const route = {path: '', canActivate: ['my_guard_token']}`); + }); + + it('removes canActivate when redirectTo is present', async () => { + writeFile( + '/index.ts', + `const route = {path: '', redirectTo: 'other', canActivate: ['my_guard_token']}`); + + await runMigration(); + + const content = tree.readContent('/index.ts'); + expect(content).toEqual(`const route = { path: '', redirectTo: 'other' }`); + }); + + function writeFile(filePath: string, contents: string) { + host.sync.write(normalize(filePath), virtualFs.stringToFileBuffer(contents)); + } + + function runMigration() { + return runner.runSchematicAsync('migration-v11.1-can-activate-with-redirect-to', {}, tree) + .toPromise(); + } +}); diff --git a/packages/core/schematics/test/google3/can_activate_with_redirect_rule_spec.ts b/packages/core/schematics/test/google3/can_activate_with_redirect_rule_spec.ts new file mode 100644 index 0000000000..0f3a479c5c --- /dev/null +++ b/packages/core/schematics/test/google3/can_activate_with_redirect_rule_spec.ts @@ -0,0 +1,83 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import {readFileSync, writeFileSync} from 'fs'; +import {dirname, join} from 'path'; +import * as shx from 'shelljs'; +import {Configuration, Linter} from 'tslint'; + +describe('Google3 canActivate with redirectTo', () => { + const rulesDirectory = + dirname(require.resolve('../../migrations/google3/canActivateWithRedirectToRule')); + + let tmpDir: string; + + beforeEach(() => { + tmpDir = join(process.env['TEST_TMPDIR']!, 'google3-test'); + shx.mkdir('-p', tmpDir); + + writeFile('tsconfig.json', JSON.stringify({ + compilerOptions: { + module: 'es2015', + baseUrl: './', + }, + })); + }); + + afterEach(() => shx.rm('-r', tmpDir)); + + function runTSLint(fix: boolean) { + const program = Linter.createProgram(join(tmpDir, 'tsconfig.json')); + const linter = new Linter({fix, rulesDirectory: [rulesDirectory]}, program); + const config = Configuration.parseConfigFile({rules: {'can-activate-with-redirect-to': true}}); + + program.getRootFileNames().forEach(fileName => { + linter.lint(fileName, program.getSourceFile(fileName)!.getFullText(), config); + }); + + return linter; + } + + function writeFile(fileName: string, content: string) { + writeFileSync(join(tmpDir, fileName), content); + } + + function getFile(fileName: string) { + return readFileSync(join(tmpDir, fileName), 'utf8'); + } + + it('should not flag canActivate when redirectTo is not present', async () => { + writeFile('/index.ts', `const route = {path: '', canActivate: ['my_guard_token']}`); + + const linter = runTSLint(false); + const failures = linter.getResult().failures.map(failure => failure.getFailure()); + + expect(failures.length).toBe(0); + }); + + it('should flag when canActivate when redirectTo is present', async () => { + writeFile( + '/index.ts', + `const route = {path: '', redirectTo: 'other', canActivate: ['my_guard_token']}`); + + const linter = runTSLint(false); + const failures = linter.getResult().failures.map(failure => failure.getFailure()); + expect(failures.length).toBe(1); + expect(failures[0]).toMatch(/canActivate cannot be used with redirectTo./); + }); + + it('should fix when canActivate when redirectTo is present', async () => { + writeFile( + '/index.ts', + `const route = {path: '', redirectTo: 'other', canActivate: ['my_guard_token']}`); + + runTSLint(true); + const content = getFile('/index.ts'); + expect(content).toContain(`const route = { path: '', redirectTo: 'other' }`); + }); +});