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 authentication Types for authenticators. #5901

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

Conversation

Thisara-Welmilla
Copy link
Contributor

@Thisara-Welmilla Thisara-Welmilla commented Aug 31, 2024

Task issue:

With above feature, we will introduce an authentication adapter that extends the AbstractApplicationAuthenticator and implements the FederatedApplicationAuthenticator 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:

  1. Introduced new authenticator types (SYSTEM and CUSTOM).
  2. Introduce new method, getAuthenticatorType() to the ApplicationAuthenticator interface, with SYSTEM as the default return value.
  3. The authenticator extension will override the getAuthenticatorType() method to return their CUSTOM type.
  4. The some authentication flows logic differs based on the instanceof FederatedApplicationAuthenticator check, which ideally should be based on the authenticated user (isFedUser()) of the corresponding step. Those will be updated.

@Thisara-Welmilla Thisara-Welmilla force-pushed the add-authentication-type branch 2 times, most recently from 6121b29 to 6f22057 Compare September 1, 2024 12:33
AuthenticatorType authenticatorType = authenticator.getAuthenticatorType();
if ((AuthenticatorType.SYSTEM.equals(authenticatorType) && authenticator instanceof
FederatedApplicationAuthenticator) || (AuthenticatorType.CUSTOM.equals(authenticatorType)
&& stepConfig.getAuthenticatedUser().isFederatedUser())) {
Copy link
Member

Choose a reason for hiding this comment

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

does step config has the respective user authenticated by the respective authenticator ?

@malithie
Copy link
Member

malithie commented Sep 2, 2024

Let's cover the test cases along with the PR or let's merge to a feature branch and merge as tests are covered only

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