Remove the need for a custom `SANDPAPER_WORKFLOW` PAT

[Bisaloo]

Currently, the update-cache.yaml and update-cache.yaml workflows require a custom PAT:

sandpaper/inst/workflows/update-workflows.yaml

Line 52 in aacaff0

token: ${{ secrets.SANDPAPER_WORKFLOW }}

sandpaper/inst/workflows/update-cache.yaml

Line 98 in aacaff0

token: ${{ secrets.SANDPAPER_WORKFLOW }}

But for the purpose of creating the pull requests, the default GITHUB_TOKEN token, provided in each GitHub Actions run, is sufficient, as shown in this example repo: Bisaloo/tutorials-early#2

Some extra work is required to trigger the other workflows that normally run when a pull request is opened since they won't run automatically when the pull request is created with GITHUB_TOKEN. However, it is still possible by manually triggering them (adding a workflow_dispatch: trigger in the workflows to start & running gh workflow run in the workflow opening the pull request).

Overall, the code would not be much more complex but there would be two immense benefits:

• the creation and setting of the custom PAT is a very common source of pain for community members who start to use the workbench for externally hosted lessons. We regularly get message in the #workbench slack channel about this
• having a custom PAT adds a single point of failure since it is attached to a specific individual. If this individual leaves the organization, the workflows will stop running

Relying on the default PAT would immediately resolve these two pain points.

What do you think? Is it something you'd like to receive a PR for? Are there other benefits of the custom PAT that I'm missing?

[froggleston]

I think @zkamvar would be better able to answer this as it points to the original rationale of the Workbench. I'm always up for removing pain points for users! 😃

However, I'd like to advocate for the use of the new Docker container for the Workbench GitHub Actions as this would largely (completely?) remove the need for the SANDPAPER_TOKEN - the full "stack" for building and maintaining a lesson repo would be contained within a managed and "safe" containerised environment.

[zkamvar]

Hi @Bisaloo, the tokens are required precisely because they trigger the pull request and downstream workflows to run. Moreover, the update-workflows workflow cannot be triggered from the default GITHUB_TOKEN because it requires workflows: write permissions (as is shown in the documentation for these workflows). IIRC, you cannot push a workflow file with the default token.

The downstream workflows and the PR comments they create are important for quality control, especially for package updates, where they will show what package updates cause output to change. I would be interested to see how the workflow_dispatch trigger would work WRT to the pull requests, ensuring that the pull request is correctly triggered.

I will admit that the token shenanigans is a bit of a mistake on my part because these aren't easy concepts to work with for novices. One thing that I've wanted to do was to create a bot that would do all of these things (updates, PR previews) on a fork in its account, which would bypass the need for tokens and avoid the confusion with all of the md-outputs-PR-NN branches.

[Bisaloo]

The downstream workflows and the PR comments they create are important for quality control, especially for package updates, where they will show what package updates cause output to change.

Yes, 100% agree. This is a great feature and it needs to stay.

I would be interested to see how the workflow_dispatch trigger would work WRT to the pull requests, ensuring that the pull request is correctly triggered.

I have a (very dirty) proof of concept here: https://github.com/Bisaloo/demo-workbench-notoken. It would definitely need a lot of cleaning before potentially making its way to sandpaper. I have disabled a couple of security features I couldn't understand at this time but I'm quite confident they can be added again. I would also split the git history into more easily understandable changes.

I wonder more generally how bad the current proof of concept is in term of security, and how much this could be mitigated. My understanding is that the workflows are structured the way the are, successively calling one another, because you are given a lot of thought to the security implications.

Moreover, the update-workflows workflow cannot be triggered from the default GITHUB_TOKEN because it requires workflows: write permissions (as is shown in the documentation for these workflows). IIRC, you cannot push a workflow file with the default token.

Interesting, I didn't investigate this one. The main priority for me would be to get at least the update-cache workflow to work without a token, as it is the one that actually generates a PR almost every month. The workflow updates are less common/

[froggleston]

I've made good progress on the docker container workflows which would remove the need for SANDPAPER_TOKEN for all workflows except the update-cache. You can see examples of my works-in-progress here (new workflows prefixed by docker-: https://github.com/froggleston/R-ecology-lesson/tree/main/.github/workflows

I'd also be very interested to understand how this last token could be removed. As the PR is raised in a new branch, and as the update-cache workflow in the "new model" in my mind would only ever be run manually when users wanted to update their cache, I'm wondering what the attack vectors would actually look like without this token and rather using the GitHub supplied ones....

The most common problems we see with users' builds are that recently updated R versions or packages break because there's no option in the current GitHub actions to avoid remotes::install() and sandpaper::manage_deps() that are performed by sandpaper-main. The new docker workflows remove this, relying on staged renv cache updates and renv::restore() instead.

I'm happy to discuss this more!

[Bisaloo]

I'd also be very interested to understand how this last token could be removed.

Here is a slightly more guided explanation in case that's useful.

The most important change here is using the gh workflow run command to manually trigger the next workflow instead of relying on the automatic trigger:

https://github.com/Bisaloo/demo-workbench-notoken/blob/f6075d0924d8ac083bb17092fc54b6d8e04d5932/.github/workflows/update-cache.yaml#L120-L124

      - name: Trigger checks
        env:
          GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
        run: |
          gh workflow run pr-receive.yaml --field pr_number=${{ steps.cpr.outputs.pull-request-number }}

The nice thing is that this can be done with the default token, provided it has the actions: write permission.

The other changes deal with passing parameter and handling the fact that the PR number is extracted differently depending on the trigger, but that's more "boring" code.

[froggleston]

Sadly we cannot remove this token from the update_cache and update_workflows steps due to the need for the carpentries-bot to raise the initial PRs. If the default github-actions bot raises them, then this PR creation workflow cannot trigger other workflows.

[Bisaloo]

It's a bit blurry in my memory now and I would need to dive into it again if necessary, but I think I got it working in the example posted above: Bisaloo/demo-workbench-notoken#10. Or am I still missing something there?

[froggleston]

Ah I see you have the added final step to manually run the pr-comment workflow! Sorry I missed this in my first pass. I'll go back to it :)

[froggleston]

I think the only issue left is that the PR Preflight cannot be called from a PR that has been opened by github-actions, as the preflight checks need to happen on open, close and sync (important if a PR is opened then subsequently a commit is pushed that modifies workflows).... I'm not sure how to get around this other than going back to PATs? (Or as Zhian mentioned, use a fork in the carpentries-bot account)

[froggleston]

OK I think I've reached an acceptable point.

froggleston@d994ece#diff-94236286da5f62247fb5811b85d567a770c6b0d98a216d6f6f374a21847db8b8R146

We now have three options for "who" raises the PR:

• Firstly, any SANDPAPER_WORKFLOW classic PAT in the repo will be used as a priority, which will mean the "who" is the repo/fork owner and all downstream triggers will work in all cases.
• Secondly, the new AWS-stored Carpentries Bot token handled by OIDC that will be available for all repos within our core GitHub organisations managed by the Carpentries core team (including the three current lesson programs). This is something I've been working on recently, and will help current official lesson maintainers by them not having to worry about their tokens at all.
• Thirdly, as a fallback, the default GITHUB_TOKEN which will work for most of the use cases except for PR preflight checks on PR commits in forks and personal repos. Whilst their lessons will build, we would advise that anyone wanting to secure their own lesson repos create a SANDPAPER_WORKFLOW token, as is the current situation. This would also need to be the case for the update_workflows workflow.

What do you think?

[Bisaloo]

As a user, this sounds quite convenient 👍. I hope this doesn't increase too much the code complexity and keeps the workflow code maintainable but I'll leave this up to you.

[froggleston]

Given the new workflows being used in a couple of real world places now, I think I'd be willing to close this off?

I agree about the complexity - the new workflows are more complicated in a way, but are simpler and more informative for maintainers I believe. Error messages are clearer, the use of PATs is greatly simplified (especially for official carpentries lessons and those in the incubator and lab).

However, there's still work to do! I'd like to move a lot of the complexity into carpentries/actions composite or reusable workflows. This would make it even more likely that these new workflows will need little to no updating over time.

Thank you so much for your contributions, help and testing @Bisaloo ! They are as always very much appreciated and invaluable!