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

lib: Allow parent scopes when checking if each required scope is set #38

Merged
merged 5 commits into from
Dec 12, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Dec 6, 2023

Description of the Change

  • When checking for required scopes (were repo, user:email and read:org before this PR), allow parent scopes "instead".
    • (repo is already top-level)
    • (user is the parent scope of, and implies, user:email. Accept user "instead of" user:email if user:email isn't narrowly/explicitly found on the token.)
    • (admin:org is the parent scope of, and implies read:org. Accept admin:org if read:org not explicitly/specifically seen on the token.)
    • Warn in the console which exact scope was missing. (See below why I didn't put that in the UI.)
  • Show a hint in the UI when a token with insufficient scopes has been entered.
  • Warn in the console if parent scopes of our actually required scopes are put on the token, since that's excessive permissions, and we won't even use those extra permissions. (Bad security posture for our users to do this.) (Kind of unlikely anyone will ever see this, but oh well... Harder to investigate and implement the right way to do this in the UI.)
  • Allow just public_repo scope instead of full-on repo (but accept repo as its parent scope).
  • Pre-populate the recommended scopes in the "new Personal Access Token" link in the token entry view

Screenshot or Gif

Hint in token entry view when token is missing required scope(s) (click to expand):

Token entry view in the github package for Pulsar, showing a hint "Hint: Entered token has insufficient scopes. Update the scopes on your token and try again. See Dev Tools console for details."

Message in Developer Tools console when missing a required scope (click to expand):

Message in Developer Tools console "GitHub token doesn't have a required scope! Missing: public_repo"

Warning in Developer Tools console when excessive scopes are set on the token (click to expand):

Warning about excessive scopes in Developer Tools console

Applicable Issues

Mostly fixes pulsar-edit/pulsar#801.

Alternative Approaches

NOTE: I did not update the recommended scopes, since I haven't thoroughly tested what might subtly break if not having full repo scope, and this allows just setting public_repo, read:org and user:email. I don't have time to test what a sensible minimum is, and for folks who use private repos, I don't want to have to have a page long explanation of all the intricacies of the different scopes and what they do, especially since the token entry view does not have a scroll bar at the moment!!! Too long of a blurb puts the actual token entry box off of the screen on certain display sizes!!!!!!!!!!!!!!!! (!!!)

The warning info in the UI could show the specific missing scope, but that would have to be plumbed into the UI message. That would involve more work to send the info along from the login model and the call site, where we can know what the missing scope is, to the called view, which currently doesn't know that info... It needs to know that info at its render time, in order to renderer the specific scope into the UI.

We could also warn in the UI if a totally invalid token (typo'd, or expired/deleted) is entered, but most of the existing code assumes "maybe it was just a network error" as the same status as "definitely an invalid token" --> either is just "maybe an invalid token". The package doesn't try to distinguish/leaves the situation there, and it acts like you haven't entered anything at all. So, this would take more work than simply plugging in existing bit A to existing bit B, we would have to teach some part of the code to recognize a non-empty token that is wrong even after there was no network error during checking the scopes over the network (it's an API request to the GitHub REST API server). i.e. we need to define a state for when we got a response back from GitHub that the token is actually genuinely (confirmed) invalid.

I kept it relatively simple for this PR. The above is definitely doable, I just wasn't sure if it would be worth it and kind of ran out of steam to do it right here/right now, to be honest.

Misc Notes about scopes

  • Just public_repo is all that seems to be needed from my testing, unless you're working with private repositories.
  • I suppose read:org is needed for working with org-owned repos, but I didn't test this to confirm?
  • Not sure what the reason for why user:email is needed is, TBH, but too afraid to ask (ain't got time to look this up right now, to be honest.

I'd rather folks not put more scopes on their tokens
than they actually need... But the fact this doesn't work is confusing.

We've had to troubleshoot this scenario with users multiple times.
And that's just the ones who reached out about it. Others are likely
affected and unsure how to log in.

This allows supersets of the actually required scopes.
Note to users: Please just set the actual needed scopes...
Just 'public_repo' is pretty-well sufficient to interact with the
GitHub integration this package provides.
Also log in the console which scope is missing.
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I'm not the most familiar with this codebase, the changes look solid, for something that is sorely needed. So quick approval from me to get this into core as soon as we can. Normally while I'd love to see passing tests, I know that isn't the case in this repo just yet.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Dec 12, 2023

Thanks for the review! I did manually test this and it's working for me (see screenshots).

I may be bumping dugite in this package soon, and maybe superstring also (???), so we'll see what all ends up in github package repo, and then get at least some of it out to a bump for core.

Regular release days is up in four days, so we'll see what all I get to and feel comfortable throwing into Regular without it riding in Rolling for any real amount of time. But it's certainly doable. (Time permitting with other things that might come up, etc.)

Anyhow... merging! Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub Personal Access Token not working
2 participants