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

Add support for private repos through Github oAuth #15

Merged
merged 7 commits into from Oct 12, 2012

Conversation

Projects
None yet
4 participants
@pdclark
Copy link
Contributor

pdclark commented Jun 28, 2012

Hi Joachim,

I saw your request for adding support for private repos, so I took a shot at it. This branch allows updates from private repositories using an oAuth token, and it provides setup assistance through Plugins > Github Updater.

Once an application is setup and and a token is given, the WPGitHubUpdaterSetup class isn't needed anymore. The token will authorize updates on all sites from then on, and will allow the plugin author to revoke privileges in the future if they wish.

While in there, I noticed that in get_github_data(), wp_remote_get was being passed $this->config['sslverify'] instead of array( 'sslverify' => $this->config['sslverify'] ) so I changed that as well.

Hope this helps!

Paul Clark

PS: While fishing around in the github APIs, I noticed that they allow querying a repos tags, and the response gives all versions with zipball URLS:

https://api.github.com/repos/jkudish/WordPress-GitHub-Plugin-Updater/tags

Looks like the config values for api_url, raw_url, github_url, and zip_url could all be replaced by that one URL, and the readme would no longer need to include a version number (because they're listed in the tags). Thoughts?

pdclark added some commits Jun 28, 2012

Fix delete_transients not running.
delete_transients was hooked to init priority 10, but upgrade.php is initialized on that hook, so the action is added too late to run. Changing delete transients to init priority 11 causes the method to run.
add_action('init', 'github_plugin_updater_test_init');
function github_plugin_updater_test_init() {
include_once('updater.php');
include_once 'updater.php';

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

why did you change this?

This comment has been minimized.

@pdclark

pdclark Sep 19, 2012

Contributor

Because include_once, like include, require, and echo, is a language construct, not a function. While the PHP parser tolerates this use, it is not correct. You can see examples of this syntax in use in the documentation on include: http://php.net/manual/en/function.include.php and an exploration of the differences between language constructs and functions in this Stack Overflow thread: http://stackoverflow.com/questions/1180184/what-is-the-difference-between-a-language-construct-and-a-built-in-function-in

From PHP documentation:
"Because include is a special language construct, parentheses are not needed around its argument."

From StackOverflow:
"A language builtin is faster to call than a function. This is true, if only marginally, because the PHP interpreter doesn't need to map that function to its language-builtin equivalents before parsing. On a modern machine, though, the difference is fairly negligible.

plugin.php Outdated
add_action( 'network_admin_menu', array( $this, 'add_page' ) );
add_action('wp_ajax_set_github_oauth_key', array($this, 'ajax_set_github_oauth_key') );

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

missing a space before 'wp_ajax...' and before $this

plugin.php Outdated
// Send user to Github for account authorization
header( "Location: https://github.com/login/oauth/authorize?scope=repo&client_id={$gh['client_id']}&redirect_uri=$redirect");

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

please use wp_redirect() instead of header()

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

also please use add_query_arg()

plugin.php Outdated
}
header( 'Location: '.admin_url('plugins.php?page=github-updater') );

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

please use wp_redirect()

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

also please use add_query_arg()

plugin.php Outdated
}else {
header( 'Location: '.admin_url('plugins.php?page=github-updater&authorize=false') );

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

please use wp_redirect()

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

also please use add_query_arg()

updater.php Outdated
@@ -81,6 +82,13 @@ public function __construct( $config = array() ) {
* @return void
*/
public function set_defaults() {
if ( !empty($this->config['access_token']) ) {
$this->config['access_token'] = 'access_token='.$this->config['access_token'];

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

minor spacing adjustments needed

'access_token=' . $this->config['access_token'];

updater.php Outdated
$this->config['access_token'] = 'access_token='.$this->config['access_token'];
// See Downloading a zipball (private repo) https://help.github.com/articles/downloading-files-from-the-command-line
$this->config['zip_url'] = str_replace('//github.com/', '//api.github.com/repos/', $this->config['zip_url']).'?'.$this->config['access_token'];

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

spacing adjustments needed

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

I don't like this use of str_replace, can we build the zip_url differently? also please use add_query_arg to build URL

updater.php Outdated
@@ -147,7 +156,7 @@ public function get_new_version() {
if ( !isset( $version ) || !$version || '' == $version ) {
$raw_response = wp_remote_get(
trailingslashit($this->config['raw_url']).$this->config['readme'],
trailingslashit($this->config['raw_url']).$this->config['readme'].'?'.$this->config['access_token'],

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

please use add_query_arg() + spacing adjustments needed

updater.php Outdated
@@ -183,8 +192,10 @@ public function get_github_data() {
if ( ! isset( $github_data ) || ! $github_data || '' == $github_data ) {
$github_data = wp_remote_get(
$this->config['api_url']
,$this->config['sslverify']
$this->config['api_url'].'?'.$this->config['access_token'],

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

please use add_query_arg() + spacing adjustments needed

updater.php Outdated
@@ -258,7 +269,7 @@ public function api_check( $transient ) {
$response = new stdClass;
$response->new_version = $this->config['new_version'];
$response->slug = $this->config['proper_folder_name'];
$response->url = $this->config['github_url'];
$response->url = $this->config['github_url'].'?'.$this->config['access_token'];

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

could this:

$this->config['github_url'].'?'.$this->config['access_token'];

be consolidated into a single function (e.g. $this->get_url() or something like that, that would do the work to detect if an access_token was present and build the URL accordingly rather than doing it in many different places in the code...

This comment has been minimized.

@pdclark

pdclark Jul 14, 2012

Contributor

Absolutely! I've been playing with this approach a bit in my f/gitweb branch (no pull request submitted). It's still in rough form, but I distribute different services to different classes and then allow the classes to parse the URL accordingly.

I parsing all arguments from the URL works really well –– it detects an access token in the URL for Github, or normal HTTPS user:password authentication for Gitweb and other services.

This comment has been minimized.

@pdclark

pdclark Sep 19, 2012

Contributor

The new push shows that this line was changed. It was for the sake of using add_query_arg(). There is a generalized code for building URLs in the zero-config branch, but it sounds like you don't want to move to configuring through the header and supporting multiple Git hosts all at once. It will take me some time to rework those features into smaller portions.

@@ -61,7 +62,7 @@ public function __construct( $config = array() ) {
$this->set_defaults();
if ( ( defined('WP_DEBUG') && WP_DEBUG ) || ( defined('WP_GITHUB_FORCE_UPDATE') || WP_GITHUB_FORCE_UPDATE ) )
add_action( 'init', array( $this, 'delete_transients' ) );
add_action( 'init', array( $this, 'delete_transients' ), 11 );

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

why does the priority need to be higher than 10?

This comment has been minimized.

@pdclark

pdclark Jul 14, 2012

Contributor

It may have just been me, but logging the function made it look like it wasn't firing at 10. The problem seemed to be that the plugin was running __construct on init priority 10. (In the current master plugin.php: add_action('init', 'github_plugin_updater_test_init');)

So, plugin initializes at init:10, then tries to hook delete_transients onto init:10, but that priority is already running. Upping to 11 resolved the problem for me.

Has anyone else had this issue?

PS: This line as-is probably isn't running anymore again, because in this version the plugin is initialized on admin_init to save processing and remove the requirement for is_admin(). init runs before admin_init, so if the priority issue exists, it should be admin_init: 11 anyway.

This comment has been minimized.

@jkudish

jkudish Jul 14, 2012

Contributor

hmm, i never noticed this issue, but that might be quite possible. leave it at 11 and I'll play with it when I merge this in

@jkudish

This comment has been minimized.

Copy link
Contributor

jkudish commented Jul 14, 2012

Sorry for the delay in getting back to you. This does look really good and thanks for building this feature out. I left a few minor comments about coding nitpickiness on my part, would be great if you could address those and I'll gladly merge this in.

Thanks!

@pdclark

This comment has been minimized.

Copy link
Contributor

pdclark commented Jul 14, 2012

No problem, and will do! Thanks for all the input!

@jkudish

This comment has been minimized.

Copy link
Contributor

jkudish commented Sep 13, 2012

Hey @pdclark any update on this front?

@pdclark

This comment has been minimized.

Copy link
Contributor

pdclark commented Sep 13, 2012

I'm head-down prepping to present at WCLA the day after tomorrow, but should be able to finalize this immediately afterwards. I have moved it back to the top of my task list -- it's been far too long for me to finalize the small changed you laid out.

Thanks for the reminder, and for your patience!

@nunomorgadinho

This comment has been minimized.

Copy link

nunomorgadinho commented Oct 10, 2012

Hi guys, any news on this front? This would be so awesome to see :-)

@pdclark

This comment has been minimized.

Copy link
Contributor

pdclark commented Oct 10, 2012

The branch has been functionally complete since 4 months ago, and the formatting changes Joey requested were completed 22 months ago. You can use the branch on my repo if you'd like.

When discussing another pull request two weeks ago, jkudish commented that he'd review this one, but wouldn't get to them within a week.

It had been my hope to develop features for jkudish's version and keep his as the main version. But, at the moment, I'm unsure on how to proceed and unsure if my submitting work is @jkudish's preference.

@nunomorgadinho

This comment has been minimized.

Copy link

nunomorgadinho commented Oct 10, 2012

You meant 22 days right? @jkudish: I would love to see this merged in, anything preventing it?

@jkudish

This comment has been minimized.

Copy link
Contributor

jkudish commented Oct 10, 2012

@pdclark: "This pull request cannot be automatically merged."

Can you fix it up so that it merges cleanly and I will :)

@pdclark

This comment has been minimized.

Copy link
Contributor

pdclark commented Oct 11, 2012

Okay, this is done. It's been merged with your master. I tested merging f/oauth in, then also tested setting up and receiving updates from a private repo a few times on WP 3.5 beta 1. Should be good to go.

updater.php Outdated
@@ -162,14 +174,12 @@ public function delete_transients() {
public function get_new_version() {
$version = get_site_transient( $this->config['slug'].'_new_version' );
if ( !isset( $version ) || !$version || '' == $version ) {
if ( true || !isset( $version ) || !$version || '' == $version ) {

This comment has been minimized.

@jkudish

jkudish Oct 11, 2012

Contributor

if ( true ... doesn't look right

seems like this whole statement could be simplified with:

if ( empty( $version ) )

This comment has been minimized.

@jkudish

jkudish Oct 11, 2012

Contributor

adjusted my comment a bit in case you were quick

This comment has been minimized.

@pdclark

pdclark Oct 11, 2012

Contributor

Good catch! That can't be right.

I've removed the "true". The rest was there in your version, so I left it. I'd agree that empty() would make sense, the only reason I can think of that it might not be that already is if someone was working around $version being 0 (zero), which empty returns true for? Even then though, I can't imagine why a new version would ever be set to "0".

@jkudish

This comment has been minimized.

Copy link
Contributor

jkudish commented Oct 11, 2012

Looks pretty good at first glance. See my one comment on the if statement left on that line.

Otherwise, I'll do my best to test this and merge it in tomorrow. @nunomorgadinho if you're able to, please help test this merge request by doing a clone from @pdclark's fork :)

@pdclark

This comment has been minimized.

Copy link
Contributor

pdclark commented Oct 11, 2012

Good catch, that's revised. @nunomorgadinho, if you test, it'd the f/oauth branch, not master.

jkudish added a commit that referenced this pull request Oct 12, 2012

Merge pull request #15 from pdclark/f/oauth
Add support for private repos through Github oAuth, big props to @pdclark

@jkudish jkudish merged commit a64a63d into radishconcepts:master Oct 12, 2012

@jkudish

This comment has been minimized.

Copy link
Contributor

jkudish commented Oct 12, 2012

I've merged it in, at last. I plan to do some minor code tweaks to it over the next 2 weeks or so. Stay tuned

@pdclark

This comment has been minimized.

Copy link
Contributor

pdclark commented Oct 12, 2012

Woot!! Thank you!

@nunomorgadinho

This comment has been minimized.

Copy link

nunomorgadinho commented Oct 12, 2012

Nice! Thanks!

@davekiss

This comment has been minimized.

Copy link

davekiss commented Feb 27, 2013

Doesn't this make your account extremely vulnerable? You can't do this and distribute your access token with a public plugin.

@pdclark

This comment has been minimized.

Copy link
Contributor

pdclark commented Feb 27, 2013

@davekiss Using GitHub Organizations, you can set an account to have read-only access to private repos. See Next generation permissions control > Pull-only in the linked article.

Otherwise, yes, the GitHub oAuth API does set read/write access, and it would not be prudent to use it for a publicly-released private plugin without setting up permissions in your Github organization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment