Skip to content

Feature/authentication strategies #78

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

precious-void
Copy link
Contributor

@precious-void precious-void commented Feb 15, 2021

Issue

Current bitbucket lib is not supporting authorization strategies for OAuth.

What I have done?

I have implemented main OAuth2 authorization methods to retrieve access_token.
It's a draft pull request for bitbucket authStrategies. Would be happy if someone will check or test it.

  • Authorization Code Grant
  • Implicit Grant
  • Resource Owner Password Credentials Grant
  • Client Credentials Grant
  • Bitbucket Cloud JWT Grant (urn:bitbucket:oauth2:jwt)

https://developer.atlassian.com/bitbucket/api/2/reference/meta/authentication

Related issue #18

@MunifTanjim
Copy link
Owner

Hey @shtelzerartem , thanks for openning this draft.

I see there are many unrelated changes. It would make reviewing this so much harder. For example:

  • gitignore
  • updating package versions, changing build script, adding other scripts in package.json
  • Changing names for types and variables.

Can you please remove those?

@precious-void
Copy link
Contributor Author

@MunifTanjim yes, for sure!
I still feel a bit frustrated about the structure of auth plugin and its flow.
That is why I have done a draft PR: just to describe the general idea of the organization of authentication strategies.
Also, there are some other things that are missing right now, for example, after hook to refresh access_token if it is expired.

I will probably clean the code soon, so you will be able to review it.

@precious-void precious-void force-pushed the feature/authentication-strategies branch 9 times, most recently from d38e53a to bcb3cff Compare February 28, 2021 09:04
@precious-void
Copy link
Contributor Author

@MunifTanjim hey, I have tried to clear it as much as possible.
There are structural changes, I merged together auth and authentication plugins into one monolith plugin for the first solution. I need your help with splitting them, as it was done in github's octokit.

@precious-void precious-void force-pushed the feature/authentication-strategies branch from bcb3cff to 5d63fd0 Compare February 28, 2021 09:10
@precious-void precious-void marked this pull request as ready for review March 4, 2021 07:38
@Raspikabek
Copy link

It might be the case I'm implementing it wrong, but I'm getting an error when trying to test this.

What I've done so far:

  • cloned @shtelzerartem repository & checked out to feature/authentication-strategies branch
  • executed yarn & yarn pack
  • created a test script and implementing the method in the following form:
import { Bitbucket } from './lib/index.js'

const doSomething = async () => {
  const options = {
    authStrategy: 'OAuth',
    auth: {
      grant_type: 'clientCredentialsGrant',
      client_id: 'OAUTH_CONSUMER_CLIENTID',
      client_secret: 'OAUTH_CONSUMER_SECRET'
    }
  }
  try {
    const bitbucket = new Bitbucket(options)
    console.log(await bitbucket.auth())
    const result = await bitbucket.user.get({})
    console.log(result)
  } catch (err) {
    console.error(err)
  }
}

doSomething()

This returns the following error:

TypeError: Cannot read property 'defaults' of undefined
    at N (~/node-bitbucket/lib/index.js:1:12882)
    at J (~/node-bitbucket/lib/index.js:1:13336)
    at ~/node-bitbucket/lib/index.js:1:13675
    at ~/node-bitbucket/lib/index.js:1:13605
    at ~/node-bitbucket/lib/index.js:1:13641
    at ~/node-bitbucket/lib/index.js:1:13511
    at ~/node-bitbucket/lib/index.js:1:14210
    at ~/node-bitbucket/lib/index.js:1:14078
    at ~/node-bitbucket/lib/index.js:1:14112
    at ~/node-bitbucket/lib/index.js:1:13984

If I change the auth method to use an AppPassword does return my account information accordingly.

Wonder if there's something wrong in the test script or I'm missing something right now. I guess the expected result is to start the authentication process. I wonder if there's any specific callback URL we should use like HTTPs://localhost:1234/oauth2 or whatsoever in the setup of the bitbucket OAuth consumer application.

Thanks and great job! I'm really keen to see this working and report any issues. (don't have the skill-set right now to contribute in the development side)

@precious-void
Copy link
Contributor Author

precious-void commented Mar 27, 2021

@Raspikabek hey, thanks for testing! Yep, there was a bug, that I have fixed in the latest commit.
About implementation — everything is right.

About callbacks. Are you talking about Authorization Code Grant and Implicit grant authentication methods, where the user has to be redirected to bitbucket and then returned back with token params in the query? If yes, unfortunately, I haven't been working on this part.

@Raspikabek
Copy link

@Raspikabek hey, thanks for testing! Yep, there was a bug, that I have fixed in the latest commit.
About implementation — everything is right.

About callbacks. Are you talking about Authorization Code Grant and Implicit grant authentication methods, where the user has to be redirected to bitbucket and then returned back with token params in the query? If yes, unfortunately, I haven't been working on this part.

Nice! Now seems to be working! Thanks for the quick response.

Related to Implicit grant & Authorization Code Grant I guess that might require to implement and import an express application of some sort to include it in the package.

BTW Authorization Code Grant even though I've added a code, it does return an error:

  error: {
    error_description: 'Missing required field: code',
    error: 'invalid_request'
  },

Anyway... I guess the best approach to implement a secure login process using this library would be by using the JWT Auth and implementing the authorization process to get the JWT token from my own application (the one that requires this library) using something like this example provided by Atlassian

Again thanks a million for the hard work!

@precious-void
Copy link
Contributor Author

@Raspikabek thank you for another one bug! I will fix it soon.

About Implicit grant and Authorization Code Grant (all the https://bitbucket.org/site/oauth2/authorize requests). This library must not provide a way to resolve them, but just allow you to authenticate requests having responses from them.

Implicit grant
After redirect to your service you will be able to pull out from URL #access_token={token}&token_type=bearer access_token and him as option to Bitbucket Object.

new Bitbucket({
  auth: {
    type: 'token',
    token: '<YOUR BEARER TOKEN>',
  },
})

Authorization Code Grant
From https://bitbucket.org/site/oauth2/authorize?client_id={client_id}&response_type=code you will be redirected to URL with ?code={code}, which you will be able to use further.

new Bitbucket({
  authStrategy: 'OAuth',
  auth: {
    grant_type: 'authorizationCodeGrant',
    client_id: '<CLIENT ID>',
    client_secret: '<CLIENT SECRET>',
    code: '<CLIENT CODE>',
  },
})

With JWT Auth I think, the same trick.

- Added validation for strategies
- Fixed Endpoint interface
@precious-void
Copy link
Contributor Author

@MunifTanjim have you had a chance to go over, it looks, like everything we went over with @Raspikabek work fine.

client.requestHook.before(beforeRequest.bind(null, state))
}

export default authenticatePlugin
Copy link
Owner

Choose a reason for hiding this comment

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

How is removing this plugin related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there is a need for authenticate, I moved it's logic to auth/basicAuth.

Copy link
Owner

Choose a reason for hiding this comment

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

This is an optional plugin that is not included in the bundle by default. This is here for legacy reason. If we want to remove it, we should put a deprecation notice first.

Bitbucket's API specificaiton is troublesome enough, that introduces breaking changes frequently. I just don't want to add more to it from this library.

Eitherway, this removing this is not related to this PR at all. "If" we want to do this, this should be done in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sounds reasonable!

@precious-void precious-void force-pushed the feature/authentication-strategies branch from 81a28f5 to 9fac6f0 Compare April 5, 2021 07:14
@precious-void precious-void force-pushed the feature/authentication-strategies branch from 9fac6f0 to e10df78 Compare April 5, 2021 07:16
@precious-void
Copy link
Contributor Author

@MunifTanjim any update on this?

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.

3 participants