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.

Sign up for GitHub

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

Jump to bottom

Add definedBy property for authenticator #679

Merged
merged 8 commits into from
Oct 19, 2024
Merged

Add definedBy property for authenticator #679

merged 8 commits into from
Oct 19, 2024

Conversation

Thisara-Welmilla
Copy link
Contributor

@Thisara-Welmilla Thisara-Welmilla commented Oct 4, 2024 •
edited
Loading

Issue:

Related PRs:

With wso2/carbon-identity-framework#5938 PR, we have introduced following new property for authentication configs.

  • DefinedBy - to indicate whether the authenticator is system-defined or user-defined.
  1. SYSTEM: Sytem defined authenticators, all the existing authenticators are this type.
  2. USER: User defined authenticators, all the custom authenticators of authentication extension come under this type.

With this PR following changes are added.

  1. Set new authenticator property DefinedBy for the FederatedAuthenticatedConfig and LocalAuthenticatorConfig objects that pass to the framework.
  2. Retrieve the DefinedBy property from the config and display them in the APIs.
  3. Update yaml files to accommodate above point.

This was referenced Oct 4, 2024
Merged
Merged
@Thisara-Welmilla Thisara-Welmilla marked this pull request as ready for review October 4, 2024 08:15
This was referenced Oct 4, 2024
Closed
Closed
@Thisara-Welmilla Thisara-Welmilla changed the base branch from master to add-definedby-prop October 4, 2024 10:28
@Thisara-Welmilla Thisara-Welmilla force-pushed the add-definedBy-type-prop branch 2 times, most recently from 4996186 to 876ab14 Compare October 4, 2024 11:20
@Thisara-Welmilla Thisara-Welmilla force-pushed the add-definedBy-type-prop branch from 876ab14 to d356fae Compare October 4, 2024 11:21
@Thisara-Welmilla Thisara-Welmilla force-pushed the add-definedBy-type-prop branch from afdfbae to df556be Compare October 7, 2024 16:05
malithie
malithie reviewed Oct 11, 2024
View reviewed changes
.../carbon/identity/api/server/authenticators/v1/core/ServerAuthenticatorManagementService.java Outdated
@@ -421,6 +422,14 @@ private void addIdp(IdentityProvider identityProvider, List<Authenticator> authe
authenticator.setType(Authenticator.TypeEnum.FEDERATED);
authenticator.setImage(identityProvider.getImageUrl());
authenticator.setDescription(identityProvider.getIdentityProviderDescription());
// Only older existing IDP has multiple federated authenticator,
Copy link
Member

Choose a reason for hiding this comment

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

Let's improve the comment here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with commit fb2b665

malithie
malithie reviewed Oct 11, 2024
View reviewed changes
...rc/main/java/org/wso2/carbon/identity/api/server/idp/v1/core/ServerIdpManagementService.java Outdated
@@ -2838,6 +2842,7 @@ private FederatedAuthenticatorConfig createFederatedAuthenticatorConfig(String f
authConfig.setName(authenticatorName);
authConfig.setDisplayName(getDisplayNameOfAuthenticator(authenticatorName));
authConfig.setEnabled(authenticator.getIsEnabled());
authConfig.setDefinedByType(resolveDefinedByType(authenticatorName));
Copy link
Member

Choose a reason for hiding this comment

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

IMO, we cannot rely just on the fact whether this authenticator is system or not user by comparing authenticator name.
From the API it should denote that this is a user defined authenticator.
Then server should accept it's name, identifier etc.
If API doesn't say so it should be considered as system. And the authenticator name should be validated against the set of system defined authenticators

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with commit fb2b665

malithie
malithie previously approved these changes Oct 14, 2024
View reviewed changes
@Thisara-Welmilla Thisara-Welmilla changed the base branch from add-definedby-prop to master October 15, 2024 04:02
@Thisara-Welmilla Thisara-Welmilla dismissed malithie’s stale review October 15, 2024 04:02

The base branch was changed.

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11339054126

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11339054126
Status: failure

@wso2 wso2 deleted a comment from jenkins-is-staging Oct 18, 2024
@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11397771349

@wso2 wso2 deleted a comment from jenkins-is-staging Oct 18, 2024
@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11399707735

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11397771349
Status: cancelled

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11399707735
Status: cancelled

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11403138038

SujanSanjula96
SujanSanjula96 previously approved these changes Oct 18, 2024
View reviewed changes
@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11403138038
Status: failure

@jenkins-is-staging
Copy link

PR builder started
Link: https://github.com/wso2/product-is/actions/runs/11408339798

@jenkins-is-staging
Copy link

PR builder completed
Link: https://github.com/wso2/product-is/actions/runs/11408339798
Status: success

jenkins-is-staging
jenkins-is-staging approved these changes Oct 18, 2024
View reviewed changes
Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

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

Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/11408339798

@Thisara-Welmilla Thisara-Welmilla merged commit c3e995a into wso2:master Oct 19, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashanthamara ashanthamara ashanthamara approved these changes

@jenkins-is-staging jenkins-is-staging jenkins-is-staging approved these changes

@SujanSanjula96 SujanSanjula96 SujanSanjula96 left review comments

@malithie malithie malithie left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Thisara-Welmilla @jenkins-is-staging @malithie @SujanSanjula96 @ashanthamara