Skip to content

Commit

Permalink
Replace whitelist with allowlist
Browse files Browse the repository at this point in the history
The `whitelist` input has been replaced with `allowlist`. The old input
remains supported as an alias. The new input name is more inclusive and
descriptive, and isn't burdened by racist connotations.

The documentation has been updated, and two tests have been added to
ensure that `whitelist` still works correctly as a fallback.
  • Loading branch information
Gudahtt committed Aug 26, 2020
1 parent 83d45d2 commit 2669f06
Show file tree
Hide file tree
Showing 12 changed files with 106 additions and 83 deletions.
19 changes: 10 additions & 9 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ jobs:
url-to-cladocument: 'https://link/to/your/legal/CLA/document/of/choice'
# This branch can't have protections, commits are made directly to the specified branch.
branch: 'master'
whitelist: githubuser_example,anotherGitHubuser,bot
allowlist: githubuser_example,anotherGitHubuser,bot
blockchain-storage-flag: false

```
Expand All @@ -53,7 +53,7 @@ CLA action workflow will be triggered on all Pull Request `opened, synchronize,

When the CLA workflow is triggered on pull request `closed` event, it will lock the Pull Request conversation after the Pull Request merge so that the contributors cannot modify or delete the signature comments later.

If your signature is not on file and your account isn't in the whitelist, the CLA Bot will provide instructions on what to do in the PR Conversation and block the PR from merging until all authors of commits sign the CLA.
If your signature is not on file and your account isn't in the allowlist, the CLA Bot will provide instructions on what to do in the PR Conversation and block the PR from merging until all authors of commits sign the CLA.

![Screenshot 2020-02-13 at 10 24 17](https://user-images.githubusercontent.com/33329946/74420003-0ca6e780-4e4b-11ea-85a7-4ccc3f53e3d5.png)

Expand All @@ -65,18 +65,18 @@ Add a comment with the requested signature to your pull request to sign the CLA.

## Additional Configuration Options

### Whitelist Accounts
### Allowlist Accounts

The `whitelist` parameter is a comma-seprated list of accounts which should not be considered for CLA signature verification. These accounts will **completely bypass the signature process**, and if all authors are whitelisted in a PR the CLA Signature Action won't even comment on the PR.
The `allowlist` parameter is a comma-seprated list of accounts which should not be considered for CLA signature verification. These accounts will **completely bypass the signature process**, and if all authors are allowlisted in a PR the CLA Signature Action won't even comment on the PR.

This feature is particularly useful for other bot accounts, such as dependabot or greenkeeper. For example, `dependabot-preview[bot],greenkeeper[bot]` will whitelist both of those bot accounts.
This feature is particularly useful for other bot accounts, such as dependabot or greenkeeper. For example, `dependabot-preview[bot],greenkeeper[bot]` will allowlist both of those bot accounts.

Wildcards are accepted and will be treated as a regex .* character, so you can whitelist ranges of accounts. Use caution with wildcards to avoid whitelisting actual human contributors.
Wildcards are accepted and will be treated as a regex .* character, so you can allowlist ranges of accounts. Use caution with wildcards to avoid allowlisting actual human contributors.

Some common accounts you may want to whitelist:
Some common accounts you may want to allowlist:

* `dependabot[bot]` - This is the account GitHub will use to open Dependabot fixes on your account.
* Your personal account - Since you'll be opening the PR to add it you'll need to either sign the CLA or just add yourself to the whitelist.
* Your personal account - Since you'll be opening the PR to add it you'll need to either sign the CLA or just add yourself to the allowlist.

### Using the Ethereum Blockchain

Expand All @@ -97,7 +97,8 @@ The CLA Signature Bot has the option to additionally store the signatures on the
| `url-to-cladocument` | _Required_ | The full URL of your CLA document. The CLA bot will link to this document in a Pull Request comment, so make sure it's public. Could be a gist link, or a link to a file in the same repo. |
| `path-to-signatures` | _optional_ | Path to the signature file in the repository. Default is `./signatures/cla.json`. |
| `branch` | _optional_ | Repository branch to store the signature file. Default is `master` |
| `whitelist` | _optional_ | Comma-separated list of accounts to [ignore](https://github.com/roblox/cla-assistant#Whitelist-Accounts). Example: `user1,user2,bot*` |
| `allowlist` | _optional_ | Comma-separated list of accounts to [ignore](https://github.com/roblox/cla-assistant#Allowlist-Accounts). Example: `user1,user2,bot*` |
| `whitelist` | _optional_ | (Deprecated) Alias of 'allowlist' |
| `blockchain-storage-flag` | _optional_ | Whether to store the Contributor's signature data in the Ethereum blockchain. May be `true` or `false`. Default is `false`. |
| `blockchain-webhook-endpoint` | _optional_ | The URL to post the blockchain request to. Can be used when running your own [blockchain-services](https://github.com/cla-assistant/blockchain-services) docker container. |
| `use-remote-repo` | _optional_ | Whether to use an alternate repository for storing the signature file than the one running the workflow. If `true` the remote repo name and PAT must be provided. Default is `false`. |
Expand Down
45 changes: 45 additions & 0 deletions __tests__/claAllowlist.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { Allowlist } from "../src/claAllowlist"
import { Author } from "../src/authorMap";

const allowlistAuthor = new Author({
name: "SomeUsername",
signed: false,
});

const notAllowlistAuthor = new Author({
name: "JoeRandom",
signed: false
});

const allowlistBot = new Author({
name: "CompanyBotForGithub",
signed: false,
});

const weirdName = new Author({
name: "A?Weird!User.Name@somecompany.email",
signed: false,
});

const emptyList = new Allowlist("");
const whitelist = new Allowlist(`${allowlistAuthor.name},CompanyBot*,${weirdName.name},`);

it('Empty allowlist keeps author.', () => {
expect(emptyList.isUserAllowlisted(allowlistAuthor)).toStrictEqual(false);
expect(emptyList.isUserAllowlisted(notAllowlistAuthor)).toStrictEqual(false);
expect(emptyList.isUserAllowlisted(allowlistBot)).toStrictEqual(false);
expect(emptyList.isUserAllowlisted(weirdName)).toStrictEqual(false);
});

it('Author is allowlisted', () => {
expect(whitelist.isUserAllowlisted(allowlistAuthor)).toStrictEqual(true);
expect(whitelist.isUserAllowlisted(notAllowlistAuthor)).toStrictEqual(false);
});

it('Glob bot is allowlisted', () => {
expect(whitelist.isUserAllowlisted(allowlistBot)).toStrictEqual(true);
});

it('Username with special characters is still allowlisted', () => {
expect(whitelist.isUserAllowlisted(weirdName)).toStrictEqual(true);
});
10 changes: 5 additions & 5 deletions __tests__/claRunner.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ClaRunner} from "../src/claRunner"
import * as github from '@actions/github';
import { IInputSettings } from "../src/inputSettings"
import { Whitelist } from "../src/claWhitelist";
import { Allowlist } from "../src/claAllowlist";
import { PullAuthors } from "../src/pullAuthors";
import { Author } from "../src/authorMap";
import { ClaFileRepository } from "../src/claFileRepository";
Expand Down Expand Up @@ -115,7 +115,7 @@ it("Successfully constructs with full or empty settings", () => {
remoteRepositoryOwner: "owner",
signatureRegex: /.*/,
signatureText: "signature",
whitelist: ""
allowlist: ""
} as IInputSettings;

const runner = new ClaRunner({inputSettings: fullSettings});
Expand Down Expand Up @@ -156,13 +156,13 @@ it('Locks the PR when the PR is closed', async () => {

it('Returns early if there are no authors', async () => {
const settings = getSettings();
const whitelist = new Whitelist("SomeDude,SomeDudette,SomeEnby");
const allowlist = new Allowlist("SomeDude,SomeDudette,SomeEnby");

const [authors, getAuthorsSpy] = getPullAuthorsMock(settings);

const runner = new ClaRunner({
inputSettings: settings,
claWhitelist: whitelist,
claAllowlist: allowlist,
pullAuthors: authors
});
const result = await runner.execute();
Expand Down Expand Up @@ -193,7 +193,7 @@ it ('Fails if not everyone has signed', async () => {

it('succeeds if a new signature makes everyone signed', async () => {
const settings = getSettings();
settings.whitelist = "SomeDude,SomeDudette";
settings.allowlist = "SomeDude,SomeDudette";
settings.pullRequestNumber = 86;

const [authors] = getPullAuthorsMock(settings);
Expand Down
45 changes: 0 additions & 45 deletions __tests__/claWhitelist.test.ts

This file was deleted.

16 changes: 15 additions & 1 deletion __tests__/inputHelper.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,27 @@ it('sets defaults', () => {
expect(settings.localRepositoryOwner).toBe("some-owner");
expect(settings.repositoryAccessToken).toBe(settings.localAccessToken);
expect(settings.claFilePath).toBeTruthy();
expect(settings.whitelist).toBeFalsy();
expect(settings.allowlist).toBeFalsy();
expect(settings.emptyCommitFlag).toBe(false);

expect(settings.octokitRemote).toBeTruthy();
expect(settings.octokitLocal).toBeTruthy();
})

it('Accepts allowlist', () => {

This comment has been minimized.

Copy link
@rekmarks

rekmarks Aug 26, 2020

Member

Were there no existing whitelist test cases?

This comment has been minimized.

Copy link
@Gudahtt

Gudahtt Aug 26, 2020

Author Member

There were tests of the whitelist settings property (which was renamed to allowlist), but none that tested it was set properly given the input. The only test cases here are for the defaults, and for any inputs that were known to have odd properties/restrictions

inputs["url-to-cladocument"] = "some/path/to/a/doc.md";
inputs["allowlist"] = "some-allowed-user,another-allowed-user";
const settings: IInputSettings = inputHelper.getInputs();
expect(settings.allowlist).toBe("some-allowed-user,another-allowed-user");
})

it("Accepts allowlist as alias 'whitelist'", () => {
inputs["url-to-cladocument"] = "some/path/to/a/doc.md";
inputs["whitelist"] = "some-allowed-user,another-allowed-user";
const settings: IInputSettings = inputHelper.getInputs();
expect(settings.allowlist).toBe("some-allowed-user,another-allowed-user");
})

it('Rejects invalid repository name arguments', () => {
inputs["url-to-cladocument"] = "some/path/to/a/doc.md";
inputs["use-remote-repo"] = "true";
Expand Down
5 changes: 4 additions & 1 deletion action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,12 @@ inputs:
blockchain-webhook-endpoint:
description: "(Optional) The URL to post the blockchain request to."
default: "https://u9afh6n36g.execute-api.eu-central-1.amazonaws.com/dev/webhook"
whitelist:
allowlist:
description: "Comma-separated list of users to exclude from CLA requirement. Can use * characters for wildcards."
default: ""
whitelist:
description: "(Deprecated) Alias of 'allowlist'."
default: ""
signature-text:
description: "The text to require as a signature."
signature-regex:
Expand Down
2 changes: 1 addition & 1 deletion lib/index.js

Large diffs are not rendered by default.

18 changes: 9 additions & 9 deletions src/claWhitelist.ts → src/claAllowlist.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,22 @@
import { Author } from "./authorMap";
import * as core from '@actions/core'

export class Whitelist {
readonly whitelist: string[];
export class Allowlist {
readonly allowlist: string[];

readonly reRegExpChar = /[\\^$.*+\-?()[\]{}|]/g;
readonly reHasRegExpChar = RegExp(this.reRegExpChar.source);

constructor(whitelist: string) {
this.whitelist = (whitelist || "").split(',').map(s => s.trim());
constructor(allowlist: string) {
this.allowlist = (allowlist || "").split(',').map(s => s.trim());
}

/**
* Determine if an author matches a whitelist rule.
* @param user The user to check against the whitelist.
* Determine if an author matches an allowlist rule.
* @param user The user to check against the allowlist.
*/
public isUserWhitelisted(user: Author): boolean {
return this.whitelist.some(pattern => {
public isUserAllowlisted(user: Author): boolean {
return this.allowlist.some(pattern => {
let result = false;
// Glob patterns need special handling, otherwise just match username.
if (pattern.includes('*')) {
Expand All @@ -27,7 +27,7 @@ export class Whitelist {
}

if (result) {
core.info(`Whitelisted author ${user.name} excluded from CLA requirement.`);
core.info(`Allowlisted author ${user.name} excluded from CLA requirement.`);
}
return result;
});
Expand Down
2 changes: 1 addition & 1 deletion src/claFileRepository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class ClaFileRepository {
claFile: ClaFile,
fileSha?: string): Promise<any> {
if (this.settings.isRemoteRepoReadonly) {
throw new Error(`Cannot write to remote repo as no remote-repo-pat was provided. This usually means this check was run in a pull request from a fork. Initialize the CLA file manually, or open a PR from a branch in the protected repository, not a fork, and using an account that is not whitelisted.`);
throw new Error(`Cannot write to remote repo as no remote-repo-pat was provided. This usually means this check was run in a pull request from a fork. Initialize the CLA file manually, or open a PR from a branch in the protected repository, not a fork, and using an account that is not allowlisted.`);
}

return await this.settings.octokitRemote.repos.createOrUpdateFile({
Expand Down
18 changes: 9 additions & 9 deletions src/claRunner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as core from '@actions/core';
import { Author } from "./authorMap";
import { BlockchainPoster } from "./blockchainPoster";
import { ClaFileRepository } from "./claFileRepository";
import { Whitelist } from "./claWhitelist";
import { Allowlist } from "./claAllowlist";
import { IInputSettings } from "./inputSettings";
import { PullComments } from './pullComments';
import { PullAuthors } from './pullAuthors';
Expand All @@ -11,7 +11,7 @@ import { PullCheckRunner } from './pullCheckRunner';
export class ClaRunner {
readonly settings: IInputSettings;
readonly claFileRepository: ClaFileRepository;
readonly whitelist: Whitelist;
readonly allowlist: Allowlist;
readonly pullComments: PullComments;
readonly pullAuthors: PullAuthors;
readonly blockchainPoster: BlockchainPoster;
Expand All @@ -20,22 +20,22 @@ export class ClaRunner {
constructor({
inputSettings,
claRepo,
claWhitelist,
claAllowlist,
pullComments,
pullAuthors,
blockchainPoster,
pullCheckRunner }: {
inputSettings: IInputSettings;
claRepo?: ClaFileRepository;
claWhitelist?: Whitelist;
claAllowlist?: Allowlist;
pullComments?: PullComments;
pullAuthors?: PullAuthors;
blockchainPoster?: BlockchainPoster;
pullCheckRunner?: PullCheckRunner;
}) {
this.settings = inputSettings;
this.claFileRepository = (!claRepo) ? new ClaFileRepository(this.settings) : claRepo;
this.whitelist = (!claWhitelist) ? new Whitelist(this.settings.whitelist) : claWhitelist;
this.allowlist = (!claAllowlist) ? new Allowlist(this.settings.allowlist) : claAllowlist;
this.pullComments = (!pullComments) ? new PullComments(this.settings) : pullComments
this.pullAuthors = (!pullAuthors) ? new PullAuthors(this.settings) : pullAuthors
this.blockchainPoster = (!blockchainPoster) ? new BlockchainPoster(this.settings) : blockchainPoster
Expand All @@ -49,16 +49,16 @@ export class ClaRunner {
return true;
}

// Just drop whitelisted authors entirely, no sense in processing them.
// Just drop allowlisted authors entirely, no sense in processing them.
let rawAuthors: Author[] = await this.pullAuthors.getAuthors();
rawAuthors = rawAuthors.filter(a => !this.whitelist.isUserWhitelisted(a));
rawAuthors = rawAuthors.filter(a => !this.allowlist.isUserAllowlisted(a));

if (rawAuthors.length === 0) {
core.info("No committers left after whitelisting. Approving pull request.");
core.info("No committers left after allowlisting. Approving pull request.");
return true;
}

core.debug(`Found a total of ${rawAuthors.length} authors after whitelisting.`);
core.debug(`Found a total of ${rawAuthors.length} authors after allowlisting.`);
core.debug(`Authors: ${rawAuthors.map(n => n.name).join(', ')}`);

const claFile = await this.claFileRepository.getClaFile();
Expand Down
2 changes: 1 addition & 1 deletion src/inputHelper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function getInputs(): IInputSettings {

settings.claFilePath = core.getInput("path-to-signatures") || "signatures/cla.json";
settings.branch = core.getInput("branch") || "master";
settings.whitelist = core.getInput("whitelist") || "";
settings.allowlist = core.getInput("allowlist") || core.getInput("whitelist") || "";

settings.signatureText = core.getInput("signature-text") || "I have read the CLA Document and I hereby sign the CLA";
settings.signatureRegex = new RegExp(core.getInput("signature-regex") || /^.*I\s*HAVE\s*READ\s*THE\s*CLA\s*DOCUMENT\s*AND\s*I\s*HEREBY\s*SIGN\s*THE\s*CLA/);
Expand Down
7 changes: 6 additions & 1 deletion src/inputSettings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,12 @@ export interface IInputSettings {
branch: string

/**
* The whitelist of accounts which should not be prompted to sign the CLA.
* The allowlist of accounts which should not be prompted to sign the CLA.
*/
allowlist: string

/**
* Alias of 'allowlist'
*/
whitelist: string

Expand Down

0 comments on commit 2669f06

Please sign in to comment.