Skip to content
This repository has been archived by the owner on Mar 21, 2022. It is now read-only.

RegistryAuth: handle case where password contains colon #765

Merged
merged 1 commit into from
May 25, 2017

Conversation

mattnworb
Copy link
Member

Fixes #764. This now matches how docker-cli parses this field from
~/.docker/config.json.

@mattnworb mattnworb requested a review from davidxia May 25, 2017 14:32
@mavenraven
Copy link

@mattnworb what if the password contains 2 colons?

@mattnworb
Copy link
Member Author

@mavenraven it will be taken care of by String.split(String, int):

The limit parameter controls the number of times the pattern is applied and therefore affects the length of the resulting array. If the limit n is greater than zero then the pattern will be applied at most n - 1 times, the array's length will be no greater than n, and the array's last entry will contain all input beyond the last matched delimiter. If n is non-positive then the pattern will be applied as many times as possible and the array can have any length. If n is zero then the pattern will be applied as many times as possible, the array can have any length, and trailing empty strings will be discarded.

@mattnworb
Copy link
Member Author

btw seems like this bug existed before #759:

final String[] authParams = Base64.decodeAsString(authString).split(":");
if (authParams.length == 2) {
authBuilder.username(authParams[0].trim());
authBuilder.password(authParams[1].trim());
} else if (serverAuth.has("identityToken")) {

@mattnworb mattnworb force-pushed the mattbrown/fix-registryauth-forauth branch 2 times, most recently from a9c3919 to 438cc8f Compare May 25, 2017 15:58
@codecov-io
Copy link

codecov-io commented May 25, 2017

Codecov Report

Merging #765 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #765      +/-   ##
============================================
- Coverage      65.3%   65.23%   -0.07%     
+ Complexity      676      674       -2     
============================================
  Files           156      156              
  Lines          2957     2957              
  Branches        340      340              
============================================
- Hits           1931     1929       -2     
- Misses          877      878       +1     
- Partials        149      150       +1

@mattnworb mattnworb force-pushed the mattbrown/fix-registryauth-forauth branch from 438cc8f to 92705d9 Compare May 25, 2017 16:36
Fixes #764. This now matches [how docker-cli parses this field][0] from
~/.docker/config.json.

Updated a test in DockerConfigReaderTest that was implicitly expecting
the auth value to be ignored and thus failing the test with this fix.

[0]: https://github.com/docker/cli/blob/db5620026dda9b8f5da519e382cf40ed22775e6d/cli/config/configfile/file.go#L184-L189
@mattnworb mattnworb force-pushed the mattbrown/fix-registryauth-forauth branch from 92705d9 to 8b14122 Compare May 25, 2017 18:03
@mattnworb mattnworb merged commit f00168e into master May 25, 2017
@mattnworb mattnworb deleted the mattbrown/fix-registryauth-forauth branch May 25, 2017 18:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants