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

git-webkit setup: Fix various pitfalls with credentials input #779

Conversation

ntrrgc
Copy link
Contributor

@ntrrgc ntrrgc commented May 19, 2022

3ab5d8e

git-webkit setup: Fix various pitfalls with credentials input
https://bugs.webkit.org/show_bug.cgi?id=240574

Reviewed by Jonathan Bedard.

Yesterday I tried to run `git webkit setup`.

To put it mildly, it wasn't a smooth ride. I ended up having to debug the
tooling for hours just to be able to get it running.

This patch fixes several issues I found during the process, so that the next
unlucky person doesn't have to go through this again.

1. Whenever a request failed, the response from the server was not shown in
anyway, instead printing an unhelpful generic message. This patch adds code to
write to the screen the responses obtained from the GitHub API, so that the
next person having problems with it doesn't need to add debugging code to know
what is wrong.

2. When copying and pasting tokens from the browser it's very easy to
accidentally grab some leading or trailing whitespace. This is especially easy
to miss for the blind terminal key prompt. This patch adds code to trim these
fields. This is generally good UX practice since leading and trailing spaces
are virtually always accidental. [1]

3. The validation code for GitHub username and token was not run under `git
webkit setup`. Looking at the code it's clear the intention was for that
validation to be done to check (1) a plain GitHub username is used instead of
an email address associated to that account and (2) that a test log-in
succeeds. But because the credentials function is called in many places, the
first instance that actually gets called happens to not set validate=True in
the arguments.

   Validating passwords that have just been input for the first time should not
be an optional feature that is disabled by default. Otherwise any mistake in
the credentials input can cause cryptic errors and the user is left on their
own to figure out what is going on, and eventually, how to manually interact
with the keychain to remove or edit the bogus username and/or token. This patch
makes changes so that validation is made whenever the user is prompt for
username and token, no matter in what codepath this becomes necessary.

[1] https://tonyshowoff.com/articles/should-you-trim-passwords/

* Tools/Scripts/libraries/webkitbugspy/webkitbugspy/bugzilla.py:
(Tracker.credentials):
* Tools/Scripts/libraries/webkitbugspy/webkitbugspy/github.py:
* Tools/Scripts/libraries/webkitcorepy/webkitcorepy/credentials.py:
(credentials):
* Tools/Scripts/libraries/webkitscmpy/webkitscmpy/program/setup.py:
(Setup.github):

Canonical link: https://commits.webkit.org/250750@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@294491 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@JonWBedard JonWBedard self-requested a review May 19, 2022 14:55
@ntrrgc ntrrgc force-pushed the eng/git-webkit-setup-Fix-various-pitfalls-with-credentials-input branch from 896ad78 to fb10eb4 Compare May 19, 2022 14:56
Copy link
Member

@JonWBedard JonWBedard left a comment

Choose a reason for hiding this comment

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

Want to make sure @sundiamonde sees this change, it's the thorough fix for what she encountered last week that I half-fixed in https://commits.webkit.org/250423@main

@ntrrgc ntrrgc added the merge-queue Applied to send a pull request to merge-queue label May 19, 2022
@webkit-early-warning-system webkit-early-warning-system merged commit 3ab5d8e into WebKit:main May 19, 2022
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/git-webkit-setup-Fix-various-pitfalls-with-credentials-input branch from fb10eb4 to 3ab5d8e Compare May 19, 2022 17:14
@webkit-early-warning-system
Copy link
Collaborator

Committed r294491 (250750@main): https://commits.webkit.org/250750@main

Reviewed commits have been landed. Closing PR #779 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label May 19, 2022
@ntrrgc ntrrgc deleted the eng/git-webkit-setup-Fix-various-pitfalls-with-credentials-input branch September 2, 2022 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants