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

Allow credentials to be retrieved from the environment #74

Merged
merged 1 commit into from
Mar 19, 2019

Conversation

pento
Copy link
Member

@pento pento commented Mar 17, 2019

So that the upload_patch task can be run programatically, it would be helpful if it could retrieve WordPress.org credentials from the environment.

To Test

In WordPress' package.json, set grunt-patch-wordpress to WordPress/grunt-patch-wordpress#add/credentials-from-environment, then run npm install.

Run:

export WPORG_USERNAME=matt
export WPORG_PASSWORD=MyPasswordIsVerySecure12345
grunt uploadPatch:40000

@aaronjorbin
Copy link
Member

Related: #40, #37

https://www.npmjs.com/package/keytar seems like it might be a bit more secure than a plain text ENV variable.

If we do go this route though, I think it might make sense to not add the useEnv flag but instead to look for if the variables are set, and if they are set try to use them. Especially as we are trying to remove grunt as a dependency.

@kadamwhite
Copy link
Contributor

I'm fine with either the approach here or the more secure option proposed by @aaronjorbin but, with apologies, the landing of a few other PR's have introduced merge conflicts either way. Sorry for dropping that on you @pento

@pento
Copy link
Member Author

pento commented Mar 19, 2019

Huh, keytar is cute. I'm launching grunt upload_patch from a Node spawn() call though, so providing credentials via the ENV is simple, I don't know how well it'll play with system keychains.

I added the useEnv option to ensure it couldn't be used accidentally, but it's probably a reasonable assumption that the ENV variables will only be set if someone actually wants to use this behaviour.

@pento pento force-pushed the add/credentials-from-environment branch from 344dfa8 to 0e77288 Compare March 19, 2019 07:12
@ntwb
Copy link
Member

ntwb commented Mar 19, 2019

No objections on either method, I'm happy to go as-is for now and look to adding keytar in a future iteration 👍

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

👍

@aaronjorbin aaronjorbin merged commit 0ef6459 into master Mar 19, 2019
@ntwb ntwb deleted the add/credentials-from-environment branch March 19, 2019 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants