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

feat: Add team groups support to bitbucket connector #1688

Merged
merged 1 commit into from
Oct 4, 2020

Conversation

nabokihms
Copy link
Member

@nabokihms nabokihms commented Apr 16, 2020

Description:

This PR provides support for team groups. The only option added is includeTeamGroups. When the option value is true, we make an additional request to Bitbucket API to get all team group names and combine them with the team name using /.

The groups claim in dex id_token looks like:

  • Without option: groups=["my_team"]
  • With option: groups=["my_team", "my_team/administrators", "my_team/members"]

Motivation:

I started working on it because there was no chance to do authn/authz management in the frames of one team. This behavior is something that other connectors already has (github, gitlab).

Concerns:

My only concern is sending a request to deprecated API to get team groups. However, Atlassian said, that /groups endpoint would be supported until the equal method would appear in 2.0 API.

@elebioda95
Copy link

I really like this, and need this, but I can't seem to get the groups to be added to the claim. I believe this is due to the comparison of the original whitelisted teams in config compared to what is provided after group concatenation?

@nabokihms
Copy link
Member Author

@crytome1995 to make includeTeamGroups: true works with teams filter correctly, you should add additional entries to your filter like:

includeTeamGroups: true
teams: ["my_team", "my_team/everyone", "my_team/administrators"]

This behavior helps to filter unnecessary groups from the result ID token.

@nabokihms
Copy link
Member Author

I just rebased the branch to activate new CI jobs.

@sagikazarmark
Copy link
Member

@nabokihms Thanks! The PR looks OK.

Regarding the filter: I believe other connectors (GitHub, GitLab) behave the same way, but what pains me is that config looks differently (again). 😢

@nabokihms
Copy link
Member Author

In this particular case, we can avoid adding an option. We just need to inform users that default behavior will change (I'm not a big fan of breaking changes, but it has its pros and cons). By the way, I am looking forward to your decision about this PR :)

P.S.:
About difference in configs... It's hard to keep connectors flexible enough without kind specific parameters.

In my opinion, it will be good to move some options from providers config to a higher level, like baseURL, clientID, clientSecret, etc, and use the config section only for unique connector options.

@sagikazarmark
Copy link
Member

I'm actually a huge fan of how it is done in GitHub: https://github.com/banzaicloud/pipeline/blob/cb95d36/etc/config/dex.yml.dist#L104-L113

It's well structured, easy to understand.

In most of the cases just a concept or some rules to follow would be enough. But since there is no such thing, everyone just invents their own solutions.

@sagikazarmark
Copy link
Member

Also, there is this: #1635

@sagikazarmark
Copy link
Member

@nabokihms if you rebase this PR I'll merge it as well. Thanks!

Signed-off-by: m.nabokikh <maksim.nabokikh@flant.com>
@nabokihms
Copy link
Member Author

I rebased the branch and resolved conflicts.

Copy link
Member

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks @nabokihms

@sagikazarmark sagikazarmark merged commit 828a1c6 into dexidp:master Oct 4, 2020
nate-double-u added a commit to nate-double-u/dexidp-website that referenced this pull request Oct 5, 2020
Signed-off-by: Nate Waddington <nwaddington@cncf.io>
sagikazarmark pushed a commit to dexidp/website that referenced this pull request Oct 6, 2020
* Updating bitbucketcloud.md file as per changes in PR dexidp/dex#1688

Signed-off-by: Nate Waddington <nwaddington@cncf.io>

* updating bitbucketcloud.md copy as per changes introduced in dexidp/dex#1812

Signed-off-by: Nate Waddington <nwaddington@cncf.io>

* correcting content update as per dexidp/dex#1812

Signed-off-by: Nate Waddington <nwaddington@cncf.io>
kerberos727 added a commit to kerberos727/dex-project that referenced this pull request Aug 23, 2024
* Updating bitbucketcloud.md file as per changes in PR dexidp/dex#1688

Signed-off-by: Nate Waddington <nwaddington@cncf.io>

* updating bitbucketcloud.md copy as per changes introduced in dexidp/dex#1812

Signed-off-by: Nate Waddington <nwaddington@cncf.io>

* correcting content update as per dexidp/dex#1812

Signed-off-by: Nate Waddington <nwaddington@cncf.io>
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