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 3 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
2 changes: 1 addition & 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,7 @@ jobs:

strategy:
matrix:
java-version: [ 11, 17 ]
java-version: [ 11.0.19+7 ]
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for removing java version 17?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I followed this https://github.com/wso2-extensions/identity-inbound-auth-oauth/blob/6cb4c30737e62537aad1d94c07e88283dcf80198/.github/workflows/pr-builder.yml#L23 as a workaround for the build failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

include jdk-17.0.7+7 as well

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 in bea2a6a

steps:
- uses: actions/checkout@v2
- name: Set up Adopt JDK 11 and 17
Expand Down
25 changes: 15 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,16 @@ 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())) {
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 +3471,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 +3665,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