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

Read extended user attributes from auth proxy #7547

Merged
merged 1 commit into from Feb 23, 2016
Merged

Read extended user attributes from auth proxy #7547

merged 1 commit into from Feb 23, 2016

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Feb 23, 2016

Bring the requestheader IDP to parity with the other providers by allowing setting the display name, email, and preferred username from header values.

Added config for listing headers to read display name, email, and preferred username from. Example config:

oauthConfig:
  ...
  identityProviders:
  - name: "..."
    provider:
      apiVersion: v1
      kind: RequestHeaderIdentityProvider
      headers:
      - X-Remote-User
      emailHeaders:
      - X-Remote-User-Email
      nameHeaders:
      - X-Remote-User-Display-Name
      preferredUsernameHeaders:
      - X-Remote-User-Login

@liggitt
Copy link
Contributor Author

liggitt commented Feb 23, 2016

@deads2k @smarterclayton

@liggitt
Copy link
Contributor Author

liggitt commented Feb 23, 2016

[test]

@liggitt
Copy link
Contributor Author

liggitt commented Feb 23, 2016

@sgallagher PTAL

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to f6a0c52

}
if len(username) == 0 {
id := headerValue(req.Header, a.config.IDHeaders)
if len(id) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that this is incorrect, but since this is now a function, wouldn't the logic be better if we had it return a boolean success or failure, rather than running a len() on the return? I'd think that would be less error-prone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if I had that bool, I'd still do the length check, because we can't successfully provision an identity with an empty id. when we have a case where we care about a present-but-empty value, it'd be fine to add then

@sgallagher
Copy link
Contributor

LGTM

@liggitt
Copy link
Contributor Author

liggitt commented Feb 23, 2016

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/1533/)

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/1533/) (Image: devenv-rhel7_3520)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to f6a0c52

openshift-bot pushed a commit that referenced this pull request Feb 23, 2016
@openshift-bot openshift-bot merged commit 90de96b into openshift:master Feb 23, 2016
@liggitt liggitt deleted the request-header-attributes branch February 27, 2016 02:47
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.

None yet

3 participants