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

KEYCLOAK-1222 - BasicAuth KeycloakSecurityContext token issue #1167

Merged
merged 2 commits into from Apr 22, 2015
Merged

KEYCLOAK-1222 - BasicAuth KeycloakSecurityContext token issue #1167

merged 2 commits into from Apr 22, 2015

Conversation

panga
Copy link
Contributor

@panga panga commented Apr 21, 2015

No description provided.

@mposolda
Copy link
Contributor

Thanks for the fix and PR!

One small thing - since the issue is just for Basic authentication, is it possible to fix it in BasicAuthRequestAuthenticator instead of BearerTokenRequestAuthenticator? I believe you can set tokenString in the BasicAuthRequestAuthenticator.authenticate right before invoking authenticateToken. What do you think?

@panga
Copy link
Contributor Author

panga commented Apr 21, 2015

@mposolda it's another approach, but I agree. PR updated.
IMHO that class should have a refactoring.
It's strange BasicAuthRequestAuthenticator extends BearerTokenRequestAuthenticator.

mposolda added a commit that referenced this pull request Apr 22, 2015
KEYCLOAK-1222 - BasicAuth KeycloakSecurityContext token issue
@mposolda mposolda merged commit 955967f into keycloak:master Apr 22, 2015
@mposolda
Copy link
Contributor

Merged, thanks! I am not sure about needed refactoring as there is some common code between BasicAuthRequestAuthenticator and BearerTokenRequestAuthenticator. Fact is that whole BasicAuthRequestAuthenticator and support for basic authentication is kind of corner case, which is useful for legacy applications, but we don't officially recommend to use it (not sure if it's even documented in our user guide).

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

2 participants