Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix Getting 500 error response for client errors in SCIM/Groups PATCH #478

Merged
merged 5 commits into from
Aug 10, 2023
Merged

Fix Getting 500 error response for client errors in SCIM/Groups PATCH #478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79f4362
Fix issue Getting 500 error response for client errors in SCIM/Groups…
PasinduYeshan Jul 31, 2023
fedd887
Refactor code
PasinduYeshan Aug 2, 2023
6dcdab9
Fix for PR build issue
PasinduYeshan Aug 3, 2023
ed20895
Add error log for UserStoreException and add java 17 for PR build
PasinduYeshan Aug 7, 2023
bea2a6a
Merge branch 'master' into fix/scim-groups-patch-500-error
PasinduYeshan Aug 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/pr-builder.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ jobs:

strategy:
matrix:
java-version: [ 11.0.19+7 ]
java-version: [ 11.0.19+7, 17.0.7+7 ]

steps:
- uses: actions/checkout@v2
- name: Set up Adopt JDK 11 and 17
Expand Down
26 changes: 16 additions & 10 deletions ...cim2.common/src/main/java/org/wso2/carbon/identity/scim2/common/impl/SCIMUserManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
import org.wso2.carbon.user.core.claim.ClaimManager;
import org.wso2.carbon.user.core.common.AbstractUserStoreManager;
import org.wso2.carbon.user.core.constants.UserCoreClaimConstants;
import org.wso2.carbon.user.core.constants.UserCoreErrorConstants;
import org.wso2.carbon.user.core.jdbc.JDBCUserStoreManager;
import org.wso2.carbon.user.core.model.Condition;
import org.wso2.carbon.user.core.model.ExpressionAttribute;
Expand Down Expand Up @@ -3366,9 +3367,14 @@ private void doPatchGroup(String groupId, String currentGroupName, Map<String, L
temporaryMembers.clear();

for (String member : deletedMembers) {
String username = UserCoreUtil.addDomainToName(UserCoreUtil.removeDomainFromName(member),
userStoreDomainForGroup);
temporaryMembers.add(username);
if (addedMembers.isEmpty() && StringUtils.isBlank(member)) {
throw new BadRequestException(ResponseCodeConstants.INVALID_VALUE);
}
if (StringUtils.isNotBlank(member)) {
String username = UserCoreUtil.addDomainToName(UserCoreUtil.removeDomainFromName(member),
userStoreDomainForGroup);
temporaryMembers.add(username);
}
}

deletedMembers.clear();
Expand Down Expand Up @@ -3413,13 +3419,17 @@ private void doPatchGroup(String groupId, String currentGroupName, Map<String, L
carbonUM.updateGroupName(currentGroupName, newGroupName);

} catch (UserStoreException e) {
if (e instanceof org.wso2.carbon.user.core.UserStoreException && StringUtils
.equals(UserCoreErrorConstants.ErrorMessages.ERROR_CODE_NON_EXISTING_USER.getCode(),
((org.wso2.carbon.user.core.UserStoreException) e).getErrorCode())) {
log.error(UserCoreErrorConstants.ErrorMessages.ERROR_CODE_NON_EXISTING_USER.getMessage(), e);
throw new BadRequestException(ResponseCodeConstants.INVALID_VALUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

why aren't we passing the throwable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

public class BadRequestException extends AbstractCharonException {
    public BadRequestException() {
        this("invalid request");
    }

    public BadRequestException(String scimType) {
        this("Request is unparsable, syntactically incorrect, or violates schema.", scimType);
    }

    public BadRequestException(String details, String scimType) {
        super(400, details, scimType);
    }
}

This is the implementation of BadRequestException. There's no way to pass the throwable to the BadRequestException.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a log with the throwable as suggested offline.

}
throw resolveError(e, e.getMessage());
} catch (IdentitySCIMException e) {
throw new CharonException(e.getMessage(), e);
} catch (IdentityApplicationManagementException e) {
throw new CharonException("Error retrieving User Store name. ", e);
} catch (BadRequestException e) {
throw new CharonException("Error in updating the group", e);
}
}

Expand Down Expand Up @@ -3462,10 +3472,6 @@ private void prepareAddedRemovedMemberLists(Set<String> addedMembers, Set<String
SCIMConstants.OperationalConstants.REMOVE)) {
addedMembers.remove(memberObject.get(SCIMConstants.GroupSchemaConstants.DISPLAY));
removedMembers.add(memberObject.get(SCIMConstants.GroupSchemaConstants.DISPLAY));
String value = memberObject.get(SCIMConstants.GroupSchemaConstants.VALUE);
if (StringUtils.isNotBlank(value)) {
deletedMemberIds.add(value);
}
}
}

Expand Down Expand Up @@ -3660,7 +3666,7 @@ private Set<String> getMemberValuesFromUserstore(Set<String> memberUsernames, St
if (StringUtils.isEmpty(userId)) {
String error = "User: " + userName + " doesn't exist in the user store. Hence can not update the " +
"group: " + displayName;
throw new BadRequestException(error);
throw new BadRequestException(error, ResponseCodeConstants.INVALID_VALUE);
}
memberUserIds.add(userId);
}
Expand Down
Loading