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

fix: unflatten form data into correct body format #104

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

Conversation

DASPRiD
Copy link
Contributor

@DASPRiD DASPRiD commented Mar 20, 2023

The format for bodies with traits dictates that the traits come in through their own object. This becomes especially visible when using an SDK client compiled with a newer version of OpenAPI generator, as they only process properties which are defined in the API schema, which is not the case for flattened properties.

Fixes #93

Checklist

  • I have read the contributing guidelines and signed the CLA.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security vulnerability,
    I confirm that I got green light (please contact [email protected]) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added necessary documentation within the code base (if appropriate).

@DASPRiD DASPRiD force-pushed the fix/unflatten-body branch 2 times, most recently from ab0745e to e27f290 Compare March 20, 2023 22:34
@Benehiko
Copy link
Contributor

Still not sure why we need to unflatten data when the data is unflattened in Kratos.

Please take a look at some of the examples. These examples all work perfectly fine against Ory and are tested.

https://github.com/ory/elements/blob/main/examples/nextjs-spa/src/pages/login.tsx#L88-L102

Are you getting errors when trying Ory Elements against Ory Kratos? Could you please give me more information on what exactly isn't working.

@DASPRiD
Copy link
Contributor Author

DASPRiD commented Mar 30, 2023

It is working fine at the moment, because the currently generated SDK does not verify the input parameters.

When a newer generator is used (which is planned, see ory/sdk#256 (comment)), the input parameters are actually checked.

Alternatively, the API schema could be updated for these endpoint to allow additionalProperties: true, in which case unspecified flattened parameters would then be passed through by a newer SDK.

@Benehiko
Copy link
Contributor

Benehiko commented Mar 30, 2023

I see, thank you for the link and clarification. Could you rebase your branch so we can have e2e tests run on this branch?

I will then review and see to merge this :)

@DASPRiD
Copy link
Contributor Author

DASPRiD commented Mar 30, 2023

Sure, I'll take care of that later tonight. Though what's your opinion on that manual unflattening? It does feel a little bit hacky, we could alternatively use the npm library flat to take care of generic unflattening.

It could look something like this then:

            const body = unflatten<unknown, UpdateBody>(
                Object.fromEntries(formData)
            );

@Benehiko
Copy link
Contributor

I will need to look into the implementation a bit, but yes I agree that using a library might be better if the library is well tested and maintained.

@aeneasr
Copy link
Member

aeneasr commented Jun 20, 2023

@Benehiko please revisit

@Benehiko Benehiko force-pushed the fix/unflatten-body branch 2 times, most recently from 9ff7260 to 7e6d179 Compare June 26, 2023 14:00
DASPRiD and others added 2 commits June 26, 2023 16:01
The format for bodies with traits dictates that the traits come in through their
own object. This becomes especially visible when using an SDK client compiled
with a newer version of OpenAPI generator, as they only process properties which
are defined in the API schema, which is not the case for flattened properties.

Fixes ory#93
@KimGressens
Copy link

I was having the same issue but when looking at the request being sent by the browser (when posting the registration form) I discovered that it never send out the traits.email properties.

Thus the request was:

{
  "csrf_token" : "...",
  "method": "password",
  "password": "..."
}

instead of

{
  "csrf_token" : "...",
  "method": "password",
  "password": "...",
  "traits.email": "..."
}

The reason traits.email gets stripped is due to the use of TypeScript (I think the other comments also use TS). The client library from Ory ( @ory/kratos-client-fetch ) validates the request and a traits object is supported but other keys such as traits.email are removed before sending the request.

As a workaround I'm going to implement an unflatten of the keys to make it fit the TypeScript definitions (as the linked PR is doing) but it's understandable that the maintainers might never accept that PR.

The best option is probably to fix the openapi spec that generates the @ory/kratos-client-fetch library but I'm not knowledgeable enough to know if it's even fixable on that level.

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.

UserAuthCard doesn't format the form body traits correctly
4 participants