Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GitHub Personal Access Token not working #801

Closed
5 tasks done
sunnystew22 opened this issue Nov 14, 2023 · 12 comments · Fixed by pulsar-edit/github#38
Closed
5 tasks done

GitHub Personal Access Token not working #801

sunnystew22 opened this issue Nov 14, 2023 · 12 comments · Fixed by pulsar-edit/github#38
Labels
bug Something isn't working

Comments

@sunnystew22
Copy link

Thanks in advance for your bug report!

  • Have you reproduced issue in safe mode?
  • Have you used the debugging guide to try to resolve the issue?
  • Have you checked our FAQs to make sure your question isn't answered there?
  • Have you checked to make sure your issue does not already exist?
  • Have you checked you are on the latest release of Pulsar?

What happened?

I literally CANNOT authorize a GitHub Personal Access Token. Please help.

Pulsar version

Pulsar 1.110.0

Which OS does this happen on?

🪟 Windows

OS details

Windows 11 22H2 22621.2428

Which CPU architecture are you running this on?

x86_64/AMD64

What steps are needed to reproduce this?

  1. Simply click the GitHub button when Pulsar opens
  2. Enter a GitHub Personal Access Token

Issue reproduced

Additional Information:

Sgypc6VR4B

@sunnystew22 sunnystew22 added the bug Something isn't working label Nov 14, 2023
@Daeraxa
Copy link
Member

Daeraxa commented Nov 14, 2023

Does your token have too many permissions? i.e. Does your token have just the permissions mentioned in the instructions or did you give it everything?

@sunnystew22
Copy link
Author

Every permission. Should I just give it the permissions it says?

@sunnystew22
Copy link
Author

I FIXED IT! THANKS @Daeraxa!!

@sunnystew22
Copy link
Author

I think the Pulsar team should add a warning that you can only add those permissions.

@Daeraxa
Copy link
Member

Daeraxa commented Nov 17, 2023

I was thinking something similar but I'm not sure we know which one (or which combination) is causing it to not work. So it might be possible (useful??) to add more than the minimum.

@DeeDeeG
Copy link
Member

DeeDeeG commented Nov 27, 2023

Please also regenerate the token, if you haven't already, since those are meant to be secret, and the illustration you provided, which includes the token, can be used to impersonate or "authenticate as" you on GitHub's various services, including github.com.

@DeeDeeG
Copy link
Member

DeeDeeG commented Nov 27, 2023

So. Here's what I see is happening, after some debugging:

It looks like having the token lets you query the GitHub API and look up the token's scopes... and the amount of info GitHub returns to you about the scopes is a bit economical... the returned scopes don't explicitly include every "child" or "nested" scope if you have set a "parent scope" that includes it.

So, when folks are setting the parent scope admin:org (which includes all of write:org, read:org, and manage_runners:org)... github package checks for just the child scope read:org, and doesn't explicitly see read:org, and thinks the token has insufficient access. (github package doesn't understand that the admin:org scope implies read:org scope).

Some options to deal with this:

  • We could fall back to check for the parent scope admin:org in this case (when the child scope read:org isn't explicitly listed)...
  • Or add to the instructions that setting more permissions than strictly needed will NOT work...
  • And/or find a way to more loudly warn that excessive scopes have been added, (and must be removed, if we want to be explicit and strict about not allowing excessive scopes to be set).

Warning loudly about excessive scopes would be good, in my view, since honestly I'd rather not have folks out there putting an "if this token leaks, you basically own everything in my account" level of token into the github package, where it's not needed in the slightest... I don't want to encourage poor token practices such as setting super excessive permissions.

@DeeDeeG
Copy link
Member

DeeDeeG commented Nov 27, 2023

I also intend to update the "Make a token" link in the login instructions to point to this: https://github.com/settings/tokens/new?scopes=repo,workflow,user:email,read:org&description=Pulsar%20github%20package

It pre-selects exactly the permissions we need, no more no less.

@savetheclocktower
Copy link
Contributor

Having a token with “too many” permissions fail to work is something I'd find completely flummoxing. I wouldn't think “oh, they're just making sure I create a token with minimal permissions”; I'd think something much more fundamental had gone wrong, and I'd probably waste time trying to troubleshoot the issue in places other than the github package.

I think we should check for the parent admin:org scope. If we want to discourage tokens with blanket permissions, we should do so explicitly, rather than by sheer accident. Changing that link to pre-select the minimal set of permissions is the perfect way to guide users toward the right decision; if they ignore our advice, it's on them.

@DeeDeeG
Copy link
Member

DeeDeeG commented Nov 28, 2023

We could also explore the impact of not requiring read:org at all.

I feel the github package was written the way it was mostly for convenience, but with not much thought toward minimizing permissions on the token. If the experience is smooth without it, we can simply drop the requirement. Knowing what doesn't work without it would clarify how best to move forward with this option, but I don't have time to debug this part right now.

I do agree that a pretty good and very simple way forward is definitely to also allow admin:org where we check for read:org. So if I don't get time to dig into this more, that's a helpful quick fix, along with tweaking the link to nudge people even more in the right direction.

@Daeraxa
Copy link
Member

Daeraxa commented Nov 28, 2023

Having a token with “too many” permissions fail to work is something I'd find completely flummoxing. I wouldn't think “oh, they're just making sure I create a token with minimal permissions”; I'd think something much more fundamental had gone wrong, and I'd probably waste time trying to troubleshoot the issue in places other than the github package.

I agree with this take, any "superset" of a permission should work (even if it isn't a "real" superset as @DeeDeeG describes). Whether it is a good idea or not is another matter - for example if we did want to police this and say "please don't make a god token just for the GH package" then we should be detecting the unecessary scopes and failing with a descriptive error.

I also intend to update the "Make a token" link in the login instructions to point to this: github.com/settings/tokens/new?scopes=repo,workflow,user:email,read:org&description=Pulsar%20github%20package

It pre-selects exactly the permissions we need, no more no less.

Really like this approach but I do also think we need to be detecting those extra scopes and either failing with a useful error or succeeding with "parent" scopes.

@DeeDeeG
Copy link
Member

DeeDeeG commented Nov 28, 2023

The challenge with giving good feedback on the token is... the O.G. package's UI just seems to do nothing when you enter an "invalid" token.

Posting an error message in the console would be easy (I was rather surprised they don't already log anything!). This would be an improvement. One more thing to do even in a quick pass for this issue, I suppose.

What would be really nice, given how smooth and inviting the UI appears to be (and is, when the token is accepted)... what would be really nice is some feedback in the UI itself. Maybe a little infobox/popup warning over the text input field?

UI wrangling is a bit outside of my familiarity with this package, so I think that would be more of a long-term goal...

EDIT: there is supposed to be a message "Your token no longer has sufficient authorizations. Please re-authenticate and generate a new one." shown, seemingly in this exact circumstance. Hopefully we can wire it up properly so that shows... (Long-term goal). EDIT 2: It does show, see gif OP posted above. It just shows on an earlier screen than the actual token input screen. The token input screen should be updated to show feedback about this as well, IMO.

The current to-do list for a quick-pass fix, as I see it:

  • Accept admin:org if read:org isn't detected.
  • Update the link to pre-select the exact scopes we need.
  • Log something in the console if the package thinks the token had wrong permissions (such as: "missing xyz scope")

Long term to-dos?:

  • Feedback in the UI about the token not being accepted
    • Feedback for wrong scopes [✅ EDIT: got this working]
    • Feedback for net error/couldn't check scopes
    • etc.?
  • More logging to the console in general?
    • Adding a bunch of console.log() around was extremely helpful for debugging purposes just now, but it might be a bit spammy to log anything other than final errors. That said... We could check for an env var DEBUG or DEBUG_GITHUB and only log the non end-state-error stuff if that env var is set?

This is something I'm working up to doing a PR for, once the dust settles a bit on vision for what a good quick-pass PR would look like.

EDIT to add: The relevant bits are in lib/models/github-login-model.js, lib/views/github-tab-view.js and lib/views/github-login-view.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants