JuliaGraphs / Graphs.jl

An optimized graphs package for the Julia programming language
http://juliagraphs.org/Graphs.jl/
Other
451 stars 87 forks source link

Trivial change, test PR #347

Closed filchristou closed 4 months ago

filchristou commented 5 months ago

@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

gdalle commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

filchristou commented 5 months ago

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.

gdalle commented 5 months ago

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

filchristou commented 5 months ago

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

gdalle commented 5 months ago

why so complicated? I think pull_request_target is much easier

filchristou commented 5 months ago

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.

gdalle commented 5 months ago

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

gdalle commented 5 months ago

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/

filchristou commented 5 months ago

@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.

gdalle commented 4 months ago

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

filchristou commented 4 months ago

okey. yes I will join