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' }`); + }); +});