-
Notifications
You must be signed in to change notification settings - Fork 327
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
Add idtoken response #71
Add idtoken response #71
Conversation
Add resource to ADFS redirect
Was required to add resource to the ADFS redirect, or else all claims won't be returned. |
log.Info("configuring ADFS OAuth") | ||
OAuthopts = oauth2.SetAuthURLParam("resource", GenOAuth.RedirectURL) // Needed or all claims won't be included | ||
} | ||
|
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.
I suspect this should be in #68 :)
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.
Yes, it should! I'm going to try and move it.
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.
@@ -21,8 +21,10 @@ import ( | |||
|
|||
// VouchClaims jwt Claims specific to vouch | |||
type VouchClaims struct { | |||
Username string `json:"username"` | |||
Sites []string `json:"sites"` // tempting to make this a map but the array is fewer characters in the jwt | |||
Username string `json:"username"` |
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.
I've been wondering if this should be sub
instead of username
. Do you have an opinion?
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.
No opinion from my side! :)
sorry for the delay in reviewing. re: could you please add corresponding entries to |
I've added it to the |
@simongottschlag thanks again for the contribution. I've made some small adjustments, mostly cosmetic. Could you please test this branch: |
|
By the way, seems like this one should be idpIDToken instead of idpAccessToken: (line 360) Lines 359 to 361 in c28fe55
|
Hi, After testing it again today, it does seem to work. Make sure the change with viper is fixed. |
thanks so much Simon, I've adjusted the branch to remove those config items. Golang by default will initlaze and element of the struct that is not set to its "zero value". For strings, the zero value is an empty string Could you test one more time and then I'll merge? |
Hi, I had to add to do the following change to get it to work: #78 vouch-proxy/handlers/handlers.go Lines 648 to 649 in 2487000
Please remember that I've only tested with ADFS and someone else needs to verify that it works with other providers. |
Hey Simon, I need to look a little closer at how I'm traveling this week and next so I can't promise that it will happen in short order but I think this is going to be best for the longer term architecture of the user object. Thanks much! |
+1 for this |
some of this functionality was implemented in #104 by @artagel @simongottschlag I'd love it if you cared to chime in on #111 which is more along the lines of where you were storing the info in the User object But since the upstream tokens can now being passed in the headers I think it fulfills your original requirement. Do let us know if that's not the case. |
Makes it possible to configure the following two headers:
If they are empty in the configuration, nothing will be added. If configured, they will be added as response headers.
Example:
This will make it possible to use it in nginx like this:
And then use the upstream header to, for example, add it to the backend: