[BUG] Danger doesn't work with App tokens anymore about danger-js HOT 22 CLOSED

Yeah I wasn't claiming it was invalid, I totally see it as a valid possible config. I'll double check the perms on the app. Thanks.

from danger-js.

It definitely has access (read-write to pull requests, which includes comments)

from danger-js.

I have an action that looks for it's own comments by a magic string (an HTML comment) in the body, and it works well. It's a much much simpler action than Danger, and JS isn't a strong language of mine, so I haven't quite been able to follow Danger's code well enough to make the change, but I think it'd be a reasonable solution.

from danger-js.

Well in getDangerCommentIDs we call getUserID() so that we can use it in the filtering.

However, it's not necessary as we already check to make sure the body includes a check for the magic string DangerID: danger-id-Danger; we put in every comment in this filter. That combined with the filters to make sure it looks like a Danger comment in other ways should be sufficient.

Oh and the same logic is used to find inline PR comments. Again, I believe we can drop the call to /user (aka getUserID), which would make this work with Apps.

from danger-js.

I've done this in #1435

from danger-js.

@orta - Can you weigh in on if this is expected? I'm not bugging you for a fix, necessarily, I know this is all volunteer-time OSS, and I'm very grateful for it. But just knowing if this is expected or not would be super helpful. Thanks!

from danger-js.

@jaymzh this 403 error log isn't necessarily a direct indicator that App Tokens will never work.

It's a bit of a confusing log message, but basically, when DangerJS gets the token, it doesn't know what kind of token it is. It hits a series of APIs to figure that out, and this error log pops out in several conditions, but it's not a direct concern (alone). I believe the log is being generated inside of Github's API stack despite the error condition being handled in DangerJS.

Most of the projects I've used DangerJS for have that error log message despite Danger working fine. That said, I haven't used App Tokens except for one hello-world project years ago. I mostly use the built-in token, followed by a bot-account token.

from danger-js.

There are some examples of expected 403 errors in the tests here, though I haven't verified if they're exactly the same as your case!

from danger-js.

Huh, that's an interesting observation. OK, I'll test a bit more deeply, and see if I can validate it's actually functioning completely. Thanks!

from danger-js.

OK after some testing, the problem is that when it can't determine its user, it fails to update existing comments and makes new ones. So this definitely impacts functionality.

from danger-js.

@jaymzh are you sure that your token has permission to edit/remove comments?

I don't fully remember, but when using the built-in Github Token (configured via permissions in the workflow), you could easily get into that state unless you added the right permission scope. Similarly, I had this issue when I first created a User Token for this, but it didn't have sufficient read + write / edit permissions.

DangerJS is used by so many people in so many ways, that behaving like this is actually an intentional feature. Indeed, some people/projects absolutely never want a comment to be edited or removed, and some CI providers don't even allow such edits -- even though edit/replace on comments is one of the top reasons that some people chose to use DangerJS.

So DangerJS needs to support multiple outcomes depending on how you configure it -- it's unfortunate that for your case, a configuration error (for your needs) is interpreted as a "valid" configuration for someone else's needs.

from danger-js.

Sorry - on a company onsite - I feel like it was not possible to find out the account of the app which was posting a very long time ago when I added support for app tokens as a GitHub app was new then.

Maybe I fixed it by doing some hardcoding the postee in Peril, but it does seem reasonable to either see if that's possible or I'm not against the idea of having app based tokens force Danger is simply look for its own comment to update via the post body?

from danger-js.

I'm tempted to suggest that we close this ticket, and instead delete the App Token method of using DangerJS, but perhaps I'm missing a valid use-case. (Peril covers some, while user-tokens cover others!) -- Because of the security sensitivity of a tool like DangerJS, I'm not sure we should offer a public App Token.

Sounds like this isn't a blocker for anyone right now. If somebody else comes along and is motivated, I'd be happy to help review solutions.

from danger-js.

@fbartho - GH support is requiring me to use an App, actually. Here's the details:

• We were getting lots of "you requested too many files" errors, after much debugging we realized it happened if a PR touched several workflow files. You can trigger this reliably if you touch > 8, but it often happens at even 2 or 3. We opened a support request with GitHub and they said this was a known issue and the solution is to request the workflow permission. But you cannot do this on the dynamic per-PR token! SO they told us to make a PAT.
• So then we moved Danger to a PAT, which is sub-optimal, but OK. Then we started getting mass-failures due to reaching API limits as we had too many in-flight PRs. We once again went back to GitHub who said there's no way to change these limits and instead we should use an App as they have 10x higher API limits.
• So now I'm attempting to use an App.

from danger-js.

@jaymzh Thanks for sharing your use-case! If I might ask: Is this all in a large monorepo? Do you commonly have huge PRs with many files edited?

Random low-value thoughts that are running through my brain based on your story:

• Ouch!!
• Github seems like it needs to improve support for monorepos, and projects with many Github workflows.
• Workflows editing other workflows seems a little dangerous. Since Workflows have access to all the secrets, you can imagine how this could spiral out to malicious actors doing bad things extremely easily™
• We had an auto-formatter that wanted to reformat Workflow files, but indeed, as you said, we couldn't request the workflow permission for per-PR tokens. When I switched to a PAT, I decided that a DangerJS based-PR-blocker was working fine for our team, so I stopped trying to make our workflows able to edit themselves.
• We had a "you requested too many files" issue a few years back when we shipped a new autoformatter that rewrote all the files in the repository (yay Prettier!). We handled that by doing a one-time team-based review of the whole change + running the tests locally. Then we landed the PR bypassing our normal workflows. After that, our project no longer had the issue.
• Normal PRs from humans were not allowed to edit that many files at once, so it was really automatic changes that were the problem.
• If this issue primarily happens on automated PRs instead of manual ones, maybe you can skip Danger for these automated PRs/users? -- At one place, we ended up having different workflows/rules depending on if a human contributed something or if it was a bot.

from danger-js.

No I think I wasn't clear or you misunderstood.

The workflow is not editing a workflow.

Make a PR like this:

for f in .github/workflows/*; do
  echo '#test' >> .github/workflows/$f
done

When anythign requests the list of files in the PR, you'll get an error of too many files, even if that was only 9 files.

from danger-js.

(it's quite common for us to modify several workflow files in a single PR - and SEVERAL of our checks fail, and we're trying to move them all to an App now because of this)

from danger-js.

so I think this is only done in GitHubAPI.ts and only to find Danger's comments. And since there's a magic string in all of Danger's comments, I'm pretty sure it's not needed.

from danger-js.

I suspect I have misunderstood something! Would you mind sharing exactly where "this is only done in GitHubAPI.ts"? Maybe if you show me which magic string you're talking about I'll better understand.

from danger-js.

Yeah, dropping the user check seems reasonable to me

from danger-js.

That's awesome, thanks!

from danger-js.

I've rolled this out and all works well. Thanks!

from danger-js.