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

Logic to support boolean claim values is causing all claim values to be treated as strings #200

Closed
rmak-cpi opened this issue Jan 27, 2020 · 5 comments

Comments

@rmak-cpi
Copy link
Contributor

First of all, thanks to all the contributors who help to create and maintain vouch-proxy, it has definitely made my job easier! So in the spirit of contributing in whatever minor way I can, I would like to report that the following logic at https://github.com/vouch/vouch-proxy/blame/master/handlers/handlers.go#L213 intended for supporting boolean claim values:

	val := fmt.Sprint(v)
	if reflect.TypeOf(val).Kind() == reflect.String {
		// if val, ok := v.(string); ok {

seems to be causing all claim values to be treated like a string since the output of fmt.Sprint is always a string. In particular, handling of groups claims (which are slices of strings) has become rather awkward as there is no unambiguous way to differentiate spaces between strings in the slice and spaces embedded in individual strings.

Please let me know if more information is needed from me or if a PR is preferred.

@bnfinet
Copy link
Member

bnfinet commented Feb 7, 2020

@rmak-cpi sorry for the delay in getting back to you

A PR is preferred. Are you in a position to cut one?

thanks for the kind words about VP

@rmak-cpi
Copy link
Contributor Author

Hi @bnfinet, sorry for the delayed response. I was hoping to come up with some test coverage for the code that I have changed which turned out to be a bit more challenging than I anticipated. I should be able to come up with something by the end of this week (2/14) if not sooner.

@rmak-cpi
Copy link
Contributor Author

Hi @bnfinet, I have created #209 as an attempt to address this issue.

@bnfinet
Copy link
Member

bnfinet commented Mar 20, 2020

Sorry for the delay @rmak-cpi. I need to figure out how best to test this PR, and really how best to test custom claims going forward.

My gut is telling me is time to start building a set of good mocks for each IdP's return values for oauth.user_info_url. The tests you created in #209 are a step in that direction. But that's a bit of a heavy lift.

@artagel would you be in a position to evaluate this PR? It'd be nice to get extra eyes on it.

FWIW here's the compare of the relevant change that introduced the bug..

v0.6.15...v0.6.16#diff-86dfd2701799f0e8216ef67a88e1e12eR209-R218

and here's the commit..
80fcfdb

bnfinet added a commit that referenced this issue Apr 22, 2020
@bnfinet
Copy link
Member

bnfinet commented Apr 22, 2020

thanks again for the fix @rmak-cpi

@bnfinet bnfinet closed this as completed Apr 22, 2020
bnfinet added a commit that referenced this issue May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants