elements icon indicating copy to clipboard operation
elements copied to clipboard

fix: unflatten form data into correct body format

Open DASPRiD opened this issue 2 years ago • 7 comments

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

  • [x] I have read the contributing guidelines and signed the CLA.
  • [x] I have referenced an issue containing the design document if my change introduces a new feature.
  • [x] I have read the security policy.
  • [x] 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 avatar Mar 20 '23 22:03 DASPRiD

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.

Benehiko avatar Mar 30 '23 09:03 Benehiko

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 https://github.com/ory/sdk/pull/256#issuecomment-1473892227), 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.

DASPRiD avatar Mar 30 '23 11:03 DASPRiD

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 :)

Benehiko avatar Mar 30 '23 11:03 Benehiko

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)
            );

DASPRiD avatar Mar 30 '23 12:03 DASPRiD

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.

Benehiko avatar Mar 30 '23 12:03 Benehiko

@Benehiko please revisit

aeneasr avatar Jun 20 '23 05:06 aeneasr

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.

KimGressens avatar Sep 06 '24 09:09 KimGressens