Add getFederatedAssociations function #531
Conversation
|
Warning Rate limit exceeded@mpmadhavig has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
WalkthroughA new OSGi bundle module (adaptive-authentication/org.wso2.custom.auth.functions.v2) providing a federated-associations retrieval function is added; CI workflow Changes
Sequence DiagramsequenceDiagram
participant OSGi as OSGi Container
participant Comp as CustomAuthFuncComponent
participant Holder as CustomAuthFuncHolder
participant Registry as JsFunctionRegistry
participant Impl as GetFederatedAssociationsFunctionImpl
participant Manager as FederatedAssociationManager
OSGi->>Comp: activate(ComponentContext)
note right of Comp `#D6EAF8`: bind services\n(setRealmService, setRegistryService,\nsetJsFunctionRegistry, setFederatedAssociationManager)
Comp->>Holder: set* services
Comp->>Registry: register("getFederatedAssociations")
Note over Impl,Registry: Runtime JS invokes registered function
Registry->>Impl: invoke getFederatedAssociations(context, loginIdentifier)
Impl->>Holder: getFederatedAssociationManager()
Impl->>Manager: getFederatedAssociationsOfUser(tenantId, domain, username)
Manager-->>Impl: List<AssociatedAccountDTO>
Impl-->>Impl: map to List<String> provider names
Impl-->>Registry: return List<String>
OSGi->>Comp: deactivate(ComponentContext)
Comp->>Registry: unregister("getFederatedAssociations")
Comp->>Holder: unset services
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
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}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/GetFederatedAssociationsFunctionImpl.java (2)
48-60: Simplify userDomain initialization.The
userDomainvariable is initialized to"PRIMARY"on line 48 but is always reassigned in both branches of the conditional (line 54 whenloginIdentifiercontains "/", and line 59 when it's null). The initial assignment is redundant.Apply this diff:
String tenantDomain = context.getWrapped().getTenantDomain(); int tenantId = IdentityTenantUtil.getTenantId(tenantDomain); - String userDomain = "PRIMARY"; + String userDomain; String username; if (loginIdentifier != null) { username = loginIdentifier; if (loginIdentifier.contains("/")) { String[] parts = loginIdentifier.split("/", 2); userDomain = parts[0]; username = parts[1]; + } else { + userDomain = "PRIMARY"; } } else { username = context.getWrapped().getSubject().getUserName(); userDomain = context.getWrapped().getSubject().getUserStoreDomain(); }
63-65: Improve error visibility in logs.The error log message is generic and doesn't include the username or tenant context, making debugging difficult. Consider including these details in the error message.
Apply this diff:
} catch (FederatedAssociationManagerException e) { - LOG.error("Error while retrieving federated associations.", e); + LOG.error(String.format("Error while retrieving federated associations for user '%s' in domain '%s' " + + "and tenant '%s'.", username, userDomain, tenantDomain), e); }adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncComponent.java (1)
112-120: Consider thread safety for static field access.The static field
jsFunctionRegistryis assigned from instance methods without synchronization. While OSGi typically manages component lifecycle in a single thread, addingvolatileor synchronization would make the intent explicit and prevent potential issues.Apply this diff:
private static final Log LOG = LogFactory.getLog(CustomAuthFuncComponent.class); - private static JsFunctionRegistry jsFunctionRegistry; + private static volatile JsFunctionRegistry jsFunctionRegistry;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/pr-builder.yml(1 hunks)adaptive-authentication/org.wso2.custom.auth.functions.v2/README.md(1 hunks)adaptive-authentication/org.wso2.custom.auth.functions.v2/pom.xml(1 hunks)adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/GetFederatedAssociationsFunction.java(1 hunks)adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/GetFederatedAssociationsFunctionImpl.java(1 hunks)adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncComponent.java(1 hunks)adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncHolder.java(1 hunks)pom.xml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/GetFederatedAssociationsFunctionImpl.java (1)
adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncHolder.java (1)
CustomAuthFuncHolder(26-84)
adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncComponent.java (2)
adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/GetFederatedAssociationsFunctionImpl.java (1)
GetFederatedAssociationsFunctionImpl(36-73)adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncHolder.java (1)
CustomAuthFuncHolder(26-84)
🪛 markdownlint-cli2 (0.18.1)
adaptive-authentication/org.wso2.custom.auth.functions.v2/README.md
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
pom.xml (1)
72-72: LGTM!The new module is correctly added to the build.
.github/workflows/pr-builder.yml (1)
21-40: LGTM!GitHub Actions tooling is correctly upgraded to v4, and Java 11 aligns with the requirements of the new module.
| <dependencies> | ||
| <dependency> | ||
| <groupId>org.wso2.carbon.identity.framework</groupId> | ||
| <artifactId>org.wso2.carbon.identity.application.authentication.framework</artifactId> | ||
| <version>${identity.framework.version}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.wso2.carbon</groupId> | ||
| <artifactId>org.wso2.carbon.utils</artifactId> | ||
| <version>${carbon.kernel.version}</version> | ||
| <!-- <scope>provided</scope>--> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.graalvm.sdk</groupId> | ||
| <artifactId>graal-sdk</artifactId> | ||
| <version>${graalvm.version}</version> | ||
| <scope>provided</scope> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>org.wso2.carbon.identity.framework</groupId> | ||
| <artifactId>org.wso2.carbon.identity.application.authentication.framework</artifactId> | ||
| </dependency> | ||
| </dependencies> |
There was a problem hiding this comment.
Remove duplicate dependency declaration.
The dependency on org.wso2.carbon.identity.application.authentication.framework is declared twice (lines 19-23 and 36-39), with the second declaration missing a version.
Apply this diff:
<dependencies>
<dependency>
<groupId>org.wso2.carbon.identity.framework</groupId>
<artifactId>org.wso2.carbon.identity.application.authentication.framework</artifactId>
<version>${identity.framework.version}</version>
</dependency>
<dependency>
<groupId>org.wso2.carbon</groupId>
<artifactId>org.wso2.carbon.utils</artifactId>
<version>${carbon.kernel.version}</version>
- <!-- <scope>provided</scope>-->
</dependency>
<dependency>
<groupId>org.graalvm.sdk</groupId>
<artifactId>graal-sdk</artifactId>
<version>${graalvm.version}</version>
<scope>provided</scope>
</dependency>
- <dependency>
- <groupId>org.wso2.carbon.identity.framework</groupId>
- <artifactId>org.wso2.carbon.identity.application.authentication.framework</artifactId>
- </dependency>
</dependencies>🤖 Prompt for AI Agents
adaptive-authentication/org.wso2.custom.auth.functions.v2/pom.xml lines 18-40:
there is a duplicate dependency for
org.wso2.carbon.identity.application.authentication.framework (one with version
at lines ~19-23 and a second missing version at lines ~36-39); remove the second
duplicate entry (the one without a version) so the dependency is declared only
once with its version, or if you intended a different scope/version, replace the
duplicate with the correct values instead of duplicating the same artifact.
| # wso2-custom-adaptive-auth-function | ||
|
|
||
| *Steps to deploy* | ||
| - Build the sample using maven `mvn clean install` | ||
| - Copy the `org.wso2.custom.auth.functions-1.0.0` binary file from `target` directory into | ||
| `<IS_HOME>/repository/components/dropins` directory | ||
| - Restart WSO2 IS | ||
|
|
||
| **getUsernameFromContext()** | ||
|
|
||
| This custom function can be used to retrieve the username from the authentication context. | ||
|
|
||
| Example usage. | ||
| ``` | ||
| var onLoginRequest = function(context) { | ||
| executeStep(1, { | ||
| onSuccess: function (context) { | ||
| var userName = getUsernameFromContext(context, 1); | ||
| Log.info("Username: " + userName); | ||
| } | ||
| }); | ||
| }; | ||
| ``` |
There was a problem hiding this comment.
Documentation does not match the implementation.
The README documents getUsernameFromContext() function, but this module (v2) is intended to provide getFederatedAssociations() based on the PR title and implementation. Additionally, line 5 references the v1 artifact name org.wso2.custom.auth.functions-1.0.0, which should be org.wso2.custom.auth.functions.v2-4.6.4-SNAPSHOT for this module.
Update the README to document the correct function and artifact:
-# wso2-custom-adaptive-auth-function
+# wso2-custom-adaptive-auth-function v2
*Steps to deploy*
- Build the sample using maven `mvn clean install`
-- Copy the `org.wso2.custom.auth.functions-1.0.0` binary file from `target` directory into
+- Copy the `org.wso2.custom.auth.functions.v2-4.6.4-SNAPSHOT.jar` binary file from `target` directory into
`<IS_HOME>/repository/components/dropins` directory
- Restart WSO2 IS
-**getUsernameFromContext()**
+**getFederatedAssociations()**
-This custom function can be used to retrieve the username from the authentication context.
+This custom function can be used to retrieve federated identity provider associations for a user.
Example usage.
-```
+```javascript
var onLoginRequest = function(context) {
executeStep(1, {
onSuccess: function (context) {
- var userName = getUsernameFromContext(context, 1);
- Log.info("Username: " + userName);
+ var federatedIdPs = getFederatedAssociations(context, null);
+ Log.info("Federated Identity Providers: " + federatedIdPs);
}
});
};
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
3-3: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
---
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
---
14-14: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
adaptive-authentication/org.wso2.custom.auth.functions.v2/README.md lines 1-23:
The README currently documents getUsernameFromContext and references the v1
artifact name; update it to document getFederatedAssociations and correct the
artifact identifier to org.wso2.custom.auth.functions.v2-4.6.4-SNAPSHOT. Replace
the function description and example block to show using
getFederatedAssociations(context, null) and logging the returned federated IdP
list, and change the deployment step artifact name to
org.wso2.custom.auth.functions.v2-4.6.4-SNAPSHOT (and any other v1 references)
so the README matches this v2 module implementation.
</details>
<!-- This is an auto-generated comment by CodeRabbit -->
| /** | ||
| * Retrieve the federated associations for a given user. | ||
| * | ||
| * @param context JsAuthenticationContext instance. | ||
| * @param loginIdentifier Optional login identifier of the user. | ||
| * @return List of JSAssociatedAccount instances representing federated associations. | ||
| */ | ||
| List<String> getFederatedAssociations(JsAuthenticationContext context, String loginIdentifier); |
There was a problem hiding this comment.
Fix inaccurate Javadoc.
The Javadoc states the method returns "List of JSAssociatedAccount instances" but the actual return type is List<String> containing identity provider names (as seen in the implementation).
Apply this diff:
/**
* Retrieve the federated associations for a given user.
*
* @param context JsAuthenticationContext instance.
* @param loginIdentifier Optional login identifier of the user.
- * @return List of JSAssociatedAccount instances representing federated associations.
+ * @return List of identity provider names representing federated associations.
*/
List<String> getFederatedAssociations(JsAuthenticationContext context, String loginIdentifier);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Retrieve the federated associations for a given user. | |
| * | |
| * @param context JsAuthenticationContext instance. | |
| * @param loginIdentifier Optional login identifier of the user. | |
| * @return List of JSAssociatedAccount instances representing federated associations. | |
| */ | |
| List<String> getFederatedAssociations(JsAuthenticationContext context, String loginIdentifier); | |
| /** | |
| * Retrieve the federated associations for a given user. | |
| * | |
| * @param context JsAuthenticationContext instance. | |
| * @param loginIdentifier Optional login identifier of the user. | |
| * @return List of identity provider names representing federated associations. | |
| */ | |
| List<String> getFederatedAssociations(JsAuthenticationContext context, String loginIdentifier); |
🤖 Prompt for AI Agents
In
adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/GetFederatedAssociationsFunction.java
around lines 31 to 38, the Javadoc incorrectly says the method returns "List of
JSAssociatedAccount instances" while the signature returns List<String>; update
the Javadoc @return to state that the method returns a List<String> of identity
provider names (or federated association identifiers) and adjust wording to
match the actual return type (no code changes required).
| federatedAssociations = CustomAuthFuncHolder.getInstance().getFederatedAssociationManager() | ||
| .getFederatedAssociationsOfUser(tenantId, userDomain, username); | ||
| } catch (FederatedAssociationManagerException e) { | ||
| LOG.error("Error while retrieving federated associations.", e); | ||
| } |
There was a problem hiding this comment.
Add null safety check for FederatedAssociationManager.
The code assumes CustomAuthFuncHolder.getInstance().getFederatedAssociationManager() returns a non-null value. If the service is not yet bound or has been unset, this will cause a NullPointerException.
Apply this diff:
+ FederatedAssociationManager federatedAssociationManager =
+ CustomAuthFuncHolder.getInstance().getFederatedAssociationManager();
+ if (federatedAssociationManager == null) {
+ LOG.error("FederatedAssociationManager service is not available.");
+ return new ArrayList<>();
+ }
- federatedAssociations = CustomAuthFuncHolder.getInstance().getFederatedAssociationManager()
+ federatedAssociations = federatedAssociationManager
.getFederatedAssociationsOfUser(tenantId, userDomain, username);
} catch (FederatedAssociationManagerException e) {
LOG.error("Error while retrieving federated associations.", e);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| federatedAssociations = CustomAuthFuncHolder.getInstance().getFederatedAssociationManager() | |
| .getFederatedAssociationsOfUser(tenantId, userDomain, username); | |
| } catch (FederatedAssociationManagerException e) { | |
| LOG.error("Error while retrieving federated associations.", e); | |
| } | |
| FederatedAssociationManager federatedAssociationManager = | |
| CustomAuthFuncHolder.getInstance().getFederatedAssociationManager(); | |
| if (federatedAssociationManager == null) { | |
| LOG.error("FederatedAssociationManager service is not available."); | |
| return new ArrayList<>(); | |
| } | |
| federatedAssociations = federatedAssociationManager | |
| .getFederatedAssociationsOfUser(tenantId, userDomain, username); | |
| } catch (FederatedAssociationManagerException e) { | |
| LOG.error("Error while retrieving federated associations.", e); | |
| } |
...ons.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncComponent.java
Show resolved
Hide resolved
| private static JsFunctionRegistry jsFunctionRegistry; | ||
|
|
||
| @Activate | ||
| protected void activate(ComponentContext ctxt) { | ||
|
|
||
| jsFunctionRegistry.register(JsFunctionRegistry.Subsystem.SEQUENCE_HANDLER, "getFederatedAssociations", | ||
| new GetFederatedAssociationsFunctionImpl()); |
There was a problem hiding this comment.
Add null safety check for jsFunctionRegistry in activate.
The activate method uses jsFunctionRegistry directly without checking if it's null. If the service binding hasn't completed before activation, this will cause a NullPointerException.
Apply this diff:
@Activate
protected void activate(ComponentContext ctxt) {
+ if (jsFunctionRegistry == null) {
+ LOG.error("Cannot activate custom auth functions: JsFunctionRegistry is not available.");
+ return;
+ }
jsFunctionRegistry.register(JsFunctionRegistry.Subsystem.SEQUENCE_HANDLER, "getFederatedAssociations",
new GetFederatedAssociationsFunctionImpl());
}🤖 Prompt for AI Agents
In
adaptive-authentication/org.wso2.custom.auth.functions.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncComponent.java
around lines 43 to 49, the activate method invokes
jsFunctionRegistry.register(...) without checking if jsFunctionRegistry is null
which can cause a NullPointerException; update the activate method to first
check if jsFunctionRegistry is null and if so log a clear warning/error and
return (or throw a controlled IllegalStateException) instead of proceeding,
otherwise perform the register call; ensure the log message identifies the
component and that the guard prevents registration when the service binding has
not completed.
...ctions.v2/src/main/java/org/wso2/custom/auth/functions/v2/internal/CustomAuthFuncHolder.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
adaptive-authentication/org.wso2.custom.auth.functions.v2/pom.xml (1)
35-38: Remove duplicate dependency declaration.The dependency on
org.wso2.carbon.identity.application.authentication.frameworkis declared twice (lines 19-22 with version, and lines 35-38 without). The second occurrence lacks a version and should be removed.Apply this diff:
<dependency> <groupId>org.graalvm.sdk</groupId> <artifactId>graal-sdk</artifactId> <version>${graalvm.version}</version> <scope>provided</scope> </dependency> - <dependency> - <groupId>org.wso2.carbon.identity.framework</groupId> - <artifactId>org.wso2.carbon.identity.application.authentication.framework</artifactId> - </dependency> </dependencies>
🧹 Nitpick comments (1)
adaptive-authentication/org.wso2.custom.auth.functions.v2/pom.xml (1)
23-28: Remove commented-out scope declaration.Line 27 contains a commented-out
<scope>provided</scope>that should be removed as cleanup. If this scope was intentionally omitted, consider documenting why via a comment.<dependency> <groupId>org.wso2.carbon</groupId> <artifactId>org.wso2.carbon.utils</artifactId> <version>${carbon.kernel.version}</version> - <!-- <scope>provided</scope>--> </dependency>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
adaptive-authentication/org.wso2.custom.auth.functions.v2/pom.xml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
adaptive-authentication/org.wso2.custom.auth.functions.v2/pom.xml (3)
72-112: OSGi bundle configuration looks good.The bundle's
Private-PackageandExport-Packagedirectives correctly referenceorg.wso2.custom.auth.functions.v2.internalandorg.wso2.custom.auth.functions.v2. Import and dynamic import packages are well-defined for framework, carbon, and GraalVM dependencies.
41-121: Build plugins are properly configured.The compiler, SCR, bundle, and coverage plugins are appropriately versioned and configured for an OSGi bundle module.
124-129: Version properties are properly defined.All dependency versions are pinned for reproducibility, and the framework version range
[5.0.0, 8.0.0)provides appropriate flexibility for OSGi imports.
mpmadhavig
force-pushed
the
custom-auth
branch
from
6c3dfaf to
860ec91
Compare
1 participant
Summary by CodeRabbit
Chores
New Features
Documentation