-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
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? |
@crytome1995 to make
This behavior helps to filter unnecessary groups from the result ID token. |
5fb1996
to
e851a26
Compare
I just rebased the branch to activate new CI jobs. |
@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). 😢 |
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.: 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. |
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. |
Also, there is this: #1635 |
@nabokihms if you rebase this PR I'll merge it as well. Thanks! |
Signed-off-by: m.nabokikh <[email protected]>
e851a26
to
ec66ced
Compare
I rebased the branch and resolved conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nabokihms
Signed-off-by: Nate Waddington <[email protected]>
* Updating bitbucketcloud.md file as per changes in PR dexidp/dex#1688 Signed-off-by: Nate Waddington <[email protected]> * updating bitbucketcloud.md copy as per changes introduced in dexidp/dex#1812 Signed-off-by: Nate Waddington <[email protected]> * correcting content update as per dexidp/dex#1812 Signed-off-by: Nate Waddington <[email protected]>
* Updating bitbucketcloud.md file as per changes in PR dexidp/dex#1688 Signed-off-by: Nate Waddington <[email protected]> * updating bitbucketcloud.md copy as per changes introduced in dexidp/dex#1812 Signed-off-by: Nate Waddington <[email protected]> * correcting content update as per dexidp/dex#1812 Signed-off-by: Nate Waddington <[email protected]>
Description:
This PR provides support for team groups. The only option added is
includeTeamGroups
. When the option value istrue
, 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:
groups=["my_team"]
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.