nhost / hasura-auth

Authentication for Hasura.
https://nhost.io
MIT License
386 stars 114 forks source link

Oauth Provider SignIn Metadata not parsed as JSON #357

Closed im-what-im closed 9 months ago

im-what-im commented 1 year ago

While passing the metadata attribute in the query param for the Provider SignIn usecase, the metadata is stored as it is in the metadata column. No documentation is available for the structure in which metadata is to be passed. Tried with: /provider/{type}?metadata={"key": "value"} this got saved in the DB as "{\"key\":\"value\"}"

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

dbarrosop commented 1 year ago

Apologies, this one fell through the cracks.

It isn't documented because the metadata is entirely up to you. We don't use that column at all, we just have it there so you can add any additional information you see fit.

spakanati commented 1 year ago

I think this issue should be reopened. In other auth methods (sms, email/password, etc.) the metadata field is supported and when used it saves the contents as jsonb. In the flutter and js clients, metadata is assumed to be a dictionary. The presence of the field and how to pass a jsonb dictionary correctly should be documented for provider sign-in as well (while understanding that the contents of the field are up to the user). I can confirm from testing that we also see that the metadata field is passed through when used with provider signin, but the field is saved as a string instead of jsonb (unlike how sms, email/pass, and so on work). I'm not sure of the best parameter input format since I think the issue is caused by metadata being sent as a query parameter instead of a json request body. Perhaps metadata can be expected to be serialized JSON and nhost can deserialize (looks like it should be done here) before saving to the DB? Theoretically a string is valid jsonb, but nhost has typed metadata as a dict in various clients' auth methods already so it seems safe to assume a dict.

dbarrosop commented 1 year ago

I think the issue is caused by metadata being sent as a query parameter instead of a json request body

Yes, looking at the original issue it looks like I misunderstood it. We should deserialize the query arg as a json object as they will always be received as a string. The main issue is that if a user wants a literal string they should urlencdode it, for instance metadata=%22this%20is%20s%20a%20literal%20string%22 for "this is a literal string", which may be a breaking change but I think that given this is clearly a bug as it doesn't behave like the rest of the SDKs/methods we should go with it.

spakanati commented 9 months ago

Hey @onehassan looks like you might be working on this one but anything we can do to help? We'd love to be able to get rid of the workaround Postgres triggers we had to create to get around this bug. Also, the bug currently breaks nhost-dart entirely when signing in with Oauth and using metadata because User.fromJson can't handle metadata incorrectly being a string. Should I file a separate nhost-dart issue about that? (I hadn't done so because it seemed reasonable that the dart client doesn't deserialize unexpected data structures, and it seemed like it'd make more sense just to fix this bug in hasura-auth instead.)

dbarrosop commented 9 months ago

458 should fix this issue