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

Added Principal to OAuthToken #26

Closed
wants to merge 1 commit into from

Conversation

lbredeso
Copy link

This is a simple shortcut to prevent applications from needing to know what piece of additionalInformation should be considered the Principal. This mirrors functionality in dropwizard-oauth.

@llinder
Copy link

llinder commented Jan 24, 2017

Feels a little odd that peculiarities of our token handling is ending up in OSS libraries. Other than that 👍

@jeff-blaisdell
Copy link
Member

I'm a bit with Lance. Also, even in our own mixed up world the user_name isn't technically our principal is it?

@lbredeso
Copy link
Author

@lbredeso
Copy link
Author

@jeff-blaisdell I think it's actually "principal" in additionalInformation.

@lbredeso
Copy link
Author

I pushed a change that I believe is more correct. We already have ST-specific stuff here and in dropwizard-oauth, but I guess it's unclear to me where to draw the line. If folks think this doesn't belong in this repo, no big deal, just let me know, and I can close this PR and compute it myself elsewhere, was really just trying to align w/ dropwizard-oauth, which I now realize isn't correct either.

@jeff-blaisdell
Copy link
Member

I believe the original intent was to somewhat mimic this Spring class:
http://docs.spring.io/spring-security/oauth/apidocs/org/springframework/security/oauth2/common/DefaultOAuth2AccessToken.html

Which we haven't strayed too far from. Our concepts of principal I think are a bit unique.

Another option would be to provide a mechanism for overriding the DefaultOAuthToken impl with our own custom OAuthToken impl in our private auth library.

I defer to @llinder on this one.

@lbredeso
Copy link
Author

I think I found a place in an internal library to do this parsing that I think is better than this. I'm going to close this PR.

@lbredeso lbredeso closed this Jan 25, 2017
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.

3 participants