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.

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

Fix: SCIM error handling for group display name (fixes #20344) #557

Closed

Conversation

BimsaraBodaragama
Copy link
Contributor

Purpose

Fix SCIM error handling to provide appropriate messages when a group display name is used instead of a group ID.

Goals

To ensure compliance with RFC 7644 and provide clear, actionable error messages to users when an invalid request is made with a group display name.

Approach

  • Implemented error response with scimType set to invalidSyntax for requests using a group display name instead of a group ID.
  • Provided a clear and concise error message in the detail field.
  • Ensured the error response adheres to the SCIM specification.

Issue

Resolves #20334

@CLAassistant
Copy link

CLAassistant commented Jun 28, 2024

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ BimsaraBodaragama
❌ Bimsara Bodaragama


Bimsara Bodaragama seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@BimsaraBodaragama BimsaraBodaragama self-assigned this Jun 28, 2024
// Error message and SCIM type
String errorMessage = "Updating groups of the role by display name is not supported. "
+ "Update using group id instead.";
String scimType = "invalidSyntax"; // From RFC 7644 Table 9
Copy link
Contributor

Choose a reason for hiding this comment

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

Without hardcoding here you can directly use ResponseCodeConstants.INVALID_SYNTAX constant in charon

scimType variable is also redundant then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in b974ba9

@@ -1125,7 +1125,13 @@ private void doUpdateGroups(String roleId, Set<String> newGroupIDList, Set<Strin
new ArrayList<>(deleteGroupIDList), tenantDomain);
} catch (IdentityRoleManagementException e) {
if (RoleConstants.Error.INVALID_REQUEST.getCode().equals(e.getErrorCode())) {
throw new BadRequestException(e.getMessage());
// Error message and SCIM type
String errorMessage = "Updating groups of the role by display name is not supported. "
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct place to handle the mentioned error?
Issue should be directly taking display eq ... something as group id
We have to throw the error from that place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in b974ba9


String value = groupObject.get(SCIMConstants.CommonSchemaConstants.VALUE);

if (value == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we use the StringUtils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non descriptive error message for SCIM Roles v2 patch operation
4 participants