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

Exception handling for IdentityEventException #288

Closed
wants to merge 3 commits into from

Conversation

astik
Copy link
Contributor

@astik astik commented Sep 16, 2020

Follow up of #271, this PR introduces a new settings category: IdentityEventExceptionSettings which contains 2 parameters:
- exposeErrorCodeInMessage: whether or not we should expose the internal IdentityEventExceptionSettings error code
- badRequestErrorCodes: list of error code that should trigger a BadRequestException

In current codebase, an IdentityEventException in SCIMUserManager is transformed into a BadRequestException if the error code is "2201" (which will trigger a 400 error code). If not, it is wrapped in a CharonException (which will trigger a 500 error code).

In some use cases, we might need other error code to be transformed to bad request: this is what badRequestErrorCodes is for.
Also in some cases, we might need some additional context to personalized message in the UI. Without any help, we are doomed to an error message coming from the Java exception itself. Having access to the error code in the message is a good enough solution: this is what exposeErrorCodeInMessage is for.

I also add some unit test:

  • first to cover the existing use case which was not tested
  • then to cover the changes in the settings loading and in the SCIMUserManager updated exception handler.

What do you thing about this approach ?

(there is still some polish to do in the SCIMUserManager class as there are elements which are not used (dead code): ROLE_CLAIM, doUserValidation, addDomainToUserMembers, getMappedClaimList and now ERROR_CODE_PASSWORD_HISTORY_VIOLATION).

@CLAassistant
Copy link

CLAassistant commented Sep 16, 2020

CLA assistant check
All committers have signed the CLA.

Copy link

@jenkins-is-staging jenkins-is-staging left a comment

Choose a reason for hiding this comment

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

Approving the pull request based on the successful Jenkins job https://jenkins-support-all.wso2.com/jenkins/job/DEV_PR_BUILDER/1004/, started by ,

@astik
Copy link
Contributor Author

astik commented Feb 12, 2021

Some new stuff has been merged for exception handling:

  • 331f316 : adding BadRequestException everywhere in SCIMUserManager
  • bb72f81 : introducing SCIMUserStoreErrorResolver.java

@geve82 maybe the SCIMUserStoreErrorResolver might be what you were looking for when you ask me for this PR?
Is the PR still relevant with the newly upstream code? Should we rebase? should we give up this one?

@madurangasiriwardena
Copy link
Member

Hi @astik,

Thank you for your contribution and sorry about the delay in reviewing the PR.

As you mentioned I believe SCIMUserStoreErrorResolver introduced here full fill your requirement. I believe it will fit better for general use cases since the error code mapping are not required to be changed frequently (with configurations) and associates with the code developed for user store managers and event handlers in user/group management flows. Hence implementing an error resolve would be better to full fill the requirements.

Please let me know your thoughts.

Regards,
Maduranga.

@madurangasiriwardena
Copy link
Member

Hi @astik,

Based on my previous comment, I'm closing this PR. Please feel free to reopen if you think otherwise.

Thank you for your contribution and looking forward for more contributions in the future.

Regards,
Maduranga.

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.

4 participants