Add service layer to manage authorization details types #6073

VimukthiRajapaksha · 2024-10-28T04:52:28Z

Proposed changes in this pull request

This PR adds CRUD operations to manage Authorization Details Types, allowing for Create, Read, Update, and Delete operations.

Implemented database layer and queries to support the new CRUD operations.
Developed new services for managing Authorization Details Types within the framework.
Created unit tests to ensure the functionality of the new features.

Related Issues

https://github.com/wso2-enterprise/asgardeo-product/issues/22821

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.

- Add SQL queries - Add DAO layer - Modify resource access controls - Add unit tests - Modify APIs to support authorization details types

CLAassistant · 2024-10-28T04:52:34Z

All committers have signed the CLA.

codecov · 2024-10-28T08:31:20Z

Codecov Report

Attention: Patch coverage is 64.63415% with 290 lines in your changes missing coverage. Please review.

Project coverage is 45.75%. Comparing base (1c36a56) to head (52c5d04).
Report is 546 commits behind head on master.

Files with missing lines	Patch %	Lines
...ntity/api/resource/mgt/util/FilterQueriesUtil.java	49.19%	55 Missing and 8 partials ⚠️
...t/dao/impl/AuthorizationDetailsTypeMgtDAOImpl.java	75.31%	45 Missing and 13 partials ⚠️
...application/mgt/dao/impl/AuthorizedAPIDAOImpl.java	76.36%	24 Missing and 15 partials ⚠️
...ication/common/model/AuthorizationDetailsType.java	0.00%	36 Missing ⚠️
...rce/mgt/dao/impl/APIResourceManagementDAOImpl.java	57.14%	10 Missing and 5 partials ⚠️
...CacheBackedAuthorizationDetailsTypeMgtDAOImpl.java	77.04%	9 Missing and 5 partials ⚠️
...ource/mgt/AuthorizationDetailsTypeManagerImpl.java	72.09%	6 Missing and 6 partials ⚠️
...nternal/APIResourceManagementServiceComponent.java	0.00%	11 Missing ⚠️
...entity/application/common/model/AuthorizedAPI.java	0.00%	10 Missing ⚠️
...cation/mgt/AuthorizedAPIManagementServiceImpl.java	62.50%	6 Missing and 3 partials ⚠️
... and 7 more

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #6073      +/-   ##
============================================
+ Coverage     40.82%   45.75%   +4.93%     
+ Complexity    16031    14200    -1831     
============================================
  Files          1813     1639     -174     
  Lines        130155   102071   -28084     
  Branches      22264    17805    -4459     
============================================
- Hits          53130    46701    -6429     
+ Misses        69267    48626   -20641     
+ Partials       7758     6744    -1014

Flag	Coverage Δ
unit	`28.55% <64.63%> (+0.25%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thumimku · 2024-11-29T04:29:59Z

...a/org/wso2/carbon/identity/api/resource/mgt/dao/impl/AuthorizationDetailsTypeMgtDAOImpl.java

+ */
+public class AuthorizationDetailsTypeMgtDAOImpl implements AuthorizationDetailsTypeMgtDAO {
+
+    private static final Log log = LogFactory.getLog(AuthorizationDetailsTypeMgtDAOImpl.class);


Suggested change

private static final Log log = LogFactory.getLog(AuthorizationDetailsTypeMgtDAOImpl.class);

private static final Log LOG = LogFactory.getLog(AuthorizationDetailsTypeMgtDAOImpl.class);

please change other places also

Fixed in 9f2011a

Thumimku · 2024-11-29T04:35:44Z

features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/dbscripts/db2.sql

+    ID CHAR(36) NOT NULL,
+    TYPE VARCHAR(255) NOT NULL,
+    API_ID CHAR(36) NOT NULL,
+    CURSOR_KEY INTEGER NOT NULL,


Why we need this cursor key

This field can be used in the "cursor pagination" which is more efficient than "limit/offset pagination"

Capitalize private static final variable names

...main/java/org/wso2/carbon/identity/api/resource/mgt/AuthorizationDetailsTypeManagerImpl.java

...a/org/wso2/carbon/identity/api/resource/mgt/dao/impl/AuthorizationDetailsTypeMgtDAOImpl.java

shashimalcse · 2024-12-10T05:20:21Z

...rg/wso2/carbon/identity/api/resource/mgt/internal/APIResourceManagementServiceComponent.java

+    private boolean isRichAuthorizationRequestsEnabled() {
+
+        for (final String tableName : AuthorizationDetailsTypesUtil.RICH_AUTHORIZATION_REQUESTS_TABLES) {
+            if (!IdentityDatabaseUtil.isTableExists(tableName)) {


don't we have a configuration to enable this feature? Why are we relying on the table exists?

This has added to prevent database exceptions such as "Table does not exist"

We decided to not use isTableExist checks in the product due to schema owner issues in oracle DB. Shall we use a configuration and properly document the need of necessary tables before enabling the configuration.

@madurangasiriwardena instead of using

isTableExists

method can we have a configuration.
@VimukthiRajapaksha correct me if I am wrong, this config is needed to seperate the feature for any specific deployments right?

Yes @Thumimku we have added this to avoid triggering database operations for this feature in a specific deployment.

Since this is a feature going in a product release, all the required new database tables are supposed to be added with the product upgrade procedure. So there is no requirement to avoid "Table does not exist" exceptions.

If there are no other reasons to enable/disable the feature, we can get rid of this altogether.

@VimukthiRajapaksha Let's address this, and then we can proceed.

...c/main/java/org/wso2/carbon/identity/application/mgt/AuthorizedAPIManagementServiceImpl.java

...gt/src/main/java/org/wso2/carbon/identity/application/mgt/dao/impl/AuthorizedAPIDAOImpl.java

...urce.mgt/src/main/java/org/wso2/carbon/identity/api/resource/mgt/APIResourceManagerImpl.java

...src/main/java/org/wso2/carbon/identity/api/resource/mgt/AuthorizationDetailsTypeManager.java

make db operations transactional Add authorization details types to authorized api events Add missing java doc comments remove static method imports

resolve checkstyle issues

sonarqubecloud · 2024-12-18T07:17:48Z

Quality Gate passed

Issues
14 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.1% Duplication on New Code

See analysis details on SonarQube Cloud

VimukthiRajapaksha added 9 commits August 23, 2024 10:15

Add CRUD operations for Authorization Details Types

604f4ee

Add authorization details types feature

5391ce2

Add rich authorization details types management feature

8b1ea3d

- Add SQL queries - Add DAO layer - Modify resource access controls - Add unit tests - Modify APIs to support authorization details types

Accept schema as a map

07d7516

Resolve merge conflicts

9e56e30

Change SCHEMA_JSON data type to CLOB

514f0a3

Merge branch 'master' into master_rar

ad7ee7f

Add missing method comments

0c4b70e

Merge branch 'master' into master_rar

419a050

Resolve sonarcloud issues

498f0de

VimukthiRajapaksha force-pushed the master_rar branch from 3688d64 to 498f0de Compare October 28, 2024 08:06

Merge branch 'master' into master_rar

bd42eb2

VimukthiRajapaksha added 4 commits October 28, 2024 14:05

Resolve sonarcloud issues

9d933ed

Add a config method to check if RAR is enabled

507a2ca

Merge remote-tracking branch 'origin/master' into master_rar

f1558ea

Resolve sonarcloud issues

cccb748

Thumimku reviewed Nov 29, 2024

View reviewed changes

VimukthiRajapaksha added 2 commits December 9, 2024 10:18

Fix commented issues

9f2011a

Capitalize private static final variable names

resolve merge conflicts

c85153d