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

Propagate groups from upstream OIDC provider #1065

Open
furuholm opened this issue Sep 15, 2017 · 25 comments
Open

Propagate groups from upstream OIDC provider #1065

furuholm opened this issue Sep 15, 2017 · 25 comments

Comments

@furuholm
Copy link
Contributor

furuholm commented Sep 15, 2017

The OIDC connectors currently discards the groups claim. I would like this to be propagated.

@ericchiang
Copy link
Contributor

This is blocked on #863

Since so many providers implement refresh tokens differently, we don't actually re-query the upstream provider when a dex client refresh its token. So we don't have any way of keeping the groups list fresh.

@furuholm
Copy link
Contributor Author

I see. But how does groups differ from the other properties that are propagated in the current version (username, email, email verified)? Does those never change?

@ericchiang
Copy link
Contributor

Groups are much more likely to change dynamically. I'm not saying that it's good that we don't update the other claims, but we definitely want to address this before expanding our features to groups

@furuholm
Copy link
Contributor Author

Makes sense. I can use a fork until #863 is solved.

Closing this issue. Feel free to reopen if you want to track this as something to do after #863 has been solved.

@ericchiang
Copy link
Contributor

Yeah, going to re-open for tracking this issue. Thanks for opening and sorry we don't have a better answer today.

@jacksontj
Copy link
Contributor

@ericchiang I have recently run into this a few times, would you guys be open to merging an option to pull groups? I understand that they don't get update when using a refresh token, but as it stands nothing else does either. My proposal would be to (1) add an option to enable/disable groups for OIDC or (2) allow it to be enabled through config (as is) -- either way it sounds like we should specifically call out the behavior in the docs.

@jacksontj
Copy link
Contributor

I've opened #1434 with my proposed workaround.

@Morriz
Copy link

Morriz commented Jul 13, 2019

Any updates here? Concourse forked dex and managed to pull in groups. I am with @jacksontj on this one: offer documentation and recommendations (if using groups set short ttl).
Any danger from stale authorization grants can be left to the developer/implementation that way. @ericchiang are you saying no because you want to avoid all possible confusion/risk coming from such a solution? It would still be solid software and adhering to technical design principles, but functionally I think there is a strong need so why oppose?

jacksontj added a commit to jacksontj/dex that referenced this issue Sep 13, 2019
There's been some discussion in dexidp#1065 regarding what to do about
refreshing groups. As it stands today dex doesn't update any of the
claims on refresh (groups would just be another one). The main concern
with enabling it is that group claims may change more frequently. While
we continue to wait on the upstream refresh flows, this adds an option
to enable the group claim. This is disabled by default (so no behavioral
change) but enables those that are willing to have the delay in group
claim change to use oidc IDPs.

Workaround to dexidp#1065
jacksontj added a commit to jacksontj/dex that referenced this issue Sep 13, 2019
There's been some discussion in dexidp#1065 regarding what to do about
refreshing groups. As it stands today dex doesn't update any of the
claims on refresh (groups would just be another one). The main concern
with enabling it is that group claims may change more frequently. While
we continue to wait on the upstream refresh flows, this adds an option
to enable the group claim. This is disabled by default (so no behavioral
change) but enables those that are willing to have the delay in group
claim change to use oidc IDPs.

Workaround to dexidp#1065
dskatz added a commit to dskatz/dex that referenced this issue Nov 6, 2019
This builds on the terrific work in https://github.com/dexidp/dex/pull/1180/files
and dexidp#1065.

This makes some minor changes that bring the approach up-to-date with
current dex versions.
@danielsand
Copy link

Hey Guys,
we really would love to see this happen. <3

@dskatz
Copy link

dskatz commented Dec 4, 2019

Now that Dex v.2.21.0 has been released with both Refresh Tokens and Groups claims, is this still an issue? And if not, could the flag "insecureEnableGroups" be renamed "enableGroups"?
(Referenced PRs: #1180, #1185, #1434)

In the use-cases that I am testing, groups claims are refreshed pretty quickly < 3 minutes in most cases which satisfies requirements. From my perspective there does not seem to be a security issue anymore.

@dene14
Copy link

dene14 commented Feb 2, 2020

@dskatz any guidance how you enabled groups? for me Google always returns groups=[]
Thanks in advance!

@lbvffvbl
Copy link

Guys. I'm trying to enable groups for google. Was added serviceAccountFilePath, adminEmail, insecureEnableGroups: true, but I'm still getting groups=[]. Where am I wrong?

@lbvffvbl
Copy link

@dskatz any guidance how you enabled groups? for me Google always returns groups=[]
Thanks in advance!

Dene14, I found. you need to ask extra-claim groups but you need to use a connector type google, not oidc
https://github.com/dexidp/dex/blob/master/Documentation/connectors/google.md

@abdulsalama
Copy link

hi folks, so does groups now works for oidc or not? I'm trying to use oidc with okta and I tried insecureEnableGroups: true but still getting groups=[]. any idea?

@JoelSpeed
Copy link
Contributor

@abdulsalama Did you make sure to add the groups claim to the scope, as this will also be needed

@abdulsalama
Copy link

@JoelSpeed yes I did. I had this in the config:
scopes:
- openid
- profile
- email
- groups

and I see that groups is being requested in the initial call.

@dskatz
Copy link

dskatz commented Apr 5, 2020

Hi @abdulsalama,

With Okta specifically, groups aren't returned in the idtoken unless you pay for that feature. You will need to make a second call to the userinfo endpoint after requesting the claims initially.

@nabokihms
Copy link
Member

@abdulsalama If you still have the problem, you can fix it specifying getUserInfo: true in OIDC connector settings.

With the enabled option, dex will make a second call to the useinfo endpoint like @dskatz suggest. I tested it yesterday.

@abdulsalama
Copy link

Thanks @dskatz and @nabokihms for the hints. I will give that a try and see if it works.

@mlopez-eb
Copy link

@abdulsalama If you still have the problem, you can fix it specifying getUserInfo: true in OIDC connector settings.

With the enabled option, dex will make a second call to the userinfo endpoint like @dskatz suggest. I tested it yesterday.

hi @nabokihms & @dskatz . If I'm installing Concourse through Helm, where exactly would I edit this getUserInfo: true? I know this is for the dex configuration, but I don't see those options in the values.yaml for the Helm installation.

It's become a nuisance not being able to get a user's groups through OIDC or OAUTH =[

@lentzi90
Copy link

#1634
Does this mean we can close this? The readme also says that the OIDC connector supports group claims now.

@lukasmrtvy
Copy link

lukasmrtvy commented May 24, 2021

@lentzi90 you still need to use insecureEnableGroups, even if you are using claimMapping , but groups are supported, at least in native oidc connector.

@kszymans
Copy link

kszymans commented May 17, 2022

Guys. I'm trying to enable groups for google. Was added serviceAccountFilePath, adminEmail, insecureEnableGroups: true, but I'm still getting groups=[]. Where am I wrong?

Hey.
I have the exact problem and I've checked:

  • Google's IAM service account key is accessible for dex
  • Client claims groups attribute from dex

Although I gets empty groups list. I also gets no groups claim in Dex's approval screen:
image

Maybe the issue is somewhere in claims


Edit:
Nvm.. Instead connector type: google I put type: oidc. After changing to google it worked

@almereyda
Copy link

almereyda commented Feb 4, 2023

Could the issue be rescoped to also include upstream OAuth2 login flows? We are interested in passing on groups provided by GitHub, which is an OAuth2 connector (only) and would else not be covered by the OP.

The title could then read like something along the lines of:

  • Propagate groups from upstream identity provider (OAuth/OIDC)

@afirth
Copy link

afirth commented Apr 28, 2023

@almereyda it seems it's in already: https://github.com/dexidp/dex/blob/master/connector/github/github.go#L317

haven't used it yet, hoping to get this working soon

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

No branches or pull requests