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

EDR token refresh implementation is inconsistent and deviates from the documentation #1565

Open
Sebastian-Wurm opened this issue Sep 18, 2024 · 7 comments
Labels
bug Something isn't working
Milestone

Comments

@Sebastian-Wurm
Copy link

Sebastian-Wurm commented Sep 18, 2024

Describe the bug

On API consumer side, the method TokenRefreshHandlerImpl.createTokenRefreshRequest() adds the "Content-Type" header "application/x-www-form-urlencoded" to the token refresh request, but adds the parameters "grant_type" and "refresh_token" as query parameters of the URL rather than as urlencoded body.

On API provider side, the method TokenRefreshApiController.refreshToken() also adds the parameters as query parameters.

The corresponding documention correctly adds these parameters as urlencoded body in the HTTP request.

To Reproduce

Adding the "Content-Type" header "application/x-www-form-urlencoded", which describes the format of the HTTP body, and providing a zero-length HTTP body is inconsistent. The implementation also deviates from the documentation. Additionally, this leads to an incompatibility between EDC 0.7.2 and 0.7.3 when refreshing the EDR token, which is why we found this issue.

Expected behavior

Either send/receive the parameters as urlencoded body or remove the "Content-Type" header "application/x-www-form-urlencoded" and adapt the documentation. As tokens tend to be long and different environments may have different restrictions regarding URL length, it's probably the better idea to urlencode the parameters in the body as defined in the documentation.

Screenshots/Error Messages

N/A

Context Information

  • Used version: EDC 0.7.2 and 0.7.3 (but seems still to be on main, too)

Please be aware, that fixing this according to either of the proposals mentioned as expected behavior again breaks the EDR token refresh. So, best is to keep this as a known issue until the first major release of Tractus-X EDC.

Possible Implementation

Use URLEncoder.

@Sebastian-Wurm Sebastian-Wurm added bug Something isn't working triage all new issues awaiting classification labels Sep 18, 2024
Copy link
Contributor

This issue is stale because it has been open for 4 weeks with no activity.

@github-actions github-actions bot added the stale label Oct 23, 2024
Copy link
Contributor

github-actions bot commented Nov 7, 2024

This issue was closed because it has been inactive for 14 days since being marked as stale.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 7, 2024
@ndr-brt ndr-brt reopened this Nov 7, 2024
@ndr-brt ndr-brt removed the stale label Nov 7, 2024
@Sebastian-Wurm
Copy link
Author

@ndr-brt : Thanks for reopening.

@ndr-brt
Copy link
Contributor

ndr-brt commented Nov 7, 2024

Additionally, this leads to an incompatibility between EDC 0.7.2 and 0.7.3 when refreshing the EDR token, which is why we found this issue.

could your report the error you are getting? like status code/response message/stacktraces?

@Sebastian-Wurm
Copy link
Author

Sebastian-Wurm commented Nov 7, 2024

Additionally, this leads to an incompatibility between EDC 0.7.2 and 0.7.3 when refreshing the EDR token, which is why we found this issue.

could your report the error you are getting? like status code/response message/stacktraces?

HTTP error 415 unsupported media type is returned on

  1. calling data address endpoint with auto-refresh=true if EDR is expired. 415 error is wrapped into a 400 error on consumer side.
  2. calling the EDRs API refresh endpoint

(1.)
{
"servlet": "EDC-management",
"message": "Unsupported Media Type",
"url": "/api/management/v2/edrs/b07de376-c5a4-4115-a5cf-4ab309a88e76/refresh",
"status": "415"
}

(2.)
[
{
"message": "Unsupported Media Type",
"type": "InvalidRequest",
"path": null,
"invalidValue": null
}
]

@wolf4ood
Copy link
Contributor

wolf4ood commented Nov 7, 2024

I think there are two issue here.

One is the unsupported media type which was fixed in 0.7.3.
If the consumer is using 0.7.2 we cannot do much as the issue arose also with 0.7.2 vs 0.7.2.

The other issue is to use form body instead of query params which is a breaking change if we suddenly switch
to form body.

If we want to solve this we should guarantee backward compatibility

@Sebastian-Wurm
Copy link
Author

I think there are two issue here.

One is the unsupported media type which was fixed in 0.7.3. If the consumer is using 0.7.2 we cannot do much as the issue arose also with 0.7.2 vs 0.7.2.

The other issue is to use form body instead of query params which is a breaking change if we suddenly switch to form body.

If we want to solve this we should guarantee backward compatibility

Right, that's why I wrote to keep the issue until the first major release of EDC. As discussed already, the linked documentation requests x-www-form-urlencoded body.

@wolf4ood wolf4ood added this to the Backlog milestone Nov 7, 2024
@wolf4ood wolf4ood removed the triage all new issues awaiting classification label Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants