Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
check credentials before adding repositories #89
Conversation
albertodonato
requested a review
from
panlinux
Nov 24, 2017
| @@ -57,6 +57,16 @@ check_result() { | ||
| fi | ||
| } | ||
| +http_request_code() { | ||
| + curl -ILs "$1" | awk '/^HTTP\// { code=$2; }; END { print code; }' |
panlinux
Nov 24, 2017
Contributor
We should check if we can add curl as a dependency of ubuntu-advantage-tools. If yes, then we need to update debian/control. We are already pulling in the gnutls library because of apt-transport-https, so if we go ahead with this then we should make sure we are using the gnutls version of libcurl3
albertodonato
Nov 24, 2017
Contributor
They don't seem to be alternatives. apt-transport-https depends on libcurl3-gnutls, whereas curl depends on libcurl3.
FWIW I don't see a reason we can't depend on curl.
panlinux
Nov 27, 2017
•
Contributor
After a discussion in IRC (See https://irclogs.ubuntu.com/2017/11/27/%23ubuntu-devel.html#t11:34), we decided to abandon curl as it indeed pulls in too much for ubuntu-minimal. It was suggested that we try these:
- from trusty onwards:
/usr/lib/apt/apt-helper download-file <url> <path>(not available in precise) - or talk to the https method directly, like shown in http://paste.ubuntu.com/26057904/
$ /usr/lib/apt/methods/https
100 Capabilities
Version: 1.2
Pipeline: true
Send-Config: true
600 URI Acquire
URI: https://jak-linux.org
FileName: /tmp/test
panlinux
Nov 27, 2017
Contributor
I'm fine with skipping this auth-check feature for precise. Talking to the https method over stdin/stdout/stderr via shell seems icky.
| + error_msg 'Invalid credentials' | ||
| + ;; | ||
| + *) | ||
| + error_msg 'Failed checking credentials' |
| + local url | ||
| + url=$(private_repo_url "$2" "$3") |
panlinux
Nov 24, 2017
Contributor
What is $2 and $3 now? I kind of like the idea of local meaningful variables, because then we probably don't need docstrings ;)
| @@ -49,6 +49,22 @@ def test_enable_esm_install_apt_transport_https_apt_get_options(self): | ||
| self.assertIn( | ||
| 'DEBIAN_FRONTEND=noninteractive', self.read_file('apt_get.env')) | ||
| + def test_enable_esm_invalid_credentials(self): | ||
| + """If credentials are invalid, an errors is returned.""" |
| + | ||
| + def test_enable_esm_error_checking_credentials(self): | ||
| + """If credentials check fails, an error is returned.""" | ||
| + self.make_fake_binary('curl', command='exit 1') |
panlinux
Nov 24, 2017
Contributor
curl won't exit with non-zero if it encounters a 404, for example. You would get the same result in this test if it exited 0. We only check what it prints, right?
albertodonato
Nov 24, 2017
Contributor
Correct, I've updated the test (so it also tests the printed HTTP code)
| @@ -131,6 +131,22 @@ def test_enable_fips_invalid_token(self): | ||
| 'Invalid token, it must be in the form "user:password"', | ||
| process.stderr) | ||
| + def test_enable_fips_invalid_credentials(self): | ||
| + """If credentials are invalid, an errors is returned.""" |
| + if echo "$error_line" | grep -q '^\(HttpError\)\?401'; then | ||
| + error_msg 'Invalid credentials' | ||
| + else | ||
| + error_msg "Failed checking credentials ($error_line)" |
panlinux
Dec 11, 2017
Contributor
We should use the term "token" instead of credentials, that's how we refer to the pair foo:bar in all other communications with the user. This could also be changed in the code (functions, variables) or else it might get confusing.
| + # skip the check if the helper is not found | ||
| + [ -x "$APT_HELPER" ] || return 0 | ||
| + | ||
| + echo -n 'Checking credentials... ' |
panlinux
Dec 11, 2017
Contributor
What do you think about doing the APT_HELPER check after this echo, and output "SKIP" or "SKIPPED (no apt-helper found)", or some combination of these, if we don't find it?
| + tempfile="$(mktemp)" | ||
| + log="$(mktemp)" | ||
| + if "$APT_HELPER" download-file "$url" "$tempfile" >"$log" 2>&1; then | ||
| + rm -f "$tempfile" "$log" |
panlinux
Dec 11, 2017
Contributor
We only need this tempfile because apt-helper won't write to /dev/null, right? Perhaps add a quick comment here about that, and that tempfile will just be discarded? Maybe even rename it to "discardfile" or something like that :)
| + fi | ||
| + | ||
| + rm -f "$discard" "$log" | ||
| + return 1 |
panlinux
Dec 11, 2017
Contributor
Should we return 3 here perhaps? From the manpage:
3 Invalid or missing token when enabling a service
albertodonato commentedNov 24, 2017
Closes #85