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

Unable to deserialize ui nodes of type script #157

Closed
4 of 6 tasks
roboptics opened this issue Mar 11, 2022 · 8 comments · Fixed by ory/kratos#2322
Closed
4 of 6 tasks

Unable to deserialize ui nodes of type script #157

roboptics opened this issue Mar 11, 2022 · 8 comments · Fixed by ory/kratos#2322
Assignees
Labels
bug Something is not working.

Comments

@roboptics
Copy link

Preflight checklist

Describe the bug

While getting the ClientSelfServiceSettingsFlow, if webauthn is enabled, a ui node of type script is sent.

Using the SDK for .NET to deserialize this flow, an error occurs at ClientUiNodeAttributes::FromJson() because both ClientUiNodeScriptAttributes and ClientUiNodeImageAttributes can deserialize the script ui node. This happend because the script ui node has all the mandatory attributes of the ClientUiNodeImageAttributes. Adding Width and/or Height to the mandatory attributes fixes this issue.

Reproducing the bug

  1. Enable webauthn
  2. Try to get / initialize the ClientSelfServiceSettingsFlow using .NET SDK.

Relevant log output

No response

Relevant configuration

No response

Version

0.0.1-alpha.76

On which operating system are you observing this issue?

Windows

In which environment are you deploying?

Docker Compose

Additional Context

No response

@roboptics roboptics added the bug Something is not working. label Mar 11, 2022
@aeneasr
Copy link
Member

aeneasr commented Mar 15, 2022

Thank you for the report! Could you try a more recent version of the SDK please? :) 0.0.1-alpha.124 is the most recent

@roboptics
Copy link
Author

Hi,

Still not working. Tested on version 0.0.1-alpha.131.

@aeneasr
Copy link
Member

aeneasr commented Mar 18, 2022

I see, I thought this might already have been fixed but apparently it has not. Thank you for the report, your analysis looks on point.

@aeneasr
Copy link
Member

aeneasr commented Mar 18, 2022

I looked into this, I guess the problem is that the DotNet SDK is not using the discriminator but rather trying to decode the paylaod in whatever struct it knows? The problem is that width and height are not always set, therefore they're not required / ommited if empty.

I think the problem might be that we're overriding the template. I'll try removing the override and that might resolve the issue for DotNet.

aeneasr added a commit that referenced this issue Mar 18, 2022
@aeneasr
Copy link
Member

aeneasr commented Mar 18, 2022

#159

aeneasr added a commit that referenced this issue Mar 18, 2022
@aeneasr
Copy link
Member

aeneasr commented Mar 18, 2022

Ok, unfortunately my assumption was not correct. We do indeed use the dotnet generator from upstream. So I think you found a bug in the OpenAPI Generator template :/ Looking at the code, it appears that the DotNet generator is not using the discriminator value, but instead relying on typing to find the right instance:

https://github.com/ory/client-dotnet/blob/befa798bff09a5fe9f522b066c244daf29bfb7de/src/Ory.Client/Model/ClientUiNodeAttributes.cs#L216-L244

It appears that the same thing applies for the Go SDK.

So you're right, I think the only way to work around this in the generators is to mark width/height as required. I checked the code, and it's not being "unset" anywhere, so I guess this should be a good workaround!

aeneasr added a commit to ory/kratos that referenced this issue Mar 18, 2022
@aeneasr aeneasr self-assigned this Mar 18, 2022
@roboptics
Copy link
Author

Hi,

We currently have a custom class inheriting from JsonConverter that is overriding the default serialize / deserialize functions and forcing width and height to be required. So far it's working as expected.
We will test the change you made in the near future (after it becomes available). If something comes up I'll let you know.

@aeneasr
Copy link
Member

aeneasr commented Mar 18, 2022

Ok cool, thank you! I think the changes in ory/kratos#2322 should allow you to remove the workaround :)

aeneasr added a commit to ory/kratos that referenced this issue Mar 20, 2022
harnash pushed a commit to Wikia/kratos that referenced this issue Mar 28, 2022
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this issue Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants