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

Add method to return token data and modify validateAuthorization() to… #897

Closed
wants to merge 2 commits into from

Conversation

marekk
Copy link

@marekk marekk commented Apr 26, 2018

This is alternative to #851

@simonhamp
Copy link

Thanks for taking the time to submit this PR @marekk, but I can't see the value that this adds over #851.

In fact, I think this is actually a slightly more dangerous implementation as it's even easier to override core validation functionality (esp. without some code duplication) and moves the really critical stuff further away from where it's being called.

It also causes some potential confusion, as the naming of the methods is very similar while their purpose remains distinct. This feels like a side-effect of trying to work within the bounds of the existing contract whilst still splitting the code to make it more easily overridable. Your validateAuthorizationHeader is clearly named (if a bit more verbose than necessary), but your amended validateAuthorization is actually just adding claims to the token, not really validating anything.

@simonhamp simonhamp closed this Apr 30, 2018
@marekk
Copy link
Author

marekk commented Apr 30, 2018

My problem is that I need to remove that validation because we don't use the registered claim names here, and they are all optional.

validateAuthorization is not amended but the original kept for backward compatibility.

@simonhamp
Copy link

Understood. From what you've said it seems like this is very specific to your use-case.

I believe that you would be able to implement an easier fix for your needs if/when #851 does settle on improving how validation and claim extraction is done.

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.

2 participants