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

Check 'auth' field when displaying .dockercfg config #2392

Merged
merged 1 commit into from
Oct 31, 2017

Conversation

jhadvig
Copy link
Member

@jhadvig jhadvig commented Oct 27, 2017

While working on https://bugzilla.redhat.com/show_bug.cgi?id=1506998 I've noticed that we are not checking for auth field in the old .dockercfg format, which looks like:

{
        "registryaddress": {
                "auth": "auth string",
                "email": "[email protected]"
        }
}

The registryaddress could also contain username and password fields.

@spadgett PTAL

Will add the dist after review

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 27, 2017
@@ -28,17 +28,29 @@ angular.module("openshiftConsole")
return secretsByType;
};

var setDataParams = function(decodedSecretData, serverName, data) {
Copy link
Member

Choose a reason for hiding this comment

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

If this is specific to dockercfg, we should have that somewhere in the function name.

Copy link
Member Author

Choose a reason for hiding this comment

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

This method is not intended to be specific for dockercfg or config.json. It's purpose is to assemble an object that will contain server address, username, password.

username: usernamePassword[0],
password: usernamePassword[1],
email: data.email
};
Copy link
Member

Choose a reason for hiding this comment

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

I would let the caller set the value in the auths object and just let this function return the data. Then the function takes fewer arguments and doesn't need to know anything about the structure of the decodedSecretData object passed to it.

(untested)

var getDockercfgServerParams = function(serverData) {
  var params = _.pick(serverData, ['email', 'username', 'password']);
  if (serverData.auth) {
    try {
      // Decode Base64-encoded username:password string.
      _.spread(window.atob(data.auth).split(":"), function(username, password) {
        params.username = username;
        params.password = password;
      });
    } catch(e) {}
  }

  return params;
};

then below in the decodeDockercfg function

decodedSecretData.auths[serverName] = getDockercfgServerParams(data);

https://lodash.com/docs/4.17.4#pick
https://lodash.com/docs/4.17.4#spread

var setDataParams = function(decodedSecretData, serverName, data) {
var usernamePassword = [];
if (data.auth) {
usernamePassword = window.atob(data.auth).split(":");
Copy link
Member

Choose a reason for hiding this comment

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

We should catch exceptions to avoid a runtime error if it's not properly encoded.

https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/atob

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2017
@jhadvig
Copy link
Member Author

jhadvig commented Oct 30, 2017

@spadgett I've updated the PR based on your comments.
I know that you suggested to rename the method for getting the server params to getDockercfgServerParams but that method should be Docker config format agnostic (should cover both .dockercfg and docker/config.json) so I've named it getServerParams.
Also I've refactored both decodeDockercfg and decodeDockerconfigjson method into single decodeDockerConfig method since both of them have been doing almost the same operations, there have been just slight differences which should be covered with a simple if.
Also added decoding error handling, here is the example with error notification:
auth_err

Tested the change with single and multi server .dockercfg and docker/config.json and works as expected.

PTAL

@spadgett
Copy link
Member

/kind bug

@jhadvig You have a dist mismatch

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Oct 31, 2017
@spadgett
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2017
@spadgett
Copy link
Member

Hm.

pm ERR! [email protected] install: `node install.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] install script 'node install.js'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the phantomjs-prebuilt package,
npm ERR! not with npm itself.

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit e865849 into openshift:master Oct 31, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants