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: Update login instructions for PATs, not OAuth #15

Merged
merged 2 commits into from
Mar 1, 2023

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Feb 19, 2023

Please be sure to read the contributor's guide to the GitHub package before submitting any pull requests.

Requirements (from template, click to expand):
  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.

Description of the Change

Update login instructions to direct users to generate a Personal Access Token via github.com/settings/tokens, instead of via the unavailable github.atom.io/login (OAuth token generator) page.

Also: Add written directions about which scopes are needed, as users will now have to manually select the scopes they need on their own tokens in order for the github package to function right (and not reject the token for having insufficient permissions/scopes).

(The OAuth flow via github.atom.io/login used to specify the scopes without user input or control, other than the choice to grant or not grant access. Now we must advise users of which scopes are needed.)

Screenshot or Gif

Before (click to expand):

GitHub package tab showing instructions for the github atom io slash login login flow

After (click to expand):

GitHub package tab showing instructions for the github.com slash settings slash tokens login flow

Applicable Issues

Fixes #7

See also: (essentially fixes): atom#2686

NOTE: I included the workflow scope in these new instructions. The old github.atomio/login OAuth token did not have this scope. The workflow scope was separated out from the general repo scope after maintenance of the official Atom github package had already diminished, so it was never fixed there. Setting the workflow scope on a PAT and using that instead of the OAuth token was the very same best workaround as was available for some years, so I rolled that into this PR. (See again issue atom#2686).

workflow scope is not needed unless you need to push updated GitHub Actions YAML files up to your remote, however.

So these instructions go beyond bare minimum permissions/scopes needed to get up and running and do things ™️ with the github package, but it's just really confusing when [only] pushes that touch your GitHub Actions workflow YML files fail, potentially with a confusing or inaccurate error message as to why. And explaining it is harder than telling folks to set the permission.

So rather than delve into these fine details that are hard to explain and which people may not wish to think hard about when they don't have to... I just instructed folks to set the workflow scope whether they need to or not.

Much like the package could easily be changed to only require public_repo scope for repo access instead of the catch-all repo scope.

TO-DOs

Maybe explain when/why the workflow scope is needed in these instructions. (I dunno, man.)

Maybe update the app to require public_repo rather than full-on repo in terms of repo access scopes, and explain when/why you need full-on repo (for private repo access, basically).

NOTE: I consider both of these OUT OF SCOPE for the current PR. I respectfully ask that reviewers not ask that these be added to this PR, in order to avoid "scope creep" (pun not intended, but I'll take it) of this PR, review complexity, haggling over finer details, please and thank you. This is the targeted fix aimed at enabling folks to get authed in the github package without knowing ahead of time these precise instructions, or having to find them on the Discord somewhere, etc. etc.

The old OAuth login flow via github.atom.io/login is now unavailable,
as of the Atom sunset.

Direct users to generate a Personal Access Token via
github.com/settings/tokens instead.

Also: Add written directions about which scopes are needed,
as users will now have to manually select the scopes they need
on their own tokens in order for the github package to function right
(and not reject the token for having insufficient permissions/scopes).

(The OAuth flow via github.atom.io/login *used to* specify the scopes
without user input or control, other than the choice to grant or not
grant access. Now we must advise users of which scopes are needed.)
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Feb 20, 2023

A quick reminder that the testing on this repo is not testing the code, it is rather b0rked.

CI status does not pertain to this PR's changes, and I of course consider fixing CI here rather out of scope for what I'm trying to do in this particular PR.

So this is flying blind in terms of CI, though you can attempt to run the tests locally I think the generous timeouts for the CI environment may come in handy. I forget how to activate those, though... Hm.

Testing Notes

Anyhow, IMO the best way to verify these changes is first off to see that they are valid HTML... and beyond that, I think manual testing is pretty appropriate here. Clone this PR branch, cd into it and do ppm install (wait for it to finish), then ppm link, make sure you find and rename or delete your existing saved github package token (see this comment atom#2686 (comment) for how to view/edit tokens on each OS) so it shows the login screen, reload (or quit/relaunch) Pulsar, verify that the package doesn't crash when you try to click through to the login screen, and lastly verify that the login screen matches my "After" screenshot from above).

Comment on lines 66 to 71
<li>Visit <a href="https://github.com/settings/tokens">github.com/settings/tokens</a> to generate
an authentication token.</li>
<ul>
<li>Generate a new token (classic) with at least the following permissions:
<code>repo</code>, <code>workflow</code>, <code>read:org</code>, and <code>user:email</code>.</li>
</ul>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<li>Visit <a href="https://github.com/settings/tokens">github.com/settings/tokens</a> to generate
an authentication token.</li>
<ul>
<li>Generate a new token (classic) with at least the following permissions:
<code>repo</code>, <code>workflow</code>, <code>read:org</code>, and <code>user:email</code>.</li>
</ul>
<li>
Visit <a href="https://github.com/settings/tokens">GitHub</a> to generate a new
<a href="https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/creating-a-personal-access-token#creating-a-personal-access-token-classic"> Personal
Access Token (classic)</a>.
</li>
<li>
Ensure it has the following permissions <code>repo</code>, <code>workflow</code>, <code>read:org</code>, and <code>user:email</code>.
</li>
<li>
Enter the token below:
</li>

I wonder if this could be made a bit more concise by not repeating instructions ("generate token" is mentioned twice) and could be more helpful with a link to the GH docs on how to actually create one.
I also feel the <ul> is a bit weird as it feels like a separate step but it isn't nested with the previous item and doesn't have a marker.

The above suggestion would look something like:
image

Copy link
Member Author

@DeeDeeG DeeDeeG Mar 1, 2023

Choose a reason for hiding this comment

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

I liked the simplified wording, thank you for that.

Also liked some of the HTML refactoring to be slightly neater, but in terms of whitespace-only suggested changes, I decided to stay with same-line/end-of-line opening/closing tags in some places, both for visual conciseness and to match surrounding HTML (JSX) code style. If we wanna refactor the use of whitespace and indentation, then I'd like it to be mostly consistent throughout the file.

I felt that the link to make the token being visually smaller than the documentation link was a no-go for user experience (UX) reasons, so I moved the docs link into its own <sup></sup> element to make it look like a "footnote" or "further reading" link, and restored suggested link text "GitHub" back to "github.com/settings/tokens" -- I like that this shows you exactly where on github.com you need to go, in case you don't realize you can click through the link or your default URL handler/browser is badly configured (??), or in case some thing about the handoff to your browser breaks, but also to make this link visually dominate (by virtue of being much larger than the "[docs]" link), since that's where we want people to go, and I don't want to "make people think" in the immortal words of Steve Apple, may he rest in peace. (Edit: Or maybe it was Steve Krug, and his book of that name, actually?)

Screenshot:

github package login view showing instructions and a token entry field, Cancel and Login buttons

Diff:

This comment is discussing changes I committed to this PR's branch in commit 249a986.

Copy link
Member

@Daeraxa Daeraxa Mar 1, 2023

Choose a reason for hiding this comment

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

I think that is a good hybrid of both, seems good to me. I agree with the reasoning.

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.

I'm gonna leave a quick approve here.

I've been excited to see these changes for a while now, so super happy you are able to find the time, as I know for a fact there are some users waiting on this.


Otherwise for future changes, I would love to see a document on our website linked that could maybe explain the differences you mentioned above in greater detail for the few that will actually care.

But otherwise for now, this is perfect. It gets the job done, and does what it needs to. Like you said much more is scope creep and we can focus on another time if it becomes needed.

PS. My approval still applies if the wording is changed slightly like other reviews are mentioning, as long as it still makes sense in this general direction I'm not to partial on the exact language used as long as it simply and accurately conveys to end users what they need to do to get this to work.

Thanks a ton @DeeDeeG

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Mar 1, 2023

Okay, I got two approves (a UI one and a verbal one), and after I got around to reviewing the feedback (sorry for the delay!), it seems this is in a good place to merge.

Merging now!

Thank you for reviews/feedback!

@DeeDeeG DeeDeeG merged commit 39e29f5 into master Mar 1, 2023
@confused-Techie
Copy link
Member

Now to actually bump the github version in Pulsar

@Spiker985 Spiker985 deleted the update-login-flow-instructions-for-PATs-not-OAuth branch March 28, 2023 23:36
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 token generation is now broken
3 participants