Add configs related to consent mgt webhook#8159
Open
hwupathum wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request introduces configuration-driven channel filtering for event profiles and registers consent-related event hooks with corresponding webhook event channels. First, it implements infrastructure to disable specific event channels during profile loading via a DAO filter and configuration entries. Second, it registers ChangesEvent Channel Disabling Infrastructure
Consent Event Hooks and Webhook Channels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.properties`:
- Around line 198-204: The properties file has module indices that are out of
sync with the JSON configuration file. The ConsentEventHook and
ConsentPurposeEventHook modules are currently at indices 63 and 64 in the
properties file but should be at indices 71 and 72 to match the JSON
configuration. Add the missing module.name entries for indices 63 through 70 to
the properties file before the ConsentEventHook entry, corresponding to the
OrganizationAgentSharingHandler and moesif-related modules that exist in the
JSON file. This will shift ConsentEventHook and ConsentPurposeEventHook to the
correct indices (71 and 72) and synchronize the module numbering between both
configuration files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 404e58a4-4779-4422-8f47-9761bf882f73
📒 Files selected for processing (3)
features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.propertiesfeatures/identity-event/org.wso2.carbon.identity.event.server.feature/resources/org.wso2.carbon.identity.event.server.feature.default.jsonfeatures/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json
9 tasks
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (37.77%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #8159 +/- ##
============================================
- Coverage 52.97% 52.94% -0.03%
- Complexity 20994 21001 +7
============================================
Files 2197 2197
Lines 129323 129269 -54
Branches 19265 19245 -20
============================================
- Hits 68504 68443 -61
- Misses 52473 52479 +6
- Partials 8346 8347 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
hwupathum
force-pushed
the
consent-webhook
branch
from
ecc40e5 to
d0d35b7
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
features/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json (1)
155-157: ⚡ Quick winSemantic confusion: "Consent authorized" event description is unclear.
The event name "Consent authorized" typically implies approval, but the description states "Notify if a user consent is approved, rejected, or revoked." This creates semantic confusion because:
- "Authorized" doesn't naturally convey rejection or revocation
- Event consumers may misinterpret what this event represents
If this event is meant to cover multiple authorization state changes, consider either:
- Using a more neutral name like "Consent authorization changed" or "Consent status updated"
- Splitting into separate events: "Consent approved", "Consent rejected", "Consent revoked"
- Clarifying in documentation that "authorized" is a catch-all term
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json` around lines 155 - 157, The event name "Consent authorized" in the wso2-event-profile.json file is semantically misaligned with its eventDescription, which indicates the event covers approval, rejection, and revocation states. Either rename the eventName to a more neutral term like "Consent authorization changed" or "Consent status updated" that accurately reflects all covered states, or split this into separate events for each state. Ensure the eventName, eventDescription, and eventUri fields are consistent with the chosen approach to avoid confusing event consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@features/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json`:
- Around line 155-157: The event name "Consent authorized" in the
wso2-event-profile.json file is semantically misaligned with its
eventDescription, which indicates the event covers approval, rejection, and
revocation states. Either rename the eventName to a more neutral term like
"Consent authorization changed" or "Consent status updated" that accurately
reflects all covered states, or split this into separate events for each state.
Ensure the eventName, eventDescription, and eventUri fields are consistent with
the chosen approach to avoid confusing event consumers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7676b585-ba70-4ef5-b9d0-0e1b33192865
📒 Files selected for processing (3)
features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.propertiesfeatures/identity-event/org.wso2.carbon.identity.event.server.feature/resources/org.wso2.carbon.identity.event.server.feature.default.jsonfeatures/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json
🚧 Files skipped from review as they are similar to previous changes (2)
- features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/org.wso2.carbon.identity.event.server.feature.default.json
- features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.properties
hwupathum
force-pushed
the
consent-webhook
branch
from
d0d35b7 to
3804871
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.java`:
- Around line 136-138: The DEBUG log statement performing string concatenation
with "Skipping disabled event profile: " + profile.getProfile() is not guarded
by an isDebugEnabled() check, causing unnecessary string concatenation overhead
when DEBUG logging is disabled. Wrap the log.debug call in an if
(log.isDebugEnabled()) guard to prevent the concatenation from executing when
DEBUG level is not enabled, following the coding guidelines for
performance-sensitive logging.
In
`@features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/org.wso2.carbon.identity.event.server.feature.default.json`:
- Around line 471-477: The ConsentEventHook and ConsentPurposeEventHook hooks
are set to enabled (true) in the JSON file at their respective properties.enable
settings, but this contradicts the disabled default state defined in the paired
identity-event.properties module configuration. To maintain consistency with the
module-registration contract and prevent unintended hook activation, change the
enable values from true to false for both ConsentEventHook.properties.enable and
ConsentPurposeEventHook.properties.enable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 17782d09-a666-403c-b042-0cbebfa393c2
📒 Files selected for processing (6)
components/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.jsonfeatures/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.propertiesfeatures/identity-event/org.wso2.carbon.identity.event.server.feature/resources/org.wso2.carbon.identity.event.server.feature.default.jsonfeatures/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json
🚧 Files skipped from review as they are similar to previous changes (2)
- features/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json
- features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.properties
hwupathum
force-pushed
the
consent-webhook
branch
from
3804871 to
adabebe
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.java (1)
137-142: ⚡ Quick winAdd a DEBUG log when channels are filtered out by configuration.
This branch introduces a key behavior change (profile channels are removed), but it is silent today. A guarded DEBUG log with safe identifiers and removed-count will make config troubleshooting much easier.
Suggested fix
if (!disabledChannels.isEmpty() && profile.getChannels() != null) { + int originalCount = profile.getChannels().size(); List<Channel> filteredChannels = profile.getChannels().stream() .filter(channel -> !disabledChannels.contains(channel.getUri())) .collect(Collectors.toList()); + if (log.isDebugEnabled() && originalCount != filteredChannels.size()) { + log.debug("Filtered disabled channels for profile: " + profile.getProfile() + + ", removed count: " + (originalCount - filteredChannels.size())); + } profile = new EventProfile(profile.getProfile(), profile.getUri(), filteredChannels); }As per coding guidelines, logs should cover key business decisions, and DEBUG logs with runtime string building should be guarded.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.java` around lines 137 - 142, The code block filters channels from a profile based on disabledChannels configuration but does not log this important behavioral change. Add a guarded DEBUG log statement after the profile filtering logic in the if block that checks if DEBUG logging is enabled, then logs a message containing the profile URI (as a safe identifier) and the count of removed channels (calculated as the original channel count minus the filtered channel count). This will help with configuration troubleshooting while following the coding guideline that DEBUG logs with runtime string building should be guarded.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.java`:
- Around line 138-140: In the FileBasedEventProfileMetadataDAOImpl file where
the profile.getChannels() stream is filtered, add a null check in the filter
predicate to exclude null channel entries before dereferencing channel.getUri().
Modify the filter condition to first check that the channel is not null, then
verify that the channel's URI is not in the disabledChannels list to prevent
NullPointerException during profile loading.
In
`@features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.infer.json`:
- Around line 446-455: The IS_7.3.0 configuration block is currently at the
wrong nesting level in the JSON structure. Move the entire IS_7.3.0 object
(containing consent_mgt.enable_v2_api, identity_mgt.events.schemes
configurations, and webhooks.event_profiles.disabled_channels) inside the
preserve_previous_product_behaviour.version object where it belongs, ensuring it
closes the version object properly rather than appearing as a sibling top-level
key. This will ensure the version-specific overrides are correctly applied as
part of the version preservation configuration.
---
Nitpick comments:
In
`@components/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.java`:
- Around line 137-142: The code block filters channels from a profile based on
disabledChannels configuration but does not log this important behavioral
change. Add a guarded DEBUG log statement after the profile filtering logic in
the if block that checks if DEBUG logging is enabled, then logs a message
containing the profile URI (as a safe identifier) and the count of removed
channels (calculated as the original channel count minus the filtered channel
count). This will help with configuration troubleshooting while following the
coding guideline that DEBUG logs with runtime string building should be guarded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 739a78e2-4007-4bcd-a3a3-3dc75cb8f477
📒 Files selected for processing (7)
components/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.jsonfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.infer.jsonfeatures/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.propertiesfeatures/identity-event/org.wso2.carbon.identity.event.server.feature/resources/org.wso2.carbon.identity.event.server.feature.default.jsonfeatures/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json
🚧 Files skipped from review as they are similar to previous changes (2)
- features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/org.wso2.carbon.identity.event.server.feature.default.json
- features/webhook-mgt/org.wso2.carbon.identity.webhook.management.server.feature/resources/identity/eventprofiles/wso2-event-profile.json
hwupathum
force-pushed
the
consent-webhook
branch
from
3d0881b to
fd21a8c
Compare
hwupathum
force-pushed
the
consent-webhook
branch
2 times, most recently
from
a98850b to
ef6fd45
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/consent/PolicyConsentPostAuthnHandler.java (1)
419-424: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the consent-app mapping lookup failure before wrapping it.
This catch block converts a data-access failure into
PostAuthenticationFailedExceptionwithout a framework log. Add a safe WARN log with identifiers and the error message only.As per path instructions, "Suggest log statements at error handling boundaries (catch blocks, error returns)."
Proposed log
} catch (ConsentAppMappingException e) { + if (LOG.isWarnEnabled()) { + LOG.warn(String.format("Error retrieving policy config mappings for application: %s in tenant: %s. " + + "Error: %s.", appResourceId, context.getTenantDomain(), e.getMessage())); + } throw new PostAuthenticationFailedException( ErrorMessages.ERROR_WHILE_PROCESSING_POLICY_CONSENT.getCode(), String.format("Error retrieving policy config mappings for application: %s in tenant: %s.",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/consent/PolicyConsentPostAuthnHandler.java` around lines 419 - 424, The ConsentAppMappingException catch in PolicyConsentPostAuthnHandler should emit a safe WARN log before throwing PostAuthenticationFailedException. Add a warning in the catch block that includes the relevant identifiers already available there, such as appResourceId and context.getTenantDomain(), and log only the exception message (not the stack trace) before wrapping and rethrowing the failure.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/consent/PolicyConsentPostAuthnHandler.java`:
- Around line 346-356: Normalize the upstream persona before constructing the
consent flow in PolicyConsentPostAuthnHandler: when reading the current flow
from IdentityContext and its initiating persona, map unsupported values like
SYSTEM to a valid persona (for CONSENT_ADD this should fall back to APPLICATION
or another supported persona) before calling Flow.Builder.build(). Update the
consent-flow creation logic so Flow.Builder.validate() never sees an invalid
persona and the consent error handling path remains reachable.
In
`@components/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Flow.java`:
- Line 122: The divider comment in Flow’s consent management section does not
follow the Java comment guideline. Update the section comments around the
consent flow markers so they start with a space, begin with a capitalized word,
and end with a period, keeping the same intent while making the comment style
consistent with the rest of the class.
---
Nitpick comments:
In
`@components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/consent/PolicyConsentPostAuthnHandler.java`:
- Around line 419-424: The ConsentAppMappingException catch in
PolicyConsentPostAuthnHandler should emit a safe WARN log before throwing
PostAuthenticationFailedException. Add a warning in the catch block that
includes the relevant identifiers already available there, such as appResourceId
and context.getTenantDomain(), and log only the exception message (not the stack
trace) before wrapping and rethrowing the failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: afc9361c-505f-4368-854e-d3a818f74be7
📒 Files selected for processing (8)
components/authentication-framework/org.wso2.carbon.identity.application.authentication.framework/src/main/java/org/wso2/carbon/identity/application/authentication/framework/handler/request/impl/consent/PolicyConsentPostAuthnHandler.javacomponents/identity-core/org.wso2.carbon.identity.core/src/main/java/org/wso2/carbon/identity/core/context/model/Flow.javacomponents/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.javafeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.jsonfeatures/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.infer.jsonfeatures/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.propertiesfeatures/identity-event/org.wso2.carbon.identity.event.server.feature/resources/org.wso2.carbon.identity.event.server.feature.default.json
🚧 Files skipped from review as they are similar to previous changes (5)
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.infer.json
- features/identity-event/org.wso2.carbon.identity.event.server.feature/resources/identity-event.properties
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/org.wso2.carbon.identity.core.server.feature.default.json
- components/webhook-mgt/org.wso2.carbon.identity.webhook.metadata/src/main/java/org/wso2/carbon/identity/webhook/metadata/internal/dao/impl/FileBasedEventProfileMetadataDAOImpl.java
- features/identity-core/org.wso2.carbon.identity.core.server.feature/resources/identity.xml.j2
hwupathum
force-pushed
the
consent-webhook
branch
from
ef6fd45 to
227d387
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes in this pull request
Add webhook event support for consent and consent purpose events.
WSO2ConsentPurposeVersionAddedEventPayload) and supporting model classes (Consent, ConsentElement, ConsentPurpose).
identity-event.properties.
Dependent changes required in carbon-identity-framework:
When should this PR be merged
After the dependent carbon-identity-framework PR (adding the identity-event.properties and default.json entries) is merged.
Follow up actions
Checklist (for reviewing)
General
Functionality
that are not handled.
Code
Tests
Security
Documentation