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

OAuth Support for Bitbucket #5055

Merged
merged 6 commits into from Apr 1, 2016
Merged

OAuth Support for Bitbucket #5055

merged 6 commits into from Apr 1, 2016

Conversation

ghost
Copy link

@ghost ghost commented Mar 14, 2016

I used existing github-oauth code as a template to develop bitbucket-oauth functionality. Users can add consumer-key and consumer-secret to auth.json. Instructions on how to obtain the consumer keys have been added to the documentation.


// if available use token from git config
if (0 === $this->process->execute('git config bitbucket.accesstoken', $output)) {
$this->io->setAuthentication($originUrl, trim($output), 'x-oauth-basic');
Copy link
Member

Choose a reason for hiding this comment

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

This seems like it won't work for bitbucket?

Copy link
Author

Choose a reason for hiding this comment

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

I'll double check this.

@Seldaek
Copy link
Member

Seldaek commented Mar 14, 2016

Is the bitbucket-domains config really needed? It seems to me like this stuff only works for "bitbucket cloud" which is bitbucket.org. I don't have a bitbucket server to check though so no idea.

@ghost
Copy link
Author

ghost commented Mar 14, 2016

I've never used Atlassian's Stash (now called Bitbucket Server), so I'm not sure if they allow different domains. I can remove the config for now, and add it back later if it's requested.

@@ -27,6 +27,7 @@ class Config
'preferred-install' => 'auto',
'notify-on-install' => true,
'github-protocols' => array('https', 'ssh', 'git'),
'bitbucket-protocols' => array('https'),
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need it ? Are they allowing to clone with another protocol ?

@curry684
Copy link
Contributor

We're running a pretty out of date Stash 3.3 here (still need to plan an update to Bitbucket Server hehe) but I can't find OAuth anywhere in the configs. Don't think it's much use either behind the firewall...? All I can find on Google with a quick search refers to Bitbucket Cloud as well.

Let me know if there's anything I can test with our Stash instance though.

try {
$apiUrl = 'bitbucket.org/site/oauth2/access_token';

$json = $this->remoteFilesystem->getContents($originUrl, 'https://'.$apiUrl, false, array(
Copy link
Contributor

Choose a reason for hiding this comment

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

https:// should be in the apiUrl variable directly IMO here

@stof
Copy link
Contributor

stof commented Mar 15, 2016

Is the bitbucket-domains config really needed? It seems to me like this stuff only works for "bitbucket cloud" which is bitbucket.org. I don't have a bitbucket server to check though so no idea.

The OAuth logic always performs the OAuth calls on the bitbucket.org API in this API, so it does not support any other domain

…tication when read from git config. Grant type is now only set when requesting an access token. Removed bitbucket-domains and bitbucket-protocols from config. Fixed bitbucket typo in JsonConfigSource. Removed unecessary comments. Changed visibility of Composer/Util/Bitbucket properties to private. Added https to bitbucket url. Removed unused $note variable.
@ghost
Copy link
Author

ghost commented Mar 21, 2016

I completed the updates per the conversations above.

// handle bitbucket-oauth
if (preg_match('/^(bitbucket-oauth)\.(.+)/', $settingKey, $matches)) {
if (2 !== count($values)) {
throw new \RuntimeException('Excepted two arguments (consumer-key, consumer-secret), got '.count($values));
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo in the exception message. Should be Expected, not Excepted.

@Seldaek Seldaek merged commit 59ae271 into composer:master Apr 1, 2016
@Seldaek
Copy link
Member

Seldaek commented Apr 1, 2016

alrighty, thanks @wenkepaul

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants