-
Notifications
You must be signed in to change notification settings - Fork 539
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
Improve auth framework with authenticator type. #5854
base: authentication-extension
Are you sure you want to change the base?
Improve auth framework with authenticator type. #5854
Conversation
1d2acfb
to
22e9aa3
Compare
fc0870a
to
35adc6b
Compare
7311b5c
to
c54c2a7
Compare
c54c2a7
to
3b41d60
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
31bbc5c
to
5417c4a
Compare
5417c4a
to
567d5aa
Compare
default AuthenticatorType getAuthenticatorType() { | ||
|
||
return AuthenticatorType.FLOW_HANDLER; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed ?
Does this authenticator get returned back in authenticator API ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No this authenticator get returned back in authenticator API. But as we are overriding this method in all other marker interface, though of adding for this also.
If this is unnecessary will remove this.
@@ -713,7 +713,8 @@ protected void doAuthentication(HttpServletRequest request, HttpServletResponse | |||
} | |||
|
|||
String idpName = FrameworkConstants.LOCAL_IDP_NAME; | |||
if (context.getExternalIdP() != null && authenticator instanceof FederatedApplicationAuthenticator) { | |||
if (context.getExternalIdP() != null && (AuthenticatorType.FEDERATED.equals(authenticator | |||
.getAuthenticatorType()) || AuthenticatorType.CUSTOM.equals(authenticator.getAuthenticatorType()))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to check for the user here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, the authentication have not happened yet, therefore, cannot check user type. Also this check is just to decide, what should be goes to diagnostic log; whether LOCAL and specific idp name based on the authenticator Type (as local authenticators does not have associated IDP).
80458db
to
3737a6e
Compare
3737a6e
to
a4dc9b6
Compare
Task issue:
With above feature, we will introduce an authentication adapter that extends the
AbstractApplicationAuthenticator
and implements theFederatedApplicationAuthenticator
class. This adapter will support to authenticated both LOCAL and FEDERATED users and user type will be defined at runtime. The authentication flow will need to accommodate both types. To achieve this, the following improvements will introduced with this PR:getAuthenticatorType()
to theApplicationAuthenticator
interface, with UNDEFINED as the default return value.ApplicationAuthenticator
will override thegetAuthenticatorType()
method to return their specific authenticator type.instanceof FederatedApplicationAuthenticator
check, which ideally should be based on the authenticated user (isFedUser()
) of the corresponding step. Those will be replaced withAuthenticatorType.FEDERATED.equals(authenticator.getAuthenticatorType())
to support the existing behavior.(AuthenticatorType.CUSTOM.equals(authenticator.getAuthenticatorType()) && stepConfig.getAuthenticatedUser().isFederatedUser())
.