ci(aio): do not deploy PR if preconditions not met

This avoids incorrectly failing the build if the PR author is not a member of one of the whitelisted GitHub teams.

aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-verifier.ts

@@ -1,6 +1,6 @@
 // Imports
 import * as jwt from 'jsonwebtoken';
-import {GithubPullRequests, PullRequest} from '../common/github-pull-requests';
+import {GithubPullRequests} from '../common/github-pull-requests';
 import {GithubTeams} from '../common/github-teams';
 import {assertNotMissingOrEmpty} from '../common/utils';
 import {UploadError} from './upload-error';
@@ -31,13 +31,20 @@ export class BuildVerifier {
   }
 
   // Methods - Public
-  public verify(pr: number, authHeader: string): Promise<void> {
+  public getPrAuthorTeamMembership(pr: number): Promise<{author: string, isMember: boolean}> {
+    return Promise.resolve().
+      then(() => this.githubPullRequests.fetch(pr)).
+      then(prInfo => prInfo.user.login).
+      then(author => this.githubTeams.isMemberBySlug(author, this.allowedTeamSlugs).
+        then(isMember => ({author, isMember})));
+  }
+
+  public verify(expectedPr: number, authHeader: string): Promise<void> {
     return Promise.resolve().
       then(() => this.extractJwtString(authHeader)).
-      then(jwtString => this.verifyJwt(pr, jwtString)).
+      then(jwtString => this.verifyJwt(expectedPr, jwtString)).
-      then(jwtPayload => this.fetchPr(jwtPayload['pull-request'])).
+      then(jwtPayload => this.verifyPr(jwtPayload['pull-request'])).
-      then(prInfo => this.verifyPr(prInfo.user.login)).
+      catch(err => { throw new UploadError(403, `Error while verifying upload for PR ${expectedPr}: ${err}`); });
-      catch(err => { throw new UploadError(403, `Error while verifying upload for PR ${pr}: ${err}`); });
   }
 
   // Methods - Protected
@@ -45,19 +52,15 @@ export class BuildVerifier {
     return input.replace(/^token +/i, '');
   }
 
-  protected fetchPr(pr: number): Promise<PullRequest> {
+  protected verifyJwt(expectedPr: number, token: string): Promise<JwtPayload> {
-    return this.githubPullRequests.fetch(pr);
-  }
-
-  protected verifyJwt(pr: number, token: string): Promise<JwtPayload> {
     return new Promise((resolve, reject) => {
       jwt.verify(token, this.secret, {issuer: 'Travis CI, GmbH'}, (err, payload) => {
         if (err) {
           reject(err.message || err);
         } else if (payload.slug !== this.repoSlug) {
           reject(`jwt slug invalid. expected: ${this.repoSlug}`);
-        } else if (payload['pull-request'] !== pr) {
+        } else if (payload['pull-request'] !== expectedPr) {
-          reject(`jwt pull-request invalid. expected: ${pr}`);
+          reject(`jwt pull-request invalid. expected: ${expectedPr}`);
         } else {
           resolve(payload);
         }
@@ -65,11 +68,11 @@ export class BuildVerifier {
     });
   }
 
-  protected verifyPr(username: string): Promise<void> {
+  protected verifyPr(pr: number): Promise<void> {
-    const errorMessage = `User '${username}' is not an active member of any of: ` +
+    return this.getPrAuthorTeamMembership(pr).
-                         `${this.allowedTeamSlugs.join(', ')}`;
+      then(({author, isMember}) => isMember ? Promise.resolve() : Promise.reject(
-
+        `User '${author}' is not an active member of any of the following teams: ` +
-    return this.githubTeams.isMemberBySlug(username, this.allowedTeamSlugs).
+        `${this.allowedTeamSlugs.join(', ')}`,
-      then(isMember => isMember ? Promise.resolve() : Promise.reject(errorMessage));
+      ));
   }
 }

aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/index-preverify-pr.ts

@@ -0,0 +1,39 @@
+// Imports
+import {getEnvVar} from '../common/utils';
+import {BuildVerifier} from './build-verifier';
+
+// Run
+_main();
+
+// Functions
+function _main() {
+  const secret = 'unused';
+  const githubToken = getEnvVar('AIO_GITHUB_TOKEN');
+  const repoSlug = getEnvVar('AIO_REPO_SLUG');
+  const organization = getEnvVar('AIO_GITHUB_ORGANIZATION');
+  const allowedTeamSlugs = getEnvVar('AIO_GITHUB_TEAM_SLUGS').split(',');
+  const pr = +getEnvVar('AIO_PREVERIFY_PR');
+
+  const buildVerifier = new BuildVerifier(secret, githubToken, repoSlug, organization, allowedTeamSlugs);
+
+  // Exit codes:
+  // - 0: The PR author is a member.
+  // - 1: The PR author is not a member.
+  // - 2: An error occurred.
+  buildVerifier.getPrAuthorTeamMembership(pr).
+    then(({author, isMember}) => {
+      if (isMember) {
+        process.exit(0);
+      } else {
+        const errorMessage = `User '${author}' is not an active member of any of the following teams: ` +
+                             `${allowedTeamSlugs.join(', ')}`;
+        onError(errorMessage, 1);
+      }
+    }).
+    catch(err => onError(err, 2));
+}
+
+function onError(err: string, exitCode: number) {
+  console.error(err);
+  process.exit(exitCode || 1);
+}

aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-verifier.spec.ts

@@ -53,19 +53,15 @@ describe('BuildVerifier', () => {
       'pull-request': pr,
       'slug': defaultConfig.repoSlug,
     };
-    let prsFetchSpy: jasmine.Spy;
+    let bvGetPrAuthorTeamMembership: jasmine.Spy;
-    let teamsIsMemberBySlugSpy: jasmine.Spy;
 
     // Heleprs
     const createAuthHeader = (partialJwt: Partial<typeof defaultJwt> = {}, secret: string = defaultConfig.secret) =>
       `Token ${jwt.sign({...defaultJwt, ...partialJwt}, secret)}`;
 
     beforeEach(() => {
-      prsFetchSpy = spyOn(GithubPullRequests.prototype, 'fetch').
+      bvGetPrAuthorTeamMembership = spyOn(bv, 'getPrAuthorTeamMembership').
-        and.returnValue(Promise.resolve({user: {login: 'username'}}));
+        and.returnValue(Promise.resolve({author: 'some-author', isMember: true}));
-
-      teamsIsMemberBySlugSpy = spyOn(GithubTeams.prototype, 'isMemberBySlug').
-        and.returnValue(Promise.resolve(true));
     });
 
 
@@ -148,16 +144,16 @@ describe('BuildVerifier', () => {
     });
 
 
-    it('should fetch the corresponding PR if the token is valid', done => {
+    it('should call \'getPrAuthorTeamMembership()\' if the token is valid', done => {
       bv.verify(pr, createAuthHeader()).then(() => {
-        expect(prsFetchSpy).toHaveBeenCalledWith(pr);
+        expect(bvGetPrAuthorTeamMembership).toHaveBeenCalledWith(pr);
         done();
       });
     });
 
 
-    it('should fail if fetching the PR errors', done => {
+    it('should fail if \'getPrAuthorTeamMembership()\' rejects', done => {
-      prsFetchSpy.and.callFake(() => Promise.reject('Test'));
+      bvGetPrAuthorTeamMembership.and.callFake(() => Promise.reject('Test'));
       bv.verify(pr, createAuthHeader()).catch(err => {
         expectToBeUploadError(err, 403, `Error while verifying upload for PR ${pr}: Test`);
         done();
@@ -165,29 +161,12 @@ describe('BuildVerifier', () => {
     });
 
 
-    it('should verify the PR author\'s membership in the specified teams', done => {
+    it('should fail if \'getPrAuthorTeamMembership()\' reports no membership', done => {
-      bv.verify(pr, createAuthHeader()).then(() => {
+      const errorMessage = `Error while verifying upload for PR ${pr}: User 'test' is not an active member of any of ` +
-        expect(teamsIsMemberBySlugSpy).toHaveBeenCalledWith('username', ['team1', 'team2']);
+                           'the following teams: team1, team2';
-        done();
-      });
-    });
 
-
+      bvGetPrAuthorTeamMembership.and.returnValue(Promise.resolve({author: 'test', isMember: false}));
-    it('should fail if verifying membership errors', done => {
-      teamsIsMemberBySlugSpy.and.callFake(() => Promise.reject('Test'));
       bv.verify(pr, createAuthHeader()).catch(err => {
-        expectToBeUploadError(err, 403, `Error while verifying upload for PR ${pr}: Test`);
-        done();
-      });
-    });
-
-
-    it('should fail if the PR author is not a member of the specified teams', done => {
-      teamsIsMemberBySlugSpy.and.callFake(() => Promise.resolve(false));
-      bv.verify(pr, createAuthHeader()).catch(err => {
-        const errorMessage = `Error while verifying upload for PR ${pr}: ` +
-                             `User 'username' is not an active member of any of: team1, team2`;
-
         expectToBeUploadError(err, 403, errorMessage);
         done();
       });
@@ -200,4 +179,75 @@ describe('BuildVerifier', () => {
 
   });
 
+
+  describe('getPrAuthorTeamMembership()', () => {
+    const pr = 9;
+    let prsFetchSpy: jasmine.Spy;
+    let teamsIsMemberBySlugSpy: jasmine.Spy;
+
+    beforeEach(() => {
+      prsFetchSpy = spyOn(GithubPullRequests.prototype, 'fetch').
+        and.returnValue(Promise.resolve({user: {login: 'username'}}));
+
+      teamsIsMemberBySlugSpy = spyOn(GithubTeams.prototype, 'isMemberBySlug').
+        and.returnValue(Promise.resolve(true));
+    });
+
+
+    it('should return a promise', () => {
+      expect(bv.getPrAuthorTeamMembership(pr)).toEqual(jasmine.any(Promise));
+    });
+
+
+    it('should fetch the corresponding PR', done => {
+      bv.getPrAuthorTeamMembership(pr).then(() => {
+        expect(prsFetchSpy).toHaveBeenCalledWith(pr);
+        done();
+      });
+    });
+
+
+    it('should fail if fetching the PR errors', done => {
+      prsFetchSpy.and.callFake(() => Promise.reject('Test'));
+      bv.getPrAuthorTeamMembership(pr).catch(err => {
+        expect(err).toBe('Test');
+        done();
+      });
+    });
+
+
+    it('should verify the PR author\'s membership in the specified teams', done => {
+      bv.getPrAuthorTeamMembership(pr).then(() => {
+        expect(teamsIsMemberBySlugSpy).toHaveBeenCalledWith('username', ['team1', 'team2']);
+        done();
+      });
+    });
+
+
+    it('should fail if verifying membership errors', done => {
+      teamsIsMemberBySlugSpy.and.callFake(() => Promise.reject('Test'));
+      bv.getPrAuthorTeamMembership(pr).catch(err => {
+        expect(err).toBe('Test');
+        done();
+      });
+    });
+
+
+    it('should return the PR\'s author and whether they are members', done => {
+      teamsIsMemberBySlugSpy.and.returnValues(Promise.resolve(true), Promise.resolve(false));
+
+      Promise.all([
+        bv.getPrAuthorTeamMembership(pr).then(({author, isMember}) => {
+          expect(author).toBe('username');
+          expect(isMember).toBe(true);
+        }),
+        bv.getPrAuthorTeamMembership(pr).then(({author, isMember}) => {
+          expect(author).toBe('username');
+          expect(isMember).toBe(false);
+        }),
+      ]).then(done);
+    });
+
+  });
+
 });

aio/aio-builds-setup/scripts/travis-preverify-pr.sh

@@ -0,0 +1,13 @@
+#!/bin/bash
+set -eux -o pipefail
+
+# Set up env
+source "`dirname $0`/env.sh"
+
+# Preverify PR
+AIO_GITHUB_ORGANIZATION="angular" \
+AIO_GITHUB_TEAM_SLUGS="angular-core" \
+AIO_GITHUB_TOKEN=$(echo ${GITHUB_TEAM_MEMBERSHIP_CHECK_KEY} | rev) \
+AIO_REPO_SLUG=$TRAVIS_REPO_SLUG \
+AIO_PREVERIFY_PR=$TRAVIS_PULL_REQUEST \
+node "$SCRIPTS_JS_DIR/dist/lib/upload-server/index-preverify-pr"

scripts/ci/deploy.sh

@@ -54,11 +54,26 @@ case ${CI_MODE} in
         # This is a PR: deploy a snapshot for previewing
         travisFoldStart "deploy.aio.pr-preview"
           # Only deploy if this PR has touched relevant files.
-          AIO_CHANGED_FILES_COUNT=$(git diff --name-only $TRAVIS_COMMIT_RANGE | grep ^aio/ | wc -l)
+          readonly AIO_CHANGED_FILES_COUNT=$(git diff --name-only $TRAVIS_COMMIT_RANGE | grep ^aio/ | wc -l)
           if [[ AIO_CHANGED_FILES_COUNT -eq 0 ]]; then
             echo "Skipping deploy because this PR did not touch any files inside 'aio/'."
           else
-            yarn run deploy-preview
+            # Only deploy if this PR meets certain preconditions.
+            readonly AIO_PREVERIFY_EXIT_CODE=$(./aio-builds-setup/scripts/travis-preverify-pr.sh && echo 0 || echo $?)
+            case $AIO_PREVERIFY_EXIT_CODE in
+              2)
+                # An error occurred: Fail the build
+                exit 1;
+                ;;
+              1)
+                # Preconditions not met: Skip deploy
+                echo "Skipping deploy because this PR did not meet the preconditions."
+                ;;
+              0)
+                # Preconditions met: Deploy
+                yarn run deploy-preview
+                ;;
+            esac
           fi
         travisFoldEnd "deploy.aio.pr-preview"
       else

scripts/ci/env.sh

@@ -77,6 +77,9 @@ if [[ ${TRAVIS:-} ]]; then
   # WARNING: NGBUILDS_IO_KEY should NOT be printed
   export NGBUILDS_IO_KEY=${NGBUILDS_IO_KEY:-$SAUCE_ACCESS_KEY}
 
+  # Personal token generated by mary-poppins, with only `read_org` permission
+  export GITHUB_TEAM_MEMBERSHIP_CHECK_KEY=35fc4093c1f29a2ddaf60cce5d57065454180bf6
+
   # Used by karma and karma-chrome-launcher
   # In order to have a meaningful SauceLabs badge on the repo page,
   # the angular2-ci account is used only when pushing commits to master;