Graphs.jl icon indicating copy to clipboard operation
Graphs.jl copied to clipboard

Trivial change, test PR

Open filchristou opened this issue 1 year ago • 1 comments

filchristou avatar Feb 26 '24 13:02 filchristou

@gdalle I am having some permission issue in the Graphs.jl repo.

The same workflow works for my fork Trivial change, test PR · filchristou/Graphs.jl@f0c892d · GitHub but doesn't in JuliaGraphs Trivial change, test PR · JuliaGraphs/Graphs.jl@f0c892d · GitHub.

If you go to the "Set up job" -> "GITHUB_TOKEN Permissions" I get "write" in my fork and "read" in JuliaGraphs

Did you specifically modify something because from what i read around the default permissions for public PR should be read/write https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token.

The specific error I get is "GraphQL: Resource not accessible by integration (addComment)" and there is some discussion here git - "Resource not accessible by integration" on github post /repos/{owner}/{repo}/actions/runners/registration-token API - Stack Overflow

filchristou avatar Feb 26 '24 14:02 filchristou

It's weird cause the actions indeed have read/write permissions in Graphs.jl as well as the organization in general. It's not the most secure setting but it should work

gdalle avatar Feb 28 '24 10:02 gdalle

I think this is the answer: https://github.com/cli/cli/issues/8374#issuecomment-1829442375

I think that you are opening a PR from a fork and expecting the workflow on: pull_request to have write permissions but as far as I know that will never happen for security reasons.

Over in https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token you can see that "Maximum access for pull requests from public forked repositories" is always going to be read.

It happens cause you're on a fork

gdalle avatar Feb 28 '24 11:02 gdalle

write-all doesn't change anything, I've tried it

The issue is that you're doing it from a fork, and so the comment posting will never work. In this case the best we can do is use the option from BenchmarkCI to print the benchmarking results inside the workflow log, in addition to the PR comment (which will only work from branches, i.e. for PRs made by maintainers).

gdalle avatar Feb 28 '24 12:02 gdalle

it seems the workflow didn't even run on those latest commits

gdalle avatar Feb 28 '24 14:02 gdalle

yeah. sorry for the noise. I will be playing around for a while. I am not satisfied with the logging public PR benchmarks in the workflow so I am investigating other possibilities. Let me know if you have any ideas.

filchristou avatar Feb 28 '24 14:02 filchristou

When a workflow is triggered by the pull_request_target event, the GITHUB_TOKEN is granted read/write repository permission, even when it is triggered from a public fork. For more information, see "Events that trigger workflows."

Might be worth a shot?

From https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

gdalle avatar Feb 28 '24 16:02 gdalle

probably it is worth a chance. Now I am considering a chain of workflows as described here: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

filchristou avatar Feb 28 '24 16:02 filchristou

why so complicated? I think pull_request_target is much easier

gdalle avatar Feb 28 '24 16:02 gdalle

I don't get what's the practical difference between pull_request and pull_request_target. An activity type of the pull_request_target is opened which I guess is the default created PR ? Also the docs mention

Avoid using this event if you need to build or run code from the pull request.

Which is exactly what benchmarking does. However, if I can't get the chained workflows to work I will start considering it.

filchristou avatar Feb 28 '24 16:02 filchristou

I have asked on slack: https://julialang.slack.com/archives/C681P8ABG/p1709137580258319

gdalle avatar Feb 28 '24 16:02 gdalle

On second thought I agree with you this is a bad security practice, let's not use pull_request_target

https://securitylab.github.com/research/github-actions-preventing-pwn-requests/

gdalle avatar Feb 28 '24 16:02 gdalle

@gdalle Okey. it seems that the workflow_run thingy will only work if that file is in the "default branch". For Graphs.jl that is the master.

Note: This event will only trigger a workflow run if the workflow file is on the default branch.

So either I need to start experimenting with the master or changing the default for a little while. What do you prefer ?

Good thing is that all these changes do not interacting with user code.

filchristou avatar Feb 28 '24 18:02 filchristou

Let's discuss it tonight at the community call if you can attend?

gdalle avatar Feb 29 '24 08:02 gdalle

okey. yes I will join

filchristou avatar Feb 29 '24 09:02 filchristou