Skip to content

Conversation

@imesh94
Copy link
Contributor

@imesh94 imesh94 commented Dec 19, 2025

[OB4] Fix IS application authorization flow failure

This PR addresses a failure in the authorization flow when using Service Providers created via the WSO2 IS Management Console.
In an OB4 setup, the authorization engine expects a regulatory property in the SP_METADATA table. However, applications created through the standard IS Console do not have this property populated by default, leading to validation failures.

Issue link: #873

Doc Issue: Optional, link issue from documentation repository

Applicable Labels: Spec, product, version, type (specify requested labels)


Development Checklist

  1. Build complete solution with pull request in place.
  2. Ran checkstyle plugin with pull request in place.
  3. Ran Findbugs plugin with pull request in place.
  4. Ran FindSecurityBugs plugin and verified report.
  5. Formatted code according to WSO2 code style.
  6. Have you verified the PR doesn't commit any keys, passwords, tokens, usernames, or other secrets?
  7. Migration scripts written (if applicable).
  8. Have you followed secure coding standards in WSO2 Secure Engineering Guidelines?

Testing Checklist

  1. Written unit tests.
  2. Verified tests in multiple database environments (if applicable).
  3. Tested with BI enabled (if applicable).

Summary by CodeRabbit

  • Bug Fixes
    • Improved system resilience by enhancing error handling when regulatory configuration is unavailable, allowing continued operation instead of application failure.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 19, 2025

Walkthrough

A single method in IdentityCommonUtils.java has been updated to handle missing regulatory properties differently. When a regulatory property cannot be retrieved from service provider metadata, the method now logs a debug message and returns false instead of throwing an exception.

Changes

Cohort / File(s) Summary
Error Handling Refinement
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/util/IdentityCommonUtils.java
Modified isRegulatoryApp method to log a debug message and return false when regulatory property is null, rather than throwing a FinancialServicesException. Changes error handling from exception-based to default non-regulatory outcome.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Verify that returning false on null regulatory property is the correct semantic behavior for the use case
  • Confirm debug logging is appropriately gated and provides sufficient diagnostic information
  • Check that existing callers of isRegulatoryApp handle the false return value as expected

Poem

🐰 A graceful hop through error's way,
Where exceptions softly fade away,
Log and return, a gentle hand,
False by default—much better planned! 🌙

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description check ⚠️ Warning PR description covers the main issue and includes development/testing checklists, but lacks required sections and detailed test documentation. Add missing required sections: Secure Development Checklist (FindSecurityBugs, Dependency-check plugins), Automation Test Details table with test IDs, and Conformance Tests Details table. Mark completed checklist items with [x].
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title '[OB4] Fix IS application authorization flow failure' directly and clearly summarizes the main change: fixing an authorization flow failure in OB4 when using IS Service Providers.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@wso2-engineering wso2-engineering bot left a comment

Choose a reason for hiding this comment

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

AI Agent Log Improvement Checklist

⚠️ Warning: AI-Generated Review Comments

  • The log-related comments and suggestions in this review were generated by an AI tool to assist with identifying potential improvements. Purpose of reviewing the code for log improvements is to improve the troubleshooting capabilities of our products.
  • Please make sure to manually review and validate all suggestions before applying any changes. Not every code suggestion would make sense or add value to our purpose. Therefore, you have the freedom to decide which of the suggestions are helpful.

✅ Before merging this pull request:

  • Review all AI-generated comments for accuracy and relevance.
  • Complete and verify the table below. We need your feedback to measure the accuracy of these suggestions and the value they add. If you are rejecting a certain code suggestion, please mention the reason briefly in the suggestion for us to capture it.
Comment Accepted (Y/N) Reason
#### Log Improvement Suggestion No: 1
#### Log Improvement Suggestion No: 2

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/util/IdentityCommonUtils.java (1)

381-385: Consider elevating the log level for better observability in production.

The current implementation logs at debug level when the regulatory property is missing. Since this represents a significant control-flow decision that affects regulatory compliance checks, consider using log.info() or log.warn() instead. Debug logs are typically disabled in production environments, which could make it difficult to audit why certain applications were treated as non-regulatory.

🔎 Proposed fix to use info-level logging
-            } else {
-                if (log.isDebugEnabled()) {
-                    log.debug("Regulatory property not found in service provider metadata for clientId: "
-                            + clientId + ". Hence treating it as a non-regulatory application.");
-                }
-                return false;
+            } else {
+                log.info("Regulatory property not found in service provider metadata for clientId: "
+                        + clientId + ". Hence treating it as a non-regulatory application.");
+                return false;
📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fc7ff9f and 9696963.

📒 Files selected for processing (1)
  • financial-services-accelerator/components/org.wso2.financial.services.accelerator.identity.extensions/src/main/java/org/wso2/financial/services/accelerator/identity/extensions/util/IdentityCommonUtils.java (1 hunks)

@Ashi1993 Ashi1993 merged commit 820ba3c into wso2:main Dec 20, 2025
3 checks passed
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.

4 participants