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

bitbucket requires to have auth at this level #213

Closed
wants to merge 2 commits into from
Closed

bitbucket requires to have auth at this level #213

wants to merge 2 commits into from

Conversation

lukepolo
Copy link

No description provided.

@taylorotwell
Copy link
Member

Can anyone else independently confirm this? @themsaid?

only need client id no secret
@themsaid
Copy link
Member

themsaid commented Mar 16, 2017

I can confirm that it works without the changes in this PR.

@lukepolo
Copy link
Author

Then what am i doing wrong, it says I'm missing the client credentials, only works when adding that line of code there ~

@lukepolo
Copy link
Author

lukepolo commented Mar 16, 2017

Error :
Client error: POST https://bitbucket.org/site/oauth2/access_token` resulted in a 400 Bad Request response: {"error_description": "Client credentials missing; this request needs to be authenticated with the OAuth client id and s (truncated...)`

Error Location :
image

Socialite Version :

"name": "laravel/socialite",
        "version": "v3.0.3",

Bit bucket Permissions :

image

@taylorotwell
Copy link
Member

Closing for now since we can't recreate this issue. If you can recreate it on a fresh, simple Laravel application and link @themsaid to it maybe he could recreate it then?

@lukepolo
Copy link
Author

Yup will try it.

@lukepolo
Copy link
Author

lukepolo commented Mar 17, 2017

https://github.com/lukepolo/BitBucketTest

There is the repository , same issue

image

@themsaid Figured id add you as a notification , its a fresh install of laravel with socialite and a simple oauth controller

@rdgout
Copy link

rdgout commented Mar 23, 2017

I have the same issue as above with a fresh Laravel installation

@lukepolo
Copy link
Author

@rdgout we may have to open a new issue not sure if this will get noticed~

@rdgout
Copy link

rdgout commented Apr 4, 2017

@themsaid could you verify these findings? It's a little tedious to modify the source of the package when I deploy my application.

@lukepolo
Copy link
Author

@rdgout @themsaid were you able to solve this without my patch?

@rdgout
Copy link

rdgout commented Apr 10, 2017

@lukepolo Nope, I had to manually change the code in the package when I deployed. Which means that I'm screwed when it's updated without the fix. Maybe we should ping @taylorotwell so he can take another look if @themsaid is busy.

@themsaid
Copy link
Member

Hey, I'll look into the repository you created now and try to replicate.

@themsaid
Copy link
Member

I still confirm that it's working, I hit http://zzzz.ngrok.io/oauth/callback/bitbucket and then grant access on BitBucket, then get redirected back to http://zzz.ngrok.io/oauth/callback/bitbucket?state=TT6s2S3S9fFoIJTlv0M1E5OS0p24AlsCxX0QaCd2&code=xFYVQemDez5WA8RU7D with the user data needed.

@lukepolo
Copy link
Author

did you give the scopes as following

image

@themsaid
Copy link
Member

yes

@themsaid
Copy link
Member

I guess you may need to contact BitBucket and report this behaviour.

@lukepolo
Copy link
Author

Can i send you my client id and secret via another method , see if that works ? Im just trying to figure out why it wont work for me while it works for you perfectly

@lukepolo
Copy link
Author

Ok will report back here after i contact them ~

@themsaid
Copy link
Member

@lukepolo ok please ping me when you do, or say hello on twitter @themsaid :)

@dtao
Copy link

dtao commented Apr 11, 2017

I think @lukepolo is right that this is broken, though it probably worked in previous versions. I am not very familiar with this library (I work on Bitbucket) but I can see that this commit renamed the method getAccessToken to getAccessTokenResponse in the AbstractProvider class, whereas in the BitbucketProvider class the method (which provides authentication headers) is still called getAccessToken.

In light of that info, I would speculate that the immediate fix is probably to update the method name in BitbucketProvider rather than providing auth headers in the abstract class, as this PR does.

@themsaid
Copy link
Member

If apply the changes described by @dtao I get:

Client error: `GET https://api.bitbucket.org/2.0/user?access_token=` resulted in a `401 Unauthorized` response:
{"type": "error", "error": {"message": "Access token expired. Use your refresh token to obtain a new access token."}}

Without passing auth the code just works, Also merging @lukepolo PR locally causes guzzle to break since you're passing the username only not the password for the auth.

@dtao
Copy link

dtao commented Apr 11, 2017

@themsaid Do you know what version of the code you're using? I am a little confused because there appear to be 3 branches: master, 2.0, and 3.0; and the problem I described only exists in the 3.0 branch (where, incidentally, you can see that getAccessToken literally only appears in one place, BitbucketProvider.php).

@themsaid
Copy link
Member

@dtao I'm on 3.0, I think the getAccessToken() can be removed since the getAccessTokenResponse() is being used and it works. Can you please share that part in the docs where it says auth headers should be passed?

@dtao
Copy link

dtao commented Apr 11, 2017

@themsaid The portion of the RFC I referred @lukepolo to was section 3.2.1:

Confidential clients or other clients issued client credentials MUST
authenticate with the authorization server as described in
Section 2.3 when making requests to the token endpoint.

That said, on closer inspection I see that the default implementation in the AbstractProvider class does pass client_id and client_secret here, which should work as an authentication mechanism. (Basically Bitbucket will return a 400 if you haven't provided either an authorization header or these values.)

@lukepolo Is it possible these values are not set properly for you? Once again, I'm not very familiar with this library so I don't know if that question makes sense; but if that's the case then that would explain why it is working for other contributors to this thread but not for you.

@lukepolo
Copy link
Author

Ok so with 3.0.3 it does not work , but with 3.0.4 it does just tested this. My repo was using 3.0.3 so not sure why it didnt trigger for you earlier ~

@lukepolo
Copy link
Author

82882dc

Is the change that made it work

@rdgout
Copy link

rdgout commented Apr 12, 2017

I can confirm it works out-of-the-box now rather than the problem we were encountering before.

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.

5 participants