-
Notifications
You must be signed in to change notification settings - Fork 619
Feature - In-Flow Extension #8108
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.
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
base: master
Are you sure you want to change the base?
Changes from 11 commits
acd491e
c3fe3b0
ea66f24
f77b29d
4444b23
93c87ce
dbedb57
ea0efce
2400b67
1150b2c
7654677
df4857a
4db0026
f88134c
b587758
3c01603
7386607
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,9 +18,18 @@ | |
|
|
||
| package org.wso2.carbon.identity.action.execution.api.model; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonInclude; | ||
| import com.fasterxml.jackson.core.JsonGenerator; | ||
| import com.fasterxml.jackson.databind.JsonSerializer; | ||
| import com.fasterxml.jackson.databind.SerializerProvider; | ||
| import com.fasterxml.jackson.databind.annotation.JsonSerialize; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
||
| /** | ||
| * This class models the User. | ||
|
|
@@ -30,9 +39,11 @@ public class User { | |
|
|
||
| private final String id; | ||
| private final List<UserClaim> claims = new ArrayList<>(); | ||
| private final Map<String, char[]> userCredentials = new HashMap<>(); | ||
| private final List<String> groups = new ArrayList<>(); | ||
| private final List<String> roles = new ArrayList<>(); | ||
| private Organization organization; | ||
| private UserStore userStoreDomain; | ||
| /** | ||
| * Represents the user id when the user is shared across sub-organizations. | ||
| * This field differs from the regular user id ({@link #id}) in scenarios where a user is accessed in the context | ||
|
|
@@ -65,15 +76,18 @@ public User(Builder builder) { | |
|
|
||
| this.id = builder.id; | ||
| this.claims.addAll(builder.claims); | ||
| this.userCredentials.putAll(builder.userCredentials); | ||
| this.groups.addAll(builder.groups); | ||
|
Comment on lines
+79
to
80
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Defensively copy
🔧 Proposed fix@@
- this.userCredentials.putAll(builder.userCredentials);
+ builder.userCredentials.forEach((k, v) ->
+ this.userCredentials.put(k, v == null ? null : v.clone()));
@@
public Map<String, char[]> getUserCredentials() {
-
- return Collections.unmodifiableMap(userCredentials);
+ Map<String, char[]> copy = new HashMap<>();
+ userCredentials.forEach((k, v) -> copy.put(k, v == null ? null : v.clone()));
+ return Collections.unmodifiableMap(copy);
}
@@
public Builder userCredentials(Map<String, char[]> userCredentials) {
-
- this.userCredentials.putAll(userCredentials);
+ if (userCredentials != null) {
+ userCredentials.forEach((k, v) ->
+ this.userCredentials.put(k, v == null ? null : v.clone()));
+ }
return this;
}Also applies to: 101-104, 224-227 🤖 Prompt for AI Agents |
||
| this.roles.addAll(builder.roles); | ||
| this.organization = builder.organization; | ||
| this.sharedUserId = builder.sharedUserId; | ||
| this.userType = builder.userType; | ||
| this.federatedIdP = builder.federatedIdP; | ||
| this.accessingOrganization = builder.accessingOrganization; | ||
| this.userStoreDomain = builder.userStoreDomain; | ||
| } | ||
|
|
||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public String getId() { | ||
|
|
||
| return id; | ||
|
|
@@ -84,6 +98,11 @@ public List<UserClaim> getClaims() { | |
| return Collections.unmodifiableList(claims); | ||
| } | ||
|
|
||
| public Map<String, char[]> getUserCredentials() { | ||
|
|
||
| return Collections.unmodifiableMap(userCredentials); | ||
| } | ||
|
|
||
| public List<String> getGroups() { | ||
|
|
||
| return Collections.unmodifiableList(groups); | ||
|
|
@@ -99,6 +118,13 @@ public Organization getOrganization() { | |
| return organization; | ||
| } | ||
|
|
||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
|
KD23243 marked this conversation as resolved.
|
||
| @JsonSerialize(using = UserStoreNameSerializer.class) | ||
| public UserStore getUserStoreDomain() { | ||
|
KD23243 marked this conversation as resolved.
|
||
|
|
||
| return userStoreDomain; | ||
| } | ||
|
|
||
| public String getSharedUserId() { | ||
|
|
||
| return sharedUserId; | ||
|
|
@@ -126,9 +152,11 @@ public static class Builder { | |
|
|
||
| private final String id; | ||
| private final List<UserClaim> claims = new ArrayList<>(); | ||
| private final Map<String, char[]> userCredentials = new HashMap<>(); | ||
| private final List<String> groups = new ArrayList<>(); | ||
| private final List<String> roles = new ArrayList<>(); | ||
| private Organization organization; | ||
| private UserStore userStoreDomain; | ||
| private String sharedUserId; | ||
| private String userType; | ||
| private String federatedIdP; | ||
|
|
@@ -163,6 +191,12 @@ public Builder organization(Organization organization) { | |
| return this; | ||
| } | ||
|
|
||
| public Builder userStoreDomain(UserStore userStoreDomain) { | ||
|
|
||
| this.userStoreDomain = userStoreDomain; | ||
| return this; | ||
| } | ||
|
|
||
| public Builder sharedUserId(String sharedUserId) { | ||
|
|
||
| this.sharedUserId = sharedUserId; | ||
|
|
@@ -187,9 +221,24 @@ public Builder accessingOrganization(Organization accessingOrganization) { | |
| return this; | ||
| } | ||
|
|
||
| public Builder userCredentials(Map<String, char[]> userCredentials) { | ||
|
|
||
| this.userCredentials.putAll(userCredentials); | ||
| return this; | ||
| } | ||
|
|
||
| public User build() { | ||
|
|
||
| return new User(this); | ||
| } | ||
| } | ||
|
|
||
| private static class UserStoreNameSerializer extends JsonSerializer<UserStore> { | ||
|
|
||
| @Override | ||
| public void serialize(UserStore value, JsonGenerator gen, SerializerProvider serializers) throws IOException { | ||
|
|
||
| gen.writeString(value.getName() != null ? value.getName() : ""); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting an empty string might be problematic for the other action types, as this will start adding this userstoredomain attribute to the user object with an empty value. Since this userStoreDomain is specific for the Inflow extension, I suggest using an extended User class in the inflow extensions component. |
||
| } | ||
| } | ||
|
Comment on lines
+236
to
+243
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this |
||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -148,14 +148,12 @@ public ActionExecutionStatus execute(ActionType actionType, String actionId, | |||||||||||||||||||||
| throw new ActionExecutionException("Action Id cannot be blank."); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| try { | ||||||||||||||||||||||
| Action action = getActionByActionId(actionType, actionId, tenantDomain); | ||||||||||||||||||||||
| return execute(action, flowContext, tenantDomain); | ||||||||||||||||||||||
| } catch (ActionExecutionRuntimeException e) { | ||||||||||||||||||||||
| LOG.debug("Skip executing action for action type: " + actionType.name(), e); | ||||||||||||||||||||||
| // Skip executing actions when no action available is considered as action execution being successful. | ||||||||||||||||||||||
| Action action = getActionByActionId(actionType, actionId, tenantDomain); | ||||||||||||||||||||||
| if (action == null) { | ||||||||||||||||||||||
| LOG.debug("No action found for action Id: " + actionId + ". Skipping action execution."); | ||||||||||||||||||||||
| return new SuccessStatus.Builder().setResponseContext(flowContext.getContextData()).build(); | ||||||||||||||||||||||
|
Comment on lines
+151
to
154
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard the new concatenated DEBUG log. The new debug statement does string concatenation without a debug guard. Proposed fix Action action = getActionByActionId(actionType, actionId, tenantDomain);
if (action == null) {
- LOG.debug("No action found for action Id: " + actionId + ". Skipping action execution.");
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("No action found for action Id: " + actionId + ". Skipping action execution.");
+ }
return new SuccessStatus.Builder().setResponseContext(flowContext.getContextData()).build();
}As per coding guidelines, "DEBUG: guard all non-trivial debug logs with 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
| return execute(action, flowContext, tenantDomain); | ||||||||||||||||||||||
|
Comment on lines
+151
to
+156
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need this?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ashanthamara for feedback |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| private ActionExecutionStatus<?> execute(Action action, FlowContext flowContext, String tenantDomain) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -94,7 +94,11 @@ public String getLatestVersion(ActionTypes actionType) throws ActionMgtServerExc | |
| return getVersion( | ||
| ActionTypeConfig.PRE_UPDATE_PROFILE.getLatestVersionProperty(), actionType); | ||
| case PRE_ISSUE_ID_TOKEN: | ||
| return getVersion(ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getLatestVersionProperty(), actionType); | ||
| return getVersion( | ||
| ActionTypeConfig.PRE_ISSUE_ID_TOKEN.getLatestVersionProperty(), actionType); | ||
| case FLOW_EXTENSION: | ||
| return getVersion( | ||
| ActionTypeConfig.FLOW_EXTENSION.getLatestVersionProperty(), actionType); | ||
| default: | ||
| throw new ActionMgtServerException("Unsupported action type: " + actionType); | ||
|
KD23243 marked this conversation as resolved.
|
||
| } | ||
|
|
@@ -140,6 +144,10 @@ public enum ActionTypeConfig { | |
| "Actions.Types.PreIssueIdToken.ActionRequest.ExcludedHeaders.Header", | ||
| "Actions.Types.PreIssueIdToken.ActionRequest.ExcludedParameters.Parameter", | ||
| "Actions.Types.PreIssueIdToken.Version.Latest" | ||
| ), | ||
| FLOW_EXTENSION( | ||
| "Actions.Types.FlowExtension.ActionRequest.ExcludedHeaders.Header", | ||
| "Actions.Types.FlowExtension.Version.Latest" | ||
| ); | ||
|
|
||
| private final String excludedHeadersProperty; | ||
|
|
@@ -153,6 +161,11 @@ public enum ActionTypeConfig { | |
| this.latestVersionProperty = latestVersionProperty; | ||
| } | ||
|
|
||
| ActionTypeConfig(String excludedHeadersProperty, String latestVersionProperty) { | ||
|
|
||
| this(excludedHeadersProperty, null, latestVersionProperty); | ||
| } | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can pass null value for the exclueded params, from when defining the object right ? No need to introduce another constructor |
||
| public String getExcludedHeadersProperty() { | ||
|
|
||
| return excludedHeadersProperty; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
revert!