bitbucket: remove 2FA test #743

mminns · 2022-06-14T15:34:28Z

BitbucketHostProvider: fix runtime exceptions when authentication requests for Bitbucket DC would incorrectly call a Bitbucket Cloud REST API

A bug was introduced in commit: 5a2cfd7. Prior to this only authentication requests for Bitbucket Cloud would try and automatically determine if 2FA was required by the current user by calling a Bitbucket Cloud REST API using user provided Basic Auth credentials

Commit: 5a2cfd7 removed checking of the current host was Bitbucket Cloud vs DC. This meant the check would be run for Bitbucket Cloud and DC regardless. It would fail for Bitbucket DC

The fix is more radical than simply re-instating the check on the type of Bitbucket host.From 1st March 2022 support for using a Bitbucket Cloud user's account password to access REST or Git HTTPS operations has been removed, https://atlassian.community/t5/x/x/ba-p/1948231. As such this automatic test to see if 2FA is required no longer works.

Therefore the check against the Bitbcuket Cloud REST API has been removed in its entirety

ldennington

Thanks for jumping on this so quickly Mike! I've just got a couple questions.

ldennington · 2022-06-14T20:06:23Z

src/shared/Atlassian.Bitbucket/BitbucketHostProvider.cs

@@ -156,28 +156,16 @@ private async Task<ICredential> GetRefreshedCredentials(Uri remoteUri, string us

                    switch (result.AuthenticationMode)
                    {
+                        case AuthenticationModes.Basic:


Probably a naive question, but what will happen here if the user still requires 2FA?

So what used to happen was:

The user would first be shown a Basic Auth dialog in the helper.
They would enter username/password which are returned from the dialog.

If they were using a URL that isn't bitbucket.org the username/password is not tested, simply passed back to the GCM app and then back to Git.

If they were using bitbucket.org host, then the username/password is tested by calling the REST API.
If it returns a 200 then the username/password is passed back to the GCM app and then back to Git.
If it returns a 403 it would show the OAuth dialog in the helper.
Anything else would throw an exception.

Removing this code, means the behaviour for Bitbucket.org is the same as for other URLs.

Users will now need to manually choose the OAuth option on the Basic Auth dialog, or force OAuth via credential.bitbucketAuthModes.

I was planning to provide a follow up PR to switch the default behaviour of the Helper dialog and make OAuth the first option, Users would then need to manually choose Basic Auth.

The problem here is that the user would not necessarily know the reason their username/password didn't work (because they need to auth via OAuth).

Might be worth only presenting the OAuth option for Cloud and only U/P for Server and DC.

App Passwords will still work on Bitbucket.org with Basic Auth.

But I do think when the host is bitbucket.org the UI should be reversed to put OAuth first with the option to switch to Basic Auth and renaming "Basic Auth" to "App Password".

We could also provide a link to the documentation referring to the removal of rights from the user's main u/p ?

But I do think when the host is bitbucket.org the UI should be reversed to put OAuth first with the option to switch to Basic Auth and renaming "Basic Auth" to "App Password".

I think this is the right path forward - is this something you'd be able to take on?

Agreed! This would be similar to the GitHub auth prompt then (show OAuth first - if possible, and switch to u/p/app pwd if poss)

is this something you'd be able to take on?

I can do. I can't make guarantees about deliver timescales unfortunately.

Do you want it addressed in this PR? I wouldn't really want to delay this fix too long.

No, I think we can take this fix. Thank you, Mike!

ldennington · 2022-06-14T20:06:51Z

src/shared/Atlassian.Bitbucket.Tests/BitbucketHostProviderTest.cs

                }
-
-                // Basic Auth 403-ed so push user through OAuth flow


Do we actually want to remove this OAuth flow?

BitbucketHostProvider: fix runtime exceptions when authentication requests for Bitbucket DC would incorrectly call a Bitbucket Cloud REST API A bug was introduced in commit: 5a2cfd7. Prior to this only authentication requests for Bitbucket Cloud would try and automatically determine if 2FA was required by the current user by calling a Bitbucket Cloud REST API using user provided Basic Auth credentials Commit: 5a2cfd7 removed checking of the current host was Bitbucket Cloud vs DC. This meant the check would be run for Bitbucket Cloud and DC regardless. It would fail for Bitbucket DC The fix is more radical than simply re-instating the check on the type of Bitbucket host. From 1st March 2022 support for using a Bitbucket Cloud user's account password to access REST or Git HTTPS operations has been removed, https://atlassian.community/t5/x/x/ba-p/1948231. As such this automatic test to see if 2FA is required no longer works. Therefore the check against the Bitbucket Cloud REST API has been removed in its entirety

mminns · 2022-06-15T08:32:19Z

GitHub Actions seems to be a bit flaky at the moment?

ldennington · 2022-06-16T15:38:44Z

GitHub Actions seems to be a bit flaky at the moment?

Yea. You hit an issue that we've increasingly been seeing with our Windows CI. I normally just kick it and it works the second time around 😬. Probably worth filing an issue against the actions team, though - I'll do that today.

mminns · 2022-06-16T16:22:56Z

I normally just kick it and it works the second time around

Am I right I don't have the rights to "kick" it?

ldennington · 2022-06-16T20:00:02Z

Am I right I don't have the rights to "kick" it?

I will confess that I find the GitHub permissions model a bit confusing. However, based on this, I think you should have permissions to re-run your PR workflows:

I'm wondering...if you click on the Details link next to any of the checks above, do you see a button in the top right that will allow you to re-run jobs? Should look like this:

mjcheetham

LGTM - I'm ready to follow this up with a change to the UI to more visibly offer OAuth/browser flows for cloud users.

mminns · 2022-06-20T13:03:44Z

I'm wondering...if you click on the Details link next to any of the checks above, do you see a button in the top right that will allow you to re-run jobs? Should look like this:

I don't think I do.

mminns · 2022-06-20T13:04:54Z

LGTM - I'm ready to follow this up with a change to the UI to more visibly offer OAuth/browser flows for cloud users.

@mjcheetham Does that mean you are working on re-ordering the UI or should I still take look?

mjcheetham · 2022-06-20T14:49:04Z

LGTM - I'm ready to follow this up with a change to the UI to more visibly offer OAuth/browser flows for cloud users.

@mjcheetham Does that mean you are working on re-ordering the UI or should I still take look?

Check out #754

mminns force-pushed the issue/722 branch from a8ca456 to 26d6e4c Compare June 14, 2022 15:34

mminns changed the title ~~sissue 722~~ issue 722 Jun 14, 2022

mminns mentioned this pull request Jun 14, 2022

Authentication on Bitbucket DC fails - Redirects to bitbucket.org #722

Closed

mminns force-pushed the issue/722 branch from 26d6e4c to 1ff6843 Compare June 14, 2022 19:53

ldennington reviewed Jun 14, 2022

View reviewed changes

ldennington changed the title ~~issue 722~~ bitbucket: remove 2FA test Jun 14, 2022

mminns force-pushed the issue/722 branch from 1ff6843 to b64cbc3 Compare June 15, 2022 08:26

ldennington approved these changes Jun 16, 2022

View reviewed changes

mjcheetham approved these changes Jun 20, 2022

View reviewed changes

mjcheetham merged commit f128d21 into git-ecosystem:main Jun 20, 2022

ldennington mentioned this pull request Jun 20, 2022

Release 2.0.779 #746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bitbucket: remove 2FA test #743

bitbucket: remove 2FA test #743

mminns commented Jun 14, 2022

ldennington left a comment •

edited

Loading

ldennington Jun 14, 2022

mminns Jun 14, 2022

mjcheetham Jun 15, 2022

mminns Jun 15, 2022 •

edited

Loading

mminns Jun 15, 2022

ldennington Jun 15, 2022

mjcheetham Jun 15, 2022

mminns Jun 16, 2022

ldennington Jun 16, 2022

ldennington Jun 14, 2022

mminns commented Jun 15, 2022

ldennington commented Jun 16, 2022

mminns commented Jun 16, 2022

ldennington commented Jun 16, 2022

mjcheetham left a comment

mminns commented Jun 20, 2022

mminns commented Jun 20, 2022

mjcheetham commented Jun 20, 2022

bitbucket: remove 2FA test #743

bitbucket: remove 2FA test #743

Conversation

mminns commented Jun 14, 2022

ldennington left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mminns Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mminns commented Jun 15, 2022

ldennington commented Jun 16, 2022

mminns commented Jun 16, 2022

ldennington commented Jun 16, 2022

mjcheetham left a comment

Choose a reason for hiding this comment

mminns commented Jun 20, 2022

mminns commented Jun 20, 2022

mjcheetham commented Jun 20, 2022

ldennington left a comment •

edited

Loading

mminns Jun 15, 2022 •

edited

Loading