Set up application labelling and commenting #924

jodyheavener · 2024-03-26T18:11:28Z

/dev/web/developer.1password.com/-/issues/1065

Summary

We are working on improvements to the 1Password for Open Source program. This PR continues from #918 and sets up application labelling and commenting from the bot account based on the application's status and validity.

What's changed?

The main change here is setting up what our "bot" account does in response to an issue. Now, when an application issue is opened or updated, we do the following:

Get the status of the issue - is it closed? Was it previously valid or invalid? Has it already been approved?
Use the status to write a comment on the issue. This is determined both by the current status, as well as the validity of the application. If the issue is closed or the application is already approved, we can't do anything. If the issue is not valid, report that. If the issue is valid, report that - get specific if it was previously valid. In these latter cases, also print the data we're going to store once the application is approved.

Additionally the following was changed:

Updates how the application renders its formatted problem strings
Adds a handful of test issues

Testing

There are no new unit/integration tests just yet (but there will be), but there are several updates to the manual testing flow to help debug and validate the application processor, like the last issue. Here's how to use it:

git checkout jh/application-label-comment
make install_deps && make build_processor
./processor review --test-issue <issue-name>

This command will print the application input it received and the parsed data.

Example output

[DEBUG] Using test issue 'valid-project'
[DEBUG] Skipping GitHub Init
[DEBUG] Parsed input data:
{
        "Account URL": "testdb.1password.com",
        "Additional comments": "Thank you!",
        "Age confirmation": "- [X] Yes, this project is at least 30 days old",
        "Can we contact you?": "- [X] Yes, you may contact me",
        "Email": "[email protected]",
        "Event application": "- [ ] Yes, this application is for an event",
        "Homepage URL": "https://github.com/wendyappleed/test-db",
        "License URL": "https://github.com/wendyappleed/test-db/blob/main/LICENSE.md",
        "License type": "MIT",
        "Name": "Wendy Appleseed",
        "Non-commercial confirmation": "- [X] No, this account won't be used for commercial activity",
        "Number of team members/core contributors": "1",
        "Profile or website": "https://github.com/wendyappleseed/",
        "Project name": "TestDB",
        "Project role": "Core Maintainer",
        "Repository URL": "https://github.com/wendyappleed/test-db",
        "Short description": "TestDB is a free and open source, community-based forum software project.",
        "Team application": "- [ ] Yes, this application is for a team"
}
[DEBUG] Application data:
{
        "account": "testdb.1password.com",
        "project": {
                "name": "TestDB",
                "description": "TestDB is a free and open source, community-based forum software project.",
                "contributors": 1,
                "home_url": "https://github.com/wendyappleed/test-db",
                "repo_url": "https://github.com/wendyappleed/test-db",
                "license_type": "MIT",
                "license_url": "https://github.com/wendyappleed/test-db/blob/main/LICENSE.md",
                "is_event": false,
                "is_team": false
        },
        "applicant": {
                "name": "Wendy Appleseed",
                "email": "[email protected]",
                "role": "Core Maintainer",
                "id": 38230737
        },
        "can_contact": true,
        "issue_number": 6,
        "created_at": "2023-07-12T19:49:35Z"
}

In addition to this information this will output the automated bot comment's message, as well as any labels being added or removed.

The following test issues are available:

`valid-project`

./processor review --test-issue valid-project

This is a valid application for a project. It will result in the status: reviewing label being added, and a helpful comment about what's next.

Bot comment

✅ Your application is valid

Application data...

{
        "account": "testdb.1password.com",
        "project": {
                "name": "TestDB",
                "description": "TestDB is a free and open source, community-based forum software project.",
                "contributors": 1,
                "home_url": "https://github.com/wendyappleed/test-db",
                "repo_url": "https://github.com/wendyappleed/test-db",
                "license_type": "MIT",
                "license_url": "https://github.com/wendyappleed/test-db/blob/main/LICENSE.md",
                "is_event": false,
                "is_team": false
        },
        "applicant": {
                "name": "Wendy Appleseed",
                "email": "[email protected]",
                "role": "Core Maintainer",
                "id": 38230737
        },
        "can_contact": true,
        "issue_number": 6,
        "created_at": "2023-07-12T19:49:35Z"
}

Thanks for applying! Next step: our team will review your application and may have follow-up questions. You can still make changes to your application and we'll process it again.

The valid-team and valid-event test issues will have a similar output.

`valid-project-reviewing`

./processor review --test-issue valid-project-reviewing

This is a valid application, where the application was previously valid. This will not result in any label changes.

Bot comment

👍 Application still valid

Application data...

{
        "account": "testdb.1password.com",
        "project": {
                "name": "TestDB",
                "description": "TestDB is a free and open source, community-based forum software project.",
                "contributors": 1,
                "home_url": "https://github.com/wendyappleed/test-db",
                "repo_url": "https://github.com/wendyappleed/test-db",
                "license_type": "MIT",
                "license_url": "https://github.com/wendyappleed/test-db/blob/main/LICENSE.md",
                "is_event": false,
                "is_team": false
        },
        "applicant": {
                "name": "Wendy Appleseed",
                "email": "[email protected]",
                "role": "Core Maintainer",
                "id": 38230737
        },
        "can_contact": true,
        "issue_number": 6,
        "created_at": "2023-07-12T19:49:35Z"
}

We've processed your updated application and everything still looks good.

`valid-project-closed`

./processor review --test-issue valid-project-closed

This is a valid application for a project, but the issue for the application is already closed and therefor cannot be modified in any way - processing aborted. This will not result in any label changes.

Bot comment

Oops! This application is closed can no longer be processed. If this is an error, please reach out to [email protected].

`valid-project-approved`

./processor review --test-issue valid-project-approved

This is a valid application for a project, but it was already approved and therefor cannot be modified in any way - processing aborted. This will not result in any label changes.

Bot comment

Oops! This application has been updated but has already been approved and can no longer be processed. If this is an error, please reach out to [email protected].

`invalid-examples-1`

./processor review --test-issue invalid-examples-1

This is an application that contains validation errors. It will result in the status: invalid label being added, and a comment that contains the errors that need to be addressed.

Bot comment

❌ Your application needs some work

Application data...

{
        "account": "foo",
        "project": {
                "name": "TestDB 🎁",
                "description": "TestDB is a free and open source, community-based forum software project.",
                "contributors": 0,
                "home_url": "@wendyappleed",
                "repo_url": "https://github.com/wendyappleed/test-db",
                "license_type": "MIT",
                "license_url": "https://github.com/wendyappleed/test-db/blob/main/LICENSE.md",
                "is_event": false,
                "is_team": false
        },
        "applicant": {
                "name": "Wendy Appleseed",
                "email": "[email protected]",
                "role": "Lead Dev",
                "id": 38230737
        },
        "can_contact": true,
        "issue_number": 6,
        "created_at": "2023-07-12T19:49:35Z"
}

The following issues need to be addressed:

Application title is missing project name
Account URL is invalid 1Password account URL
Non-commercial confirmation must be checked
Project name cannot contain emoji characters
Number of team members/core contributors could not be parsed into a number
Homepage URL is an invalid URL

The invalid-empty-body and invalid-no-responses test issues will have a similar output.

schneedotdev

LGTM 👍

I ran through your changes and have validated everything on my end.

michaelAbon1p

Overall, I love the structure of this. I have one idiomatic suggestion. I don't think that the response messages fit with our tone. I've tried to link to the 1Password Style Guide or the base Apple Style Guide upon which ours is built. My intent here is to make it sound like we're all writing in one voice. Feel free to disagree. Ping me when you want me to take another look!

script/reviewer.go

michaelAbon1p · 2024-03-28T13:27:12Z

script/reviewer.go

+func (r *Reviewer) createComment(status Status) {
+	title := ""
+	body := ""
+	details := fmt.Sprintf("<details>\n<summary>Application data...</summary>\n\n```json\n%s\n```\n</details>", r.application.GetData())


On a second look here... you're making a GitHub comment using untrusted input from our users. What's to prevent them from adding their own HTML or XSS attacks here? Which user is making these comments and could be hijacked? I don't think that GitHub supports slash commands like GitLab (where they could force your bot to /approve the issue), but am I missing something there?

GitHub has extremely strict markdown sanitization rules (otherwise any Issue/PR/Readme could XSS you). GH doesn't directly support any slash commands that perform actions, however, if we implemented chat/slash commands with GH Actions down the road, yes this could be a problem.

Great callout! By and large json.MarshalIndent, used by the GetData method, escapes <, >, & and produces valid JSON, reducing the chances of breaking out of the printed JSON.

That said, I've hardened this even further by parsing all input data as Markdown into HTML, then retrieving only the text content of the HTML. This will help:

Protect us against Markdown syntax being used to break out of the JSON. After all, the JSON is itself being printed inside of a Markdown code block in the issue comment, so a user might have tried to use three backticks to end the code block.

Protect consumers of this data down the road are better protected if they directly render one of the JSON values (like the project description).

Which user is making these comments and could be hijacked?

We are employing a bot account to make these comments. A PAT for that account is stored as a secret on this repository and passed to the script when the workflow is invoked. Absent this PAT the script will bail.

I don't think that GitHub supports slash commands like GitLab (where they could force your bot to /approve the issue), but am I missing something there?

On the approval side, the workflow to approve and commit the data to the repository is only invoked when the status: approved label is added to the issue, and labels can only be added by a collaborator on the repository. Only AgileBits employees can be collaborators on the repository.

MNThomson

LGTM after those few comments

MNThomson · 2024-03-28T17:27:22Z

script/reviewer.go

+	if status == Invalid && r.application.IsValid() {
+		if err := r.gitHub.RemoveIssueLabel(LabelStatusInvalid); err != nil {
+			r.printErrorAndExit(
+				fmt.Errorf("could not remove issue label '%s': %s", LabelStatusInvalid, err.Error()),
+			)
+		}
+	}


Any reason to not move this block to line 93 where it is also inside the if r.application.IsValid() block?

Nice catch 👍

MNThomson · 2024-03-28T17:29:31Z

script/reviewer.go

+	// TODO: replace FILE_NAME with Application.FileName once available
+	dataPath := fmt.Sprintf("https://github.com/1Password/1password-teams-open-source/blob/main/data/%s", "FILE_NAME")


Is this TODO for this PR or for the future?

Future, once #925 is merged

Update: that PR was merged so I've updated!

MNThomson · 2024-03-28T17:30:56Z

script/reviewer.go

+		}
+	} else {
+		title = "### ❌ Your application is invalid"
+		body = fmt.Sprintf("\n\n%s\n\nOur automated pre-checks have detected the following issues:\n\n%s", details, r.application.RenderProblems())


Perhaps some sort of "Please fix these problems by updating the PR" language would be helpful

Nice one. Updated to add:

Update this issue to correct these problems and we’ll automatically re-evaluate your application.

…issue, based on issue status and validity

…using custom Status type

tatus/validity check when creating comment

…h to application file

michaelAbon1p

Awesome. Thanks for being so open to feedback here. The code looks good to me at this point.

jodyheavener added the 2024-program-updates Updates to the open source program in 2024 label Mar 26, 2024

jodyheavener force-pushed the jh/application-label-comment branch 2 times, most recently from fda14fb to 8aa1341 Compare March 26, 2024 18:27

schneedotdev approved these changes Mar 26, 2024

View reviewed changes

michaelAbon1p reviewed Mar 27, 2024

View reviewed changes

jodyheavener requested a review from michaelAbon1p March 27, 2024 17:24

michaelAbon1p reviewed Mar 28, 2024

View reviewed changes

MNThomson approved these changes Mar 28, 2024

View reviewed changes

jodyheavener added 10 commits March 28, 2024 10:48

Update format of validation error printing

bbbc9bb

Update review process for bot to modify labels and leave comments on …

d9c0b87

…issue, based on issue status and validity

Update representation of status values from strings to int constants …

14a106c

…using custom Status type

Copy updates

a612cdf

Update position of s

65ddc72

tatus/validity check when creating comment

Copy updates

7c57329

Sanitize all user input

56e5a13

Move validity check in updateLabels

f52b011

Update copy when application is invalid

53cba32

Update logic and copy for closed/approved applications to include pat…

26f627a

…h to application file

jodyheavener force-pushed the jh/application-label-comment branch from 87b528d to 26f627a Compare March 28, 2024 18:02

jodyheavener requested a review from michaelAbon1p March 28, 2024 18:03

michaelAbon1p approved these changes Mar 28, 2024

View reviewed changes

jodyheavener merged commit 14c4961 into program-updates Mar 28, 2024
1 check passed

jodyheavener deleted the jh/application-label-comment branch April 2, 2024 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up application labelling and commenting #924

Set up application labelling and commenting #924

jodyheavener commented Mar 26, 2024 •

edited

Loading

schneedotdev left a comment

michaelAbon1p left a comment

michaelAbon1p Mar 28, 2024

MNThomson Mar 28, 2024

jodyheavener Mar 28, 2024

MNThomson left a comment

MNThomson Mar 28, 2024

jodyheavener Mar 28, 2024

MNThomson Mar 28, 2024

jodyheavener Mar 28, 2024

jodyheavener Mar 28, 2024

MNThomson Mar 28, 2024

jodyheavener Mar 28, 2024

michaelAbon1p left a comment

		// TODO: replace FILE_NAME with Application.FileName once available
		dataPath := fmt.Sprintf("https://github.com/1Password/1password-teams-open-source/blob/main/data/%s", "FILE_NAME")

Set up application labelling and commenting #924

Set up application labelling and commenting #924

Conversation

jodyheavener commented Mar 26, 2024 • edited Loading

Summary

What's changed?

Testing

valid-project

✅ Your application is valid

valid-project-reviewing

👍 Application still valid

valid-project-closed

valid-project-approved

invalid-examples-1

❌ Your application needs some work

schneedotdev left a comment

Choose a reason for hiding this comment

michaelAbon1p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MNThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelAbon1p left a comment

Choose a reason for hiding this comment

jodyheavener commented Mar 26, 2024 •

edited

Loading

`valid-project`

`valid-project-reviewing`

`valid-project-closed`

`valid-project-approved`

`invalid-examples-1`