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

Refactor registry impl code #5787

Merged
merged 1 commit into from
Jul 12, 2024
Merged

Refactor registry impl code #5787

merged 1 commit into from
Jul 12, 2024

Conversation

Yoshani
Copy link
Contributor

@Yoshani Yoshani commented Jul 11, 2024 •
edited
Loading

Proposed changes in this pull request

Refactors the code.
Also brings the ability to delete already published policies from PAP back, since it can be supported by the DB impl.

Related issues

Checklist (for reviewing)

General

  • Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
  • Is the PR labeled correctly?

Functionality

  • Are all requirements met? Compare implemented functionality with the requirements specification.
  • Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.

Code

  • Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
  • Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
  • Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.

Tests

  • Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
  • If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
  • If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?

Security

  • Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
  • Are all UI and API inputs run through forms or serializers?
  • Are all external inputs validated and sanitized appropriately?
  • Does all branching logic have a default case?
  • Does this solution handle outliers and edge cases gracefully?
  • Are all external communications secured and restricted to SSL?

Documentation

  • Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes should be documented.
  • Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented.
  • Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.

@Yoshani Yoshani force-pushed the feature-remove-registry-xacml branch from 3cd1181 to 0846ae8 Compare July 11, 2024 08:08
darshanasbg
darshanasbg reviewed Jul 11, 2024
View reviewed changes
...ntitlement/src/main/java/org/wso2/carbon/identity/entitlement/dao/RegistryPolicyDAOImpl.java Outdated
@@ -84,6 +88,7 @@ public RegistryPolicyDAOImpl() {
@Override
public void init(Properties properties) {

// Do nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we avoid overriding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reintroduced the legacy implementation where we set the pdp path reading entitlement properties. Earlier impl seemed to disregard the value set in the properties file.

darshanasbg
darshanasbg requested changes Jul 11, 2024
View reviewed changes
...ntitlement/src/main/java/org/wso2/carbon/identity/entitlement/dao/RegistryPolicyDAOImpl.java
LOG.debug("Removing entitlement policy");
}

// Restricts removing policies, that have already been published
Copy link
Contributor

Choose a reason for hiding this comment

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

Original registry implementation removes the policy from PAP even its published..
But with [1] we decided and implement to avoid removing policies that are published..
But when implementing this, we noted that behaviour change is not needed and we can keep the original behaviour..

Can you confirm this is the reason to remove this code block @Yoshani ?

[1] https://github.com/wso2/carbon-identity-framework/pull/5686/files#diff-ed3186cd101a5776b4286f80107092059209ba673c0aa964230720ea17493cf9R425-R430

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this behavior can be supported in the RDBMS implementation, hence this behavioral change has been reverted.

...ntitlement/src/main/java/org/wso2/carbon/identity/entitlement/dao/RegistryPolicyDAOImpl.java Outdated Show resolved Hide resolved
...ntitlement/src/main/java/org/wso2/carbon/identity/entitlement/dao/RegistryPolicyDAOImpl.java Outdated Show resolved Hide resolved
...ntitlement/src/main/java/org/wso2/carbon/identity/entitlement/dao/RegistryPolicyDAOImpl.java
@@ -179,7 +203,7 @@ public String[] getVersions(String policyId) {
}
}
} catch (RegistryException e) {
LOG.error("Error while creating new version of policy", e);
Copy link
Contributor

Choose a reason for hiding this comment

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

Was previous message wrong, and you are correcting it over here?

Copy link
Contributor Author

@Yoshani Yoshani Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, this method retrieves existing versions, not creating any

...ement/src/main/java/org/wso2/carbon/identity/entitlement/pap/store/PAPPolicyStoreReader.java Outdated Show resolved Hide resolved
@Yoshani Yoshani force-pushed the feature-remove-registry-xacml branch 3 times, most recently from 2fadf7a to 799b5ad Compare July 12, 2024 05:40
Thumimku
Thumimku reviewed Jul 12, 2024
View reviewed changes
...lement/src/main/java/org/wso2/carbon/identity/entitlement/dao/RegistrySubscriberDAOImpl.java
/**
* This implementation handles the subscriber management in the Registry.
*/
public class RegistrySubscriberDAOImpl implements SubscriberDAO {

public static final String SUBSCRIBER_ID = "subscriberId";
Copy link
Contributor

Choose a reason for hiding this comment

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

why are we removing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is added to the PDPConstants class instead. Since RegistrySubscriberDAOImpl.java class was newly introduced, it is safe to remove this public constant.

@Yoshani Yoshani force-pushed the feature-remove-registry-xacml branch from 799b5ad to 511f6fc Compare July 12, 2024 06:03
Thumimku
Thumimku approved these changes Jul 12, 2024
View reviewed changes
Copy link
Contributor

@Thumimku Thumimku left a comment

Choose a reason for hiding this comment

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

LGTM

@Yoshani Yoshani merged commit 6459aeb into wso2:feature-remove-registry-xacml Jul 12, 2024
1 check passed
This was referenced Jul 12, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darshanasbg darshanasbg darshanasbg requested changes

@Thumimku Thumimku Thumimku approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@Yoshani @darshanasbg @Thumimku