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

ISSUE-1330: Allow jwt or knox sso as underlying authentication for SAM #1331

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

priyank5485
Copy link
Contributor

Change-Id: I788f12ffb4e0e673d5c6edbfad90e43b9d401a6b

Change-Id: I788f12ffb4e0e673d5c6edbfad90e43b9d401a6b
@priyank5485
Copy link
Contributor Author

@arunmahadevan I have tested this on a cluster and it works fine. Please review. Ideally it will be good to put this in the next maint release and all release going forward.

Copy link
Contributor

@arunmahadevan arunmahadevan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks ok. May be good to rename the classes and related method now that it supports more than kerberos and pass the right authType to the securityContext as well.

if (principal == null || !httpRequest.getAuthType().equalsIgnoreCase(KERBEROS_AUTH)) {
// We now also support Knox SSO using jwt as underlying authentication mechanism. Hence added jwt as a valid type
// Probably jwt does not sound right in this class and we are also passing KERBEROS_AUTH for jwt below where we
// instantiate a SecurityContext. That is because there are checks down the line for kerberos as well
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe then this class needs to be renamed to StreamlineAuthenticationRequestFilter and the right authType passed to the StreamlineSecurityContext as well?

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