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 new DefinedBy property to authenticator config. #5938

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Add new DefinedBy property to authenticator config. #5938

wants to merge 11 commits into from

Conversation

Thisara-Welmilla
Copy link
Contributor

@Thisara-Welmilla Thisara-Welmilla commented Sep 16, 2024 •
edited
Loading

Issue:

With this PR following changes are added.

  1. Add new authenticator property DefinedBy
  2. Add new property for authenticator config objects to hold the authenticator definedBy type.
  3. Update the config creation and saving logic to accommodate this new property.
  4. Add warn logs to identify any flow, this new property is not set.

@jenkins-is-staging

This comment was marked as outdated.

Sign in to view
@jenkins-is-staging

This comment was marked as outdated.

Sign in to view
@jenkins-is-staging

This comment was marked as outdated.

Sign in to view
@codecov Codecov

This comment was marked as outdated.

Sign in to view
@jenkins-is-staging

This comment was marked as outdated.

Sign in to view
@jenkins-is-staging

This comment was marked as outdated.

Sign in to view
@jenkins-is-staging

This comment was marked as outdated.

Sign in to view
@jenkins-is-staging

This comment was marked as outdated.

Sign in to view
@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@Thisara-Welmilla Thisara-Welmilla force-pushed the add-auth-prop branch 2 times, most recently from 9fc695e to 99f7c76 Compare September 17, 2024 14:03
@wso2 wso2 deleted a comment from jenkins-is-staging Sep 18, 2024
@wso2 wso2 deleted a comment from jenkins-is-staging Sep 18, 2024
@wso2 wso2 deleted a comment from jenkins-is-staging Sep 18, 2024
@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@Thisara-Welmilla Thisara-Welmilla force-pushed the add-auth-prop branch 2 times, most recently from 038f4a3 to 10bba5c Compare September 19, 2024 06:04
@malithie
Copy link
Member

@Thisara-Welmilla please refer to the right issue in this PR

malithie
malithie reviewed Sep 23, 2024
View reviewed changes
...c/main/java/org/wso2/carbon/identity/application/common/ApplicationAuthenticatorService.java Outdated
Comment on lines 94 to 97
//TODO: Remove warn log, once feature is ready.
if (authenticator.getDefinedByType() == null) {
LOG.warn("The defined by type is not set for the : " + authenticator.getName());
}
Copy link
Member

Choose a reason for hiding this comment

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

Well, IMO this is not a good practice to follow.
We clearly know that this can be null for exisitng authenticators, so we should move to the default which is system

malithie
malithie reviewed Sep 23, 2024
View reviewed changes
...ain/java/org/wso2/carbon/identity/application/common/model/FederatedAuthenticatorConfig.java Outdated
// TODO: Remove warn log, once feature is ready.
if (federatedAuthenticatorConfig.getDefinedByType() == null) {
federatedAuthenticatorConfig.setDefinedByType(IdentityConstants.DefinedByType.SYSTEM);
LOG.warn("The defined by type is not set for the : " + federatedAuthenticatorConfig.getName());
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 have a debug log here rather than a warn. Also, let's indicate that 'system' is set by default

malithie
malithie reviewed Sep 23, 2024
View reviewed changes
...ain/java/org/wso2/carbon/identity/application/common/model/FederatedAuthenticatorConfig.java
*
* @return String[]
*/
public IdentityConstants.DefinedByType getDefinedByType() {
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this DefinedByType enum defined relative to application authenticator than using IdentityConstants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot do this, as it cause circular dependency.

malithie
malithie reviewed Sep 23, 2024
View reviewed changes
...common/src/main/java/org/wso2/carbon/identity/application/common/model/IdentityProvider.java Outdated
Comment on lines 423 to 429

// TODO: Remove warn log, once feature is ready.
for (FederatedAuthenticatorConfig config: federatedAuthenticatorConfigs) {
if (config.getDefinedByType() == null) {
LOG.warn("The defined by type is not set for the : " + config.getName());
}
}
Copy link
Member

Choose a reason for hiding this comment

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

same comment above applies here

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

@jenkins-is-staging
Copy link

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

jenkins-is-staging
jenkins-is-staging approved these changes Sep 27, 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/11064117139

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@malithie malithie malithie left review comments

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

At least 1 approving review is required to merge this pull request.

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

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