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

Expose error response json in case extra parameters are given. #885

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

doreaimon
Copy link

Checklist

  • I read the Contribution Guidelines
  • I signed the CLA and WG Agreements
  • I ran, updated and added unit tests as necessary.
  • I verified the contribution matches existing coding style.
  • I updated the documentation if necessary.

Motivation and Context

We have found that some servers will return non-standard parameters along with the standard ones in their error responses, and we see a need for getting access to those parameters. To facilitate this, we would like to pass back the full original error response JSONObject along in the token request callback in the AuthorizationException.

Description

We held onto the original error response JSONObject and added it as an optional field in the AuthorizationException that is passed to the token request callback.

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.93%. Comparing base (5966cc7) to head (496fbb6).
Report is 7 commits behind head on master.

Files Patch % Lines
.../java/net/openid/appauth/AuthorizationService.java 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master     #885   +/-   ##
=========================================
  Coverage     82.92%   82.93%           
  Complexity      532      532           
=========================================
  Files            46       46           
  Lines          2642     2643    +1     
  Branches        264      264           
=========================================
+ Hits           2191     2192    +1     
  Misses          351      351           
  Partials        100      100           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -464,22 +464,22 @@ public static AuthorizationException byString(String error) {

private static AuthorizationException generalEx(int code, @Nullable String errorDescription) {
return new AuthorizationException(
TYPE_GENERAL_ERROR, code, null, errorDescription, null, null);
TYPE_GENERAL_ERROR, code, null, errorDescription, null, null, null);
Copy link

Choose a reason for hiding this comment

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

467

Copy link

Choose a reason for hiding this comment

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

Null

@@ -464,22 +464,22 @@ public static AuthorizationException byString(String error) {

private static AuthorizationException generalEx(int code, @Nullable String errorDescription) {
return new AuthorizationException(
TYPE_GENERAL_ERROR, code, null, errorDescription, null, null);
TYPE_GENERAL_ERROR, code, null, errorDescription, null, null, null);
Copy link

Choose a reason for hiding this comment

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

General error

@@ -464,22 +464,22 @@ public static AuthorizationException byString(String error) {

private static AuthorizationException generalEx(int code, @Nullable String errorDescription) {
return new AuthorizationException(
TYPE_GENERAL_ERROR, code, null, errorDescription, null, null);
TYPE_GENERAL_ERROR, code, null, errorDescription, null, null, null);
Copy link

Choose a reason for hiding this comment

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

Errodescriåtipn null null null

@@ -464,22 +464,22 @@ public static AuthorizationException byString(String error) {

private static AuthorizationException generalEx(int code, @Nullable String errorDescription) {
return new AuthorizationException(
TYPE_GENERAL_ERROR, code, null, errorDescription, null, null);
TYPE_GENERAL_ERROR, code, null, errorDescription, null, null, null);
Copy link

Choose a reason for hiding this comment

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

467+

Copy link

Choose a reason for hiding this comment

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

+467 to +465

Copy link

@thereeroyz thereeroyz left a comment

Choose a reason for hiding this comment

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

__

Copy link

@thereeroyz thereeroyz left a comment

Choose a reason for hiding this comment

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

@pcuriel
Copy link

pcuriel commented Feb 8, 2024

Very interested in this as well.

In this case, due to a Keycloak custom Authenticator returning additional non-standard fields for some error cases, which there is no way to access with the current implementation of AuthorizationException.

*/
@Nullable
public final JSONObject responseJson;

/**

Choose a reason for hiding this comment

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

Add corresponding field for serialization

// Line 142
@VisibleForTesting
static final String KEY_RESPONSE_JSON = "responseJson";

@@ -559,6 +563,7 @@ public static AuthorizationException fromJson(@NonNull JSONObject json) throws J
JsonUtil.getStringIfDefined(json, KEY_ERROR),
JsonUtil.getStringIfDefined(json, KEY_ERROR_DESCRIPTION),
JsonUtil.getUriIfDefined(json, KEY_ERROR_URI),
json,

Choose a reason for hiding this comment

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

Suggested change
json,
JsonUtil.getJsonObjectIfDefined(json, KEY_RESPONSE_JSON),

And toJson is missing the reponseJson field serialization:

// Line 663
JsonUtil.putIfNotNull(json, KEY_ERROR_URI, errorUri);
JsonUtil.putIfNotNull(json, KEY_RESPONSE_JSON, responseJson);
return json;

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.

9 participants