Update ExternalResourceOwnerPasswordCredentialsService.java#14149
Conversation
Added custom script WebApplicationException handling. Signed-off-by: ayushjain0702 <45758782+ayushjain0702@users.noreply.github.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughThis PR modifies error handling in ChangesROPC Script Error Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@jans-auth-server/server/src/main/java/io/jans/as/server/service/external/ExternalResourceOwnerPasswordCredentialsService.java`:
- Around line 89-93: The thrown WebApplicationException in
ExternalResourceOwnerPasswordCredentialsService currently builds a Response but
does not attach the original caught exception as the cause; update the throw to
use the WebApplicationException constructor that accepts both a Throwable cause
and the Response (i.e., new WebApplicationException(caughtException, response))
so the original exception (the caught variable in the surrounding catch block,
e.g., "e" or "ex") is preserved in the cause chain while still using
errorResponseFactory to build the Response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f0262c32-d7e5-490c-a405-07f1f0970f79
📒 Files selected for processing (1)
jans-auth-server/server/src/main/java/io/jans/as/server/service/external/ExternalResourceOwnerPasswordCredentialsService.java
| throw new WebApplicationException(errorResponseFactory | ||
| .newErrorResponse(Response.Status.INTERNAL_SERVER_ERROR) | ||
| .entity(errorResponseFactory.getErrorAsJson(TokenErrorResponseType.ACCESS_DENIED, "", "Unable to run 'executeExternalAuthenticate' method in ROPC script.")) | ||
| .build()); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Preserve the original exception as the cause.
The new WebApplicationException doesn't include the original exception in its cause chain. While the error is logged at line 87, preserving the cause helps centralized exception handlers or upstream logging capture the full context.
WebApplicationException supports a constructor that accepts both cause and response.
♻️ Proposed fix to preserve stack trace
- throw new WebApplicationException(errorResponseFactory
+ throw new WebApplicationException(ex, errorResponseFactory
.newErrorResponse(Response.Status.INTERNAL_SERVER_ERROR)
.entity(errorResponseFactory.getErrorAsJson(TokenErrorResponseType.ACCESS_DENIED, "", "Unable to run 'executeExternalAuthenticate' method in ROPC script."))
.build());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new WebApplicationException(errorResponseFactory | |
| .newErrorResponse(Response.Status.INTERNAL_SERVER_ERROR) | |
| .entity(errorResponseFactory.getErrorAsJson(TokenErrorResponseType.ACCESS_DENIED, "", "Unable to run 'executeExternalAuthenticate' method in ROPC script.")) | |
| .build()); | |
| } | |
| throw new WebApplicationException(ex, errorResponseFactory | |
| .newErrorResponse(Response.Status.INTERNAL_SERVER_ERROR) | |
| .entity(errorResponseFactory.getErrorAsJson(TokenErrorResponseType.ACCESS_DENIED, "", "Unable to run 'executeExternalAuthenticate' method in ROPC script.")) | |
| .build()); |
🧰 Tools
🪛 PMD (7.24.0)
[Medium] 89-92: PreserveStackTrace (Best Practices): Thrown exception does not preserve the stack trace of exception 'ex' on all code paths
(PreserveStackTrace (Best Practices))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@jans-auth-server/server/src/main/java/io/jans/as/server/service/external/ExternalResourceOwnerPasswordCredentialsService.java`
around lines 89 - 93, The thrown WebApplicationException in
ExternalResourceOwnerPasswordCredentialsService currently builds a Response but
does not attach the original caught exception as the cause; update the throw to
use the WebApplicationException constructor that accepts both a Throwable cause
and the Response (i.e., new WebApplicationException(caughtException, response))
so the original exception (the caught variable in the surrounding catch block,
e.g., "e" or "ex") is preserved in the cause chain while still using
errorResponseFactory to build the Response.
| saveScriptError(customScriptConfiguration.getCustomScript(), ex); | ||
| return false; | ||
| saveScriptError(context.getScript().getCustomScript(), ex); | ||
| throw new WebApplicationException(errorResponseFactory |
There was a problem hiding this comment.
We already have WebApplicationException re-thrown above. Why do we break normal execution for Exception ? What case are we trying to cover in line 89 ?
There was a problem hiding this comment.
Hi @yuriyz ,
The first catch is to catch the exception sent by the Custom ROPC script. LIke below in the Jython code:
exception = context.createWebApplicationException(503,'{"error": "access_denied", "error_description": "User is blacklisted"}')
context.setWebApplicationException(exception)The second one is to create the exception generally, in case the Script throws any other kind of exception. Like NPE, or Http error while trying to connect to some external endpoint etc.
We are trying to make the script similar to ExternalAuthorizationChallengeService.java file. Here, in the code, we are using context.getAuthzRequest() function. But, in case of ROPC, since, there is no Authorization request, we get NPE in that case.
Hope this helps to understand what we are trying to achieve here.
There was a problem hiding this comment.
@ayushjain0702 We can't apply same behavior as in ExternalAuthorizationChallengeService.java because for authorization challenge we have dedicated script for this endpoint only. ROPC script is used by Authorization Endpoint and by Token Endpoint. Throwing exception can lead to unexpected behavior. For example it's perfectly valid to get ROPC fail and let authorization go on and pass execution to lets say PostAuthn or other script.
You can easily cover case which you've described above by wrapping code inside custom script in try/catch block and throw WebApplicationException explicitly. This will work for NPE, http request failure or other error. In this way response will be propagated outside.
There was a problem hiding this comment.
Hi @yuriyz ,
We have tried this. The exception thrown by the ROPC custom script is not handled properly. The exception is only saved in the Script configuration. We have tested this multiple times and only after that we created this pull request.
Can you please suggest any other method of doing it then?
Thanks.
4 participants
Added custom script WebApplicationException handling.
Prepare
Description
Added Custom WebApplicationException handling for ROPC scripts. The current implementation does not allow to customize the error and error_description to be sent to the end user.
Target issue
#14148
closes #14148
Implementation Details
Added Custom WebApplicationException handling for ROPC scripts. The current implementation does not allow to customize the error and error_description to be sent to the end user.
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit