ory / sdk

The place where ORY's SDKs are being auto-generated
Apache License 2.0
140 stars 84 forks source link

kratos Dart API client throws exception when calling /self-service/login/browser #97

Closed provokateurin closed 2 years ago

provokateurin commented 3 years ago

Describe the bug

The kratos Dart API client tries to json decode the result of calling initializeSelfServiceLoginFlowForBrowsers (/self-service/login/browser). The problem is that endpoint returns an html page which is correct. The API client isn't aware of this and throws a FormatException while it tries to deserialize the body.

To Reproduce

Steps to reproduce the behavior:

  1. Call function V0alpha1Api.initializeSelfServiceLoginFlowForBrowsers.
  2. Function throws an exception although the API response is correct.

Expected behavior

The function should not throw an error. I don't know if it should be expected to also return some data from the response.

Environment

aeneasr commented 3 years ago

Thank you for the report - could you please investigate if the SDK is sending the Accept: application/json HTTP Header? Ory Kratos responds with HTML if that header is not included because it thinks it is a regular browser.

provokateurin commented 3 years ago

The client sends a request for a browser and therefore a html response is expected as far as I understood it. The problem is the deserialization of the response which the client tries to decode as json which obviously fails because it is not json.

aeneasr commented 3 years ago

The problem is that the client sends the incorrect HTTP header. The browser endpoint returns JSON if the correct header is included. Trust me :)

provokateurin commented 3 years ago

Ah ok. I'll try to debug it. Thanks for your quick responses!

aeneasr commented 3 years ago

np :)

provokateurin commented 3 years ago

The log I see in kratos is:

kratos_1           | time=2021-08-12T10:52:57Z level=info msg=started handling request http_request=map[headers:map[accept:*/* accept-encoding:gzip, deflate accept-language:en-US,en;q=0.5 cookie:aHR0cDovLzEyNy4wLjAuMTo0NDMzLw_csrf_token=C8dLRrXWAOFy0DXSomMp+IGA549diwF4tH/l7X5DhAA=; aHR0cDovL2xvY2FsaG9zdDo0NDMzLw_csrf_token=bwj+E+rvpi90DtREbc+3SmUTnstsmLpShLP1ZD7AGbo= referer:http://localhost:4455/login user-agent:Mozilla/5.0 (X11; Linux x86_64; rv:92.0) Gecko/20100101 Firefox/92.0 x-forwarded-for:127.0.0.1] host:localhost:4455 method:GET path:/self-service/login/browser query:<nil> remote:[::1]:42634 scheme:http]

So it is not sending Accept: application/json but Accept: */*.

aeneasr commented 3 years ago

Ok, so that is the issue then - thank you for the investigation. This to me looks like a bug in the DartJS generator because all other generators send the correct accept header.

I couldn't find an existing issue for this in openapi-generator ( https://github.com/OpenAPITools/openapi-generator/issues?q=is%3Aissue+dart+accept+is%3Aopen ) but it would be great if you could open one - or even better - fix it in the dartjs generator there :)

provokateurin commented 3 years ago

This fixes it:

diff --git a/clients/kratos/dart/lib/api/v0alpha1_api.dart b/clients/kratos/dart/lib/api/v0alpha1_api.dart
index 5a8694ab..9e4aea48 100644
--- a/clients/kratos/dart/lib/api/v0alpha1_api.dart
+++ b/clients/kratos/dart/lib/api/v0alpha1_api.dart
@@ -1140,7 +1140,7 @@ class V0alpha1Api {
     Object postBody;

     final queryParams = <QueryParam>[];
-    final headerParams = <String, String>{};
+    final headerParams = <String, String>{'Accept':'application/json',};
     final formParams = <String, String>{};

     if (refresh != null) {
provokateurin commented 3 years ago

I've never worked with the openapi generator, but i can try to fix it. How do i exactly generate the clients in this repo? Are there any instructions?

aeneasr commented 3 years ago

I would refer you to their docs here: https://github.com/OpenAPITools/openapi-generator/blob/master/CONTRIBUTING.md :)

provokateurin commented 3 years ago

No I meant in this repository to regenerate the dart client.

aeneasr commented 3 years ago

Ah, unfortunately it is not possible to fix this here as it will be overwritten everytime we regenerate the SDKs, so it needs to be fixed in the generator...

provokateurin commented 3 years ago

Yes I know, but I need to test my changes on this repo to see if it works. Like what command do I need to run to regenerate the client myself.

provokateurin commented 3 years ago

The dart generator never supported headers: https://github.com/OpenAPITools/openapi-generator/blob/48924eb1a08e6ce1726ce15ef03ae5bcf96e5d0c/modules/openapi-generator/src/main/resources/dart2/api.mustache#L70

aeneasr commented 3 years ago

I believe it should be set here:

https://github.com/OpenAPITools/openapi-generator/blob/48924eb1a08e6ce1726ce15ef03ae5bcf96e5d0c/modules/openapi-generator/src/main/resources/dart2/api.mustache#L86-L93

provokateurin commented 3 years ago

Still not set in the generated code so is a bug there.

aeneasr commented 3 years ago

No it is not a bug here, because all other generated clients work normally. Again, please trust me with the drawn conclusions. I have spent so much time with openapi generator, and wrote all of the specs. I am trying to help you resolve this issue!

Here is our spec, we clearly indicate that the response is application JSON. In OpenAPI this FORCES the client to set Accept: application/json

https://github.com/ory/kratos/blob/11bdc4a815bd1e264d0ccbf42ca56f76e6ab1bbc/spec/api.json#L2895-L2904

The openapi generator for dart apparently does not respect that in the code generation.

provokateurin commented 3 years ago

Yes I totally agree with you. With "there" I meant the openapi generator and not this repo. I feel a bit like you didn't understand me completely, but I agree with everything you said.

aeneasr commented 3 years ago

Ah, sorry, I misunderstood :) Then forget everything I said! :)

I think that if you are equipped with

https://github.com/ory/kratos/blob/11bdc4a815bd1e264d0ccbf42ca56f76e6ab1bbc/spec/api.json#L2895-L2904

you can definitely file an issue. If you want to take it a step further and fix the generator, we can also use the template temporarily in this directory here so that the fix is included even if openapi-generator does not merge it right away (they usually take their time).

provokateurin commented 3 years ago

I'm already trying to fix the generator. If I feel like it can't do it, I fill an issue.

aeneasr commented 3 years ago

See also #109

github-actions[bot] commented 2 years ago

Hello contributors!

I am marking this issue as stale as it has not received any engagement from the community or maintainers a year. That does not imply that the issue has no merit! If you feel strongly about this issue

Throughout its lifetime, Ory has received over 10.000 issues and PRs. To sustain that growth, we need to prioritize and focus on issues that are important to the community. A good indication of importance, and thus priority, is activity on a topic.

Unfortunately, burnout has become a topic of concern amongst open-source projects.

It can lead to severe personal and health issues as well as opening catastrophic attack vectors.

The motivation for this automation is to help prioritize issues in the backlog and not ignore, reject, or belittle anyone.

If this issue was marked as stale erroneous you can exempt it by adding the backlog label, assigning someone, or setting a milestone for it.

Thank you for your understanding and to anyone who participated in the conversation! And as written above, please do participate in the conversation if this topic is important to you!

Thank you 🙏✌️