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

Allow configuration of returned groups via authproxy connector #2371

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

seuf
Copy link
Contributor

@seuf seuf commented Jan 14, 2022

Overview

Added groupHeader and groups configuration for authproxy connector.

Example :

connectors:
      - type: authproxy
        id: http-proxy
        name: http-proxy
        config:
          userHeader: X-Forwarded-User # default is X-Remote-User
          groupHeader: X-Forwarded-Group # default is X-Remote-Group
          staticGroups:
            - admin

What this PR does / why we need it

This allow users to configure argocd dex to use authproxy connector and specify the users default group and the HTTP header to read to get the group.

Copy link
Member

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

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

Hello, @seuf. Thank you for your intention to work on this feature.

  1. I left a comment. We need to figure out the right approach before moving further.
  2. Would you mind adding the note that authproxy now supports groups to the README.md?

    dex/README.md

    Line 78 in 716eef8

    | [AuthProxy](https://dexidp.io/docs/connectors/authproxy/) | no | no | no | alpha | Authentication proxies such as Apache2 mod_auth, etc. |

connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
@seuf
Copy link
Contributor Author

seuf commented Jan 27, 2022

Hi @nabokihms I've updated this PR.

You can now configure both userHeader and groupHeader (defaults to X-Remote-User and X-Remote-Group).
You can also configure a list of groups (can be empty)
if http group header is present then it is appended to the configured group list.

I've tested it on my argocd dex deployment, and it works fine.

@seuf seuf requested a review from nabokihms January 27, 2022 09:02
Copy link
Member

@nabokihms nabokihms left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for your work. There are some comments left. Please look through them when you have time.

And one more thing. I would like you to also make changes to the module documentation to inform users about the feature you have made.
https://github.com/dexidp/website/blob/main/content/docs/connectors/authproxy.md

connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
connector/authproxy/authproxy.go Outdated Show resolved Hide resolved
Via HTTP Header if present and with manually configured staticGroups in authproxy connector

Signed-off-by: seuf <seuf76@gmail.com>
@seuf seuf force-pushed the authproxy-groups-configuration branch from d085bc5 to 4ee9658 Compare January 31, 2022 09:37
@seuf
Copy link
Contributor Author

seuf commented Jan 31, 2022

Hi @nabokihms I've applied the changes you suggested. PR for the website documentation :
dexidp/website#109

@seuf
Copy link
Contributor Author

seuf commented Feb 7, 2022

Hi @nabokihms Do you have time this week to review this again ?

@nabokihms nabokihms added this to the v2.32.0 milestone Mar 3, 2022
@nabokihms nabokihms merged commit 5f9abc5 into dexidp:master Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants