Merge pull request #410 from bloomberg/org-check

Check HEAD branch organization user

src/client/modals/editLinkedItem.js

@@ -24,6 +24,15 @@ module.controller('EditLinkedItemCtrl', function ($scope, $modalInstance, $windo
             windowClass: 'howto'
         });
     };
+
+    $scope.whitelistInfo = function () {
+        $modal.open({
+            templateUrl: '/modals/templates/whitelist_info.html',
+            controller: 'InfoCtrl',
+            windowClass: 'howto'
+        });
+    };
+
     $scope.ok = function () {
         $scope.selected.item.gist = $scope.selected.gist.url;
         linkItemService.updateLink($scope.selected.item).then(function success(data) {

src/client/modals/templates/editLinkedItem.html

@@ -69,7 +69,8 @@ <h4 class="link-topic">
     <div class="row">
         <div class="col-xs-12">
             <div class="form-group">
-                <div style="margin-bottom: 5px; margin-top: 5px;">Provide user names, who doesn't need to sign the CLA
+                <div style="margin-bottom: 5px; margin-top: 5px;">Specify usernames to be whitelisted
+                    <span class="clickable" ng-click="whitelistInfo()" style="font-size:12px; text-decoration:underline">(how does this work?)</span>
                     <br/>
                     <span class="side-note">(you can also use wildcard
                         <span class="wildcard">*</span>)</span>

src/client/modals/templates/whitelist_info.html

@@ -0,0 +1,41 @@
+<div class="modal-body modal-primary">
+  <div ng-click="cancel()" class="fa fa-times close-button"></div>
+
+  <div class="free-space">
+    <h4>How does whitelisting work?</h4>
+    <div>
+      If a GitHub username is included in the whitelist, they will not be
+      required to sign a CLA. This also applies to organization usernames.
+    </div>
+    <h4>Why whitelist usernames?</h4>
+    <div>
+      Since there's no way for bot users (such as Dependabot or Greenkeeper) to
+      sign a CLA, you may want to whitelist them. You can do so by adding their
+      names (in this case <i>dependabot[bot]</i> and
+      <i>greenkeeper[bot]</i> separated by a comma) to the whitelist. You can
+      also use wildcard symbol in case you want to whitelist all bot users
+      <i>*[bot]</i>.
+    </div>
+    <h4>Why whitelist organizations?</h4>
+    <div>
+      We see at least two use cases you may want to do so:
+      <ul>
+        <li>
+          You develop in a team and commit your changes via feature branches. As
+          soon you create a new pull request CLA assistant would ask you and
+          your team fellows to sign a CLA even if you are part of the
+          organization and doesn't need to do so. Now if you whitelist your own
+          organization name all pull requests coming from the same repository
+          will pass the CLA check.
+        </li>
+        <li>
+          You get contributions from another company which has already signed
+          your corporate CLA. You may want to let CLA assistant accept all PRs
+          coming from this company. Now you can whitelist the GitHub
+          organization name of this company and all pull requests coming from
+          this GitHub organization (i.e., from that organization's fork) will pass the check.
+        </li>
+      </ul>
+    </div>
+  </div>
+</div>

src/config.js

@@ -104,6 +104,7 @@ module.exports = {
 
         feature_flag: {
             required_signees: process.env.REQUIRED_SIGNEES || '',
+            organization_override_enabled: process.env.ORG_OVERRIDE_ENABLED || false,
         },
 
         static: [

src/server/services/cla.js

@@ -244,9 +244,8 @@ module.exports = function () {
         return deferred.promise;
     };
 
-    let getLastSignatureOnMultiDates = function (user, userId, repoId, orgId, sharedGist, gist_url, gist_version, date) {
+    let getLastSignatureOnMultiDates = function (user, userId, repoId, orgId, sharedGist = false, gist_url, gist_version, date) {
         let deferred = q.defer();
-        sharedGist = sharedGist === undefined ? false : sharedGist;
 
         if (!user || (!repoId && !orgId) || !gist_url || (date && !Array.isArray(date))) {
             let msg = `Not provide enough arguments for getSignature() ${user} ${userId} ${repoId} ${orgId} ${sharedGist} ${gist_url} ${gist_version} ${date}`;
@@ -383,9 +382,16 @@ module.exports = function () {
 
                     return args;
                 }).then(function (args) {
-                    return getLastSignatureOnMultiDates(args.user, args.userId, item.repoId, item.orgId, item.sharedGist, item.gist, args.gist_version, args.onDates).then(function (cla) {
-                        return cla;
-                    });
+                    return getLastSignatureOnMultiDates(
+                        args.user,
+                        args.userId,
+                        item.repoId,
+                        item.orgId,
+                        item.sharedGist,
+                        item.gist,
+                        args.gist_version,
+                        args.onDates
+                    );
                 });
             }).then(function (cla) {
                 return done(null, cla);
@@ -403,9 +409,7 @@ module.exports = function () {
          *   number (optional)
          */
         checkUserSignature: function (args, done) {
-            let self = this;
-
-            return self.getLastSignature(args, function (error, cla) {
+            return this.getLastSignature(args, function (error, cla) {
                 done(error, { signed: !!cla });
             });
         },
@@ -418,85 +422,118 @@ module.exports = function () {
          *   owner (mandatory)
          *   repo (optional)
          */
-        checkPullRequestSignatures: function (args, done) {
-            getLinkedItem(args.repo, args.owner, args.token).then(function (item) {
-                args.gist = item.gist;
-                args.repoId = item.repoId;
-                args.orgId = item.orgId;
-                args.onDates = [new Date()];
-                if (!args.gist) {
-                    return done(null, {
-                        signed: true
-                    });
-                }
+        checkPullRequestSignatures: async function (args) {
+            const submitterSignatureRequired = config.server.feature_flag.required_signees.indexOf('submitter') > -1;
+            const committerSignatureRequired = !config.server.feature_flag.required_signees || config.server.feature_flag.required_signees.indexOf('committer') > -1;
+            const organizationOverrideEnabled = config.server.feature_flag.organization_override_enabled;
+            const item = await getLinkedItem(args.repo, args.owner, args.token);
+            let signees = [];
+
+            if (!item) {
+                throw new Error('No linked item found');
+            }
 
-                return getGistObject(args.gist, item.token).then(function (gist) {
-                    args.gist_version = gist.data.history[0].version;
+            args.gist = item.gist;
+            args.repoId = item.repoId;
+            args.orgId = item.orgId;
+            args.onDates = [new Date()];
 
-                    return getPR(args.owner, args.repo, args.number, item.token).then(pullRequest => {
-                        args.onDates.push(new Date(pullRequest.created_at));
-                        const signees = [];
-                        if (config.server.feature_flag.required_signees.indexOf('submitter') > -1 && !(item.isUserWhitelisted !== undefined && item.isUserWhitelisted(pullRequest.user.login))) { //TODO: test it
-                            signees.push({
-                                name: pullRequest.user.login,
-                                id: pullRequest.user.id
-                            });
-                        }
+            if (!args.gist) { 
+                return ({ signed: true });
+            }
 
-                        return q(signees);
-                    }).then(signees => {
-                        const deferred = q.defer();
-                        if (!config.server.feature_flag.required_signees || config.server.feature_flag.required_signees.indexOf('committer') > -1) {
-                            repoService.getPRCommitters(args, function (error, committers) {
-                                if (error) {
-                                    logger.warn(new Error(error).stack);
-                                    if ((!signees || signees.length < 1) && (!committers || committers.length < 1)) {
-                                        done(error);
-                                    }
-                                }
-                                signees = _.uniqWith(signees.concat(committers), (object, other) => {
-                                    return object.id == other.id;
-                                }).filter((signee) => {
-                                    return signee && !(item.isUserWhitelisted !== undefined && item.isUserWhitelisted(signee.name)); //TODO: test it
-                                });
-                                deferred.resolve(signees);
-                            });
-                        } else {
-                            deferred.resolve(signees);
-                        }
+            const gist = await getGistObject(args.gist, item.token);
+            if (!gist) {
+                throw new Error('No gist found for item');
+            }
+            args.gist_version = gist.data.history[0].version;
+
+            const pullRequest = await getPR(args.owner, args.repo, args.number, item.token);
+            if (!pullRequest) {
+                throw new Error('No pull request found');
+            }
+            args.onDates.push(new Date(pullRequest.created_at));
 
-                        return deferred.promise;
-                    }).then(signees => {
-                        return checkAll(signees, item.repoId, item.orgId, item.sharedGist, item.gist, args.gist_version, args.onDates).then(function (result) {
-                            done(null, result);
+            const isOrgHead = pullRequest.head.repo.owner.type === 'Organization';
+            if (organizationOverrideEnabled && isOrgHead) {
+                const { owner: headOrg } = pullRequest.head.repo;
+                if (item.isUserWhitelisted !== undefined && item.isUserWhitelisted(headOrg.login)) {
+                    return ({ signed: true });
+                }
+                const { signed, user_map } = await checkAll(
+                    [{ name: headOrg.login, id: headOrg.id }],
+                    item.repoId,
+                    item.orgId,
+                    item.sharedGist,
+                    item.gist,
+                    args.gist_version,
+                    args.onDates
+                );
+                if (signed && user_map.includes(headOrg.login)) {
+                    return ({ signed, user_map });
+                }
+            }
+
+            if (submitterSignatureRequired) { //TODO: test it
+                signees.push({
+                    name: pullRequest.user.login,
+                    id: pullRequest.user.id
+                });
+            }
+
+            if (committerSignatureRequired) {
+                try {
+                    const committers = await new Promise((resolve) => {
+                        repoService.getPRCommitters(args, function (error, committers) {
+                            if (error) {
+                                logger.warn(new Error(error).stack);
+                                if ((!signees || signees.length < 1) && (!committers || committers.length < 1)) {
+                                    throw new Error(error);
+                                }
+                            }
+                            resolve(committers);
                         });
-                    }, function (er) {
-                        done(er);
                     });
-                }, function (er) {
-                    done(er);
-                });
-                // }).catch(function (er) {
-            }, function (er) {
-                done(er);
-            });
+
+                    signees = _.uniqWith([...signees, ...committers], (object, other) => object.id == other.id);
+                } catch (error) {
+                    throw new Error(error);
+                }
+            }
+
+            signees = signees.filter(signee =>
+                signee && !(item.isUserWhitelisted !== undefined && item.isUserWhitelisted(signee.name))
+            );
+
+            return checkAll(
+                signees,
+                item.repoId,
+                item.orgId,
+                item.sharedGist,
+                item.gist,
+                args.gist_version,
+                args.onDates
+            );
         },
 
-        check: function (args, done) {
-            let self = this;
+        check: async function (args, done) {
             if (args.user) {
-                return self.checkUserSignature(args, function (error, result) {
+                return this.checkUserSignature(args, function (error, result) {
                     done(error, result.signed);
                 });
             } else if (args.number) {
-                return self.checkPullRequestSignatures(args, function (error, result) {
+                try {
+                    const result = await this.checkPullRequestSignatures(args);
                     const signed = result ? result.signed : false;
                     const user_map = result ? result.user_map : undefined;
-                    done(error, signed, user_map);
-                });
-            }
 
-            return done(new Error('A user or a pull request number is required.'));
+                    done(null, signed, user_map);
+                } catch (error) {
+                    done(new Error(error));
+                }
+            } else {
+                done(new Error('A user or a pull request number is required.'));
+            }
         },
 
         sign: async function (args) {

src/tests/server/api/cla.js

@@ -502,17 +502,26 @@ describe('', function () {
 
         it('should update status of all open pull requests for the repo if user model has no requests stored', async function () {
             testUser.requests = undefined;
+            this.timeout(100);
+            try {
+                const res = await cla_api.sign(req);
 
-            const res = await cla_api.sign(req);
-
-            assert.ok(res);
-            sinon.assert.calledWithMatch(cla.sign, expArgs.claSign);
-            assert(github.call.calledWithMatch({
-                obj: 'pullRequests',
-                fun: 'getAll'
-            }));
-            assert(prService.editComment.called);
-            assert.equal(statusService.update.callCount, 2);
+                return new Promise((resolve) => {
+                    setTimeout(function () {
+                        assert.ok(res);
+                        sinon.assert.calledWithMatch(cla.sign, expArgs.claSign);
+                        assert(github.call.calledWithMatch({
+                            obj: 'pullRequests',
+                            fun: 'getAll'
+                        }));
+                        assert(prService.editComment.called);
+                        assert.equal(statusService.update.callCount, 2);
+                        resolve();
+                    }, 50);
+                });
+            } catch (e) {
+                assert.ifError(e);
+            }
         });
 
         it('should update status of all open pull requests for the repos and orgs that shared the same gist if user model has no requests stored', async function () {