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

Add Bitbucket connector #1307

Merged
merged 5 commits into from
Oct 12, 2018
Merged

Conversation

edtan
Copy link
Contributor

@edtan edtan commented Sep 30, 2018

This adds a connector for Bitbucket Cloud. It supports mapping a Bitbucket user's teams to the groups claim. Most of the code was based off of the existing Github and Microsoft connectors, so please let me know if I'm missing anything.

@whereisaaron
Copy link

Excellent, we are really missing this capability from dex v1. It will be great to get it back. Thanks for your work @edtan.

@marco-m
Copy link

marco-m commented Oct 3, 2018

@srenatus any chance you could have a look at this PR or suggest somebody ? Thanks :-)

@srenatus
Copy link
Contributor

srenatus commented Oct 3, 2018

I'll try to review this tomorrow. Sorry for the wait.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Some comments, mostly minor nitpicks. Thank you for working on this 🎉

Documentation/connectors/bitbucket.md Outdated Show resolved Hide resolved
# Required field for connector id.
id: bitbucket
# Required field for connector name.
name: Bitbucket
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 This could also default to Bitbucket but be configurable, I suppose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I wasn't sure where we can set a default for the connector's name. It seems like it currently just comes from the yaml?

Documentation/connectors/bitbucket.md Outdated Show resolved Hide resolved
connector/bitbucket/bitbucket.go Outdated Show resolved Hide resolved
connector/bitbucket/bitbucket.go Outdated Show resolved Hide resolved
connector/bitbucket/bitbucket.go Outdated Show resolved Hide resolved
connector/bitbucket/bitbucket.go Outdated Show resolved Hide resolved
// https://developer.atlassian.com/bitbucket/api/2/reference/resource/user
type user struct {
Username string `json:"username"`
DisplayName string `json:"display_name"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't seem to be using DisplayName. I wonder if we could 🤔

Copy link
Contributor Author

@edtan edtan Oct 6, 2018

Choose a reason for hiding this comment

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

When adding the Bitbucket connector, I was initially working with Concourse's fork of Dex and they had added a Name field to connector.Identity, which is where DisplayName was used. However, I forgot to remove this field earlier when working on this PR. I've removed the DisplayName field now.

connector/bitbucket/bitbucket_test.go Outdated Show resolved Hide resolved
connector/bitbucket/bitbucket_test.go Outdated Show resolved Hide resolved
@whereisaaron
Copy link

Thanks for working in this @edtan 🎉 Looking forward to testing it out.

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thank you so much. I think this is a great addition 🎈

LGTM -- @ericchiang, any concerns on your end?

@marco-m
Copy link

marco-m commented Oct 12, 2018

@ericchiang any chance you could have a look at this PR ? Thanks :-)

@ericchiang
Copy link
Contributor

none from me. Feel free to merge without my lgtm in the future 😄

@srenatus srenatus merged commit e1acb6d into dexidp:master Oct 12, 2018
@srenatus
Copy link
Contributor

Feel free to merge without my lgtm in the future 😄

Thanks, acknowledged 😃

@srenatus
Copy link
Contributor

@edtan Thanks a lot for your contribution 🎈

@marco-m
Copy link

marco-m commented Oct 12, 2018

@edtan, @srenatus, @ericchiang Thanks a lot, this is very helpful for my team :-)

@edtan edtan deleted the upstream-add-bitbucket-connector branch October 12, 2018 11:23
mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019
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