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.

Sign up for GitHub

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

Jump to bottom

Add Bitbucket provider. #201

Merged
merged 3 commits into from
Aug 16, 2019
Merged

Add Bitbucket provider. #201

merged 3 commits into from
Aug 16, 2019

Conversation

aledegano
Copy link
Contributor

Description

Adds Bitbucket to the providers, includes tests.

Motivation and Context

Fix #40

How Has This Been Tested?

This PR is based on a fork of the original Bitly project. We've been using that fork in PROD for several months by now.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

@aledegano aledegano requested a review from a team July 2, 2019 06:57
JoelSpeed
JoelSpeed requested changes Jul 15, 2019
View reviewed changes
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

There's a lot of spurious logging in this PR which will make the logs quite noisy, can we remove the debug logging and for the error logging that we keep, use our own "github.com/pusher/oauth2_proxy/pkg/logger" package which wraps logs in a consistent manner across the project?

Also, since I'm not a bitbucket user, I'd like you to add yourself as CODEOWNER for the Bitbucket provider, this way, if there are questions or PRs to it, you should be notified, is that ok?

Finally, please add a changelog entry so that we note when the new provider has been added!

Thanks for the PR!

providers/bitbucket.go Outdated Show resolved Hide resolved
providers/bitbucket.go Outdated Show resolved Hide resolved
providers/bitbucket.go Outdated Show resolved Hide resolved
providers/bitbucket.go Outdated Show resolved Hide resolved
providers/bitbucket.go Outdated Show resolved Hide resolved
steakunderscore
steakunderscore suggested changes Jul 15, 2019
View reviewed changes
options.go Outdated Show resolved Hide resolved
providers/providers.go Outdated Show resolved Hide resolved
@aledegano
Copy link
Contributor Author

Thank you @JoelSpeed and @steakunderscore for your review!
I think I've addressed all your comments.

steakunderscore
steakunderscore suggested changes Jul 16, 2019
View reviewed changes
options.go Show resolved Hide resolved
options.go Show resolved Hide resolved
providers/bitbucket.go Show resolved Hide resolved
Add Bitbucket provider.
f32c862 
Add a new provider for Bitbucket,
can be configured from the options
specifying team and/or repository
that the user must be part/have access
to in order to grant login.
@aledegano
Copy link
Contributor Author

I've retested the PR in prod, note that #215 was necessary to properly test, but I've split the fix in its own PR.
Seems to be working as expected.

steakunderscore
steakunderscore previously approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@steakunderscore steakunderscore left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM

@YusukeIwaki
Copy link

It would be better to define GetUserName, like GitHub provider.

GetUserName has been added to the Provider interface to get User value when not equal to Email's local-part.

(from bitly/oauth2_proxy#466)

The same may be said of BitBucket account.

@steakunderscore
Copy link
Contributor

It would be better to define GetUserName, like GitHub provider.

Agreed it would be useful to define GetUserName, but I would fine with that being added in separate PR as it's not a requirement.

@davidholsgrove
Copy link
Contributor

Tested against our Bitbucket org and this looks great @aledeganopix4d
Looking forward to this being merged into master / a release 👍

@JoelSpeed JoelSpeed requested a review from loshz as a code owner August 16, 2019 12:57
JoelSpeed
JoelSpeed approved these changes Aug 16, 2019
View reviewed changes
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

steakunderscore
steakunderscore approved these changes Aug 16, 2019
View reviewed changes
Copy link
Contributor

@steakunderscore steakunderscore left a comment

Choose a reason for hiding this comment

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

Nice 👍

@JoelSpeed JoelSpeed merged commit fa6c479 into oauth2-proxy:master Aug 16, 2019
thereallukl pushed a commit to thereallukl/oauth2_proxy that referenced this pull request Sep 6, 2019
@aledegano @thereallukl
Add Bitbucket provider. (oauth2-proxy#201)
81f1525 
Add a new provider for Bitbucket,
can be configured from the options
specifying team and/or repository
that the user must be part/have access
to in order to grant login.
@YusukeIwaki YusukeIwaki mentioned this pull request Sep 7, 2019
Closed
@szczeles szczeles mentioned this pull request Mar 15, 2020
Merged
3 tasks
T-vK pushed a commit to T-vK/oauth2-proxy that referenced this pull request Nov 20, 2024
@pierluigilenoci
Merge pull request oauth2-proxy#201 from StianOvrevage/feat/add-envfrom
ec56488 
Add option to add config via envFrom
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steakunderscore steakunderscore steakunderscore approved these changes

@JoelSpeed JoelSpeed JoelSpeed approved these changes

@loshz loshz Awaiting requested review from loshz

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BitBucket support
5 participants
@aledegano @YusukeIwaki @steakunderscore @davidholsgrove @JoelSpeed