davido / gerrit-oauth-provider

OAuth2 authentication provider for Gerrit Code Review. Please upload changes for review to: https://gerrit-review.googlesource.com/#/admin/projects/plugins/oauth
Apache License 2.0
140 stars 84 forks source link

bitbucket oauth broken #127

Open mrdfuse opened 5 years ago

mrdfuse commented 5 years ago

With plugin 2.14.6.2, it seems bitbucket oauth is broken since today?

[2019-05-29 07:58:41,400] [HTTP-693] ERROR com.google.gerrit.pgm.http.jetty.HiddenErrorHandler : Error in GET /oauth?state=xxxxxxxxxxxxxxxxxxxxxxxxx&code=xxxxxxxxxxxxxxxx
java.lang.NullPointerException
        at com.googlesource.gerrit.plugins.oauth.BitbucketOAuthService.getUserInfo(BitbucketOAuthService.java:95)
        at com.google.gerrit.httpd.auth.oauth.OAuthSession.login(OAuthSession.java:105)
        at com.google.gerrit.httpd.auth.oauth.OAuthWebFilter.doFilter(OAuthWebFilter.java:105)
davido commented 5 years ago

Strange. What is different now?

mrdfuse commented 5 years ago

I have no idea, I thought I logged this issue quicky while scrambling away to get our developers back on track. We switched to DEVELOPMENT_BECOME_ANY_ACCOUNT now, just to be able to work again. I have time now to further debug what might be wrong. What can I do? Can I increase the logging level to see more? We are using the openfrontier docker image, which does make it a bit harder to quickly change config here and there.

davido commented 5 years ago

May be Bitbucket changed something on their side, can you ask them? Yes, it should be possible to increase logging level in gerrit.

mrdfuse commented 5 years ago

Yes that is also my guess. We are trying to debug it, will keep you posted.

azhou2019 commented 5 years ago

We have the same issue with BitBucket OAuth starting May 29th. It is odd as only some BitBucket users would experience this server error and other users are fine.

davido commented 5 years ago

Looking at the code:

    JsonElement usernameElement = userObject.get("username");
    String username = usernameElement.getAsString(); // <== NPE is here, so that username JSON element in the result seems to be null.

we have a problem to extract username from the https://bitbucket.org/api/1.0/user endpoint. I think that someone would have to open issue on BitBucket side.

davido commented 5 years ago

Nothing unusual on the API page: [1]. However, we are still using api version 1.0, while there is api version 2. available. Can someone try to patch the plugin with this diff ans see if this helps:

Date:   Thu May 30 10:15:08 2019 +0200

    Bump BitBucket api version to 2.0

    Change-Id: Ie7e42cf9e4e4e0eb3300d2f0f034d7ca04b8ec0f

diff --git a/src/main/java/com/googlesource/gerrit/plugins/oauth/BitbucketOAuthService.java b/src/main/java/com/googlesource/gerrit/plugins/oauth/BitbucketOAuthService.java
index e600067..ada287c 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/oauth/BitbucketOAuthService.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/oauth/BitbucketOAuthService.java
@@ -47,7 +47,7 @@ public class BitbucketOAuthService implements OAuthServiceProvider {
   private static final Logger log = getLogger(BitbucketOAuthService.class);
   static final String CONFIG_SUFFIX = "-bitbucket-oauth";
   private static final String BITBUCKET_PROVIDER_PREFIX = "bitbucket-oauth:";
-  private static final String PROTECTED_RESOURCE_URL = "https://bitbucket.org/api/1.0/user/";
+  private static final String PROTECTED_RESOURCE_URL = "https://bitbucket.org/api/2.0/user/";
   private final boolean fixLegacyUserId;
   private final OAuthService service;
azhou2019 commented 5 years ago

@davido @mrdfuse I have received the following response from Bitbucket Support Team:

Looks like the plugin : https://github.com/davido/gerrit-oauth-provider/blob/master/src/main/java/com/googlesource/gerrit/plugins/oauth/BitbucketOAuthService.java#L95 hasn't been updated per our deprecation/removal documentation that has been announced over 6 months ago. For reference: https://developer.atlassian.com/cloud/bitbucket/bitbucket-api-changes-gdpr/ The owner of the plugin will need to update this string: String username = usernameElement.getAsString(); to use the UUID instead of a username for the plugin to work with the changes that we have introduced to improve account privacy.

mrdfuse commented 5 years ago

Thanks for the feedback. If no else has done it by next week, I'll try to patch it. Would need to setup a dev environment first though.

davido commented 5 years ago

@azhou2019 Thanks so much, for letting us know.

I am working on a fix and will try to release it this weekend (I'm doing all this here in my spare free time).

davido commented 5 years ago

OK, I'm looking deeper now into the spec and deprecation announcement, particularly:

Replace username fields with Atlassian Account ID (account_id)

The Bitbucket's user object schema will lose its username field after 29 April 2019. This will affect clients that use username to identify accounts in API request objects.

and this example, old:

$ curl https://api.bitbucket.org/2.0/repositories/foo/bar/forks \ -H 'Content-Type: application/json' \ -d '{
    "name": "my_fork",
    "owner": {
      "username": "evzijst"
    }
}'

new:

$ curl https://api.bitbucket.org/2.0/repositories/foo/bar/forks \ -H 'Content-Type: application/json' \ -d '{
    "name": "my_fork",
    "owner": {
      "account_id": "557058:c0b72ad0-1cb5-4018-9cdc-0cde8492c443"
    }
}'

or alternatively, use uuid, example: uuid | {33e297d1-567f-4c32-89b4-4b2759c5f5ae}.

But this switch would mean, that we would create a virgin new account (!) with new account id. This would mean that site admin would have to merge those accounts. This would mean siome SQL statements or All-Users repository surgery, depending on whether ReviewDb or NoteDb is used.

We would also not be able to automagically link the deprecated usernae to the new uuid value, because username is not returned any more.

To be honest this is pretty much disruptive change! Bitbucket Site can argue, that username was deprecated and we should have enough time to react on this announcement and link meantime uuid to deprecated username in Gerrit Code Review backend.

However, I am maintaining a number of different OAuth 2 providers in this plugin in my spare free time, and am not going to follow any API announcements of those providers.

I would expect Bitbucket Site to re-consider this disruptive removal of username attribute and extend the support for this attribute (may be also protected by addition request parameter, like enforce_deprecated_username_attribute) for another one or two years.

That way, we could automagically link new uuid to the same username Bitbucket account, without site admn intervention (accounts merge procedure).

mrdfuse commented 5 years ago

I doubt Bitbucket is going to revert their change just for this project :(

davido commented 5 years ago

I am re-reading the spec change announcement in: [1] again and see this:

The exception to the removal of usernames is the /2.0/user endpoint,
which will continue to contain the username field. This endpoint only
ever returns the authenticated user's own user object, not that of other users.

So that my diff in this comment should probably fix the problem. Can someone try? I do not have access to Atlassian test site.

[1] https://developer.atlassian.com/cloud/bitbucket/bitbucket-api-changes-gdpr

davido commented 5 years ago

I have just sent this CL: [1]. Can someone test it?

If you cannot build it yourself, I can build it for you. Just tell me, which Gerrit version you are using. The above CL was uploaded to stable-2.14 branch.

mrdfuse commented 5 years ago

I can test it on 2.14, but I can't build it myself.

mionch commented 5 years ago

It looks like the v1 API has some further issues - over the last couple hours #I would get an error on any call, pointing out that this API is no longer available and linking to https://developer.atlassian.com/cloud/bitbucket/deprecation-notice-v1-apis/ for an explanation. I have tried changing the API url to 2.0 as @davido suggested, but it looks like the structure of the response has also slightly changed.

I have created a pull request with the changes that I have managed to get to work in our case: #128 Hope this helps!

davido commented 5 years ago

Thanks, @mionch! I started to work on the issue already and uploaded this CL to canonical repository for this plugin: [1]. However, I am not able to test this change and thus I have not merged it yet.

Can you please upload your change to https://gerrit-review.googlesource.com, to stable-2.14 branch? I would merge that change then to this plugin as well.

[1] https://gerrit-review.googlesource.com/c/plugins/oauth/+/227253