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

yesod-auth: Email provider broke after PR #1317 #1330

Closed
CthulhuDen opened this issue Jan 7, 2017 · 19 comments

Comments

@CthulhuDen
Copy link
Contributor

commented Jan 7, 2017

After trying to parse JSON with parseJsonBody or requireJsonBody request body is consumed so POST parameters cannot be read (look at demo PR #1331), so unconditionally trying to mix processing request as both JSON and url-encoded string breaks things for whatever option was tried second. I suggest modifying code in #1317 so that it checks Content-Type first and interprets input in appropriate way (so do not try to parse JSON if it is not application/json).

@psibi

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

What about the other way round ? Try reading some POST parameters and then JSON parsing in request body. Does that work or does it have the same problems ?

@CthulhuDen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2017

@psibi

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

@CthulhuDen Ah, okay. But this code in Yesod.Auth.Email works for me with the latest library: https://github.com/yesodweb/yesod-cookbook/blob/master/cookbook/yesod-auth-json.md

Any idea why that works ? The JSON interface should actually not work.

@CthulhuDen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2017

@psibi I may be getting something wrong, but I doubt it can work at all. I was able to use register button and confirmed my email and also use logout link, but nothing else worked: user record is created without password, class method needOldPassword overridden to always return True (yesodweb/yesod-cookbook@76ee43b#diff-2ec8e63a30fc040c7268b526e126fef1R155) so I cannot event set my password.

@CthulhuDen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2017

On second thought, asking for POST parameter then parsing as JSON works iff request header "Content-Type" is correctly set to JSON, because in this case WAI does not even try to parse body for input parameters. Perhaps it would be a good idea to make parseJsonBody likewise only parse body for correct content-type, or perhaps for any excluding www-form-urlencoded and multipart/form-data.

Ping @snoyberg

@psibi

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

@CthulhuDen Execute that as a stack script. This is the relevant code I have used at the start of the file:

#!/usr/bin/env stack
{- stack
     --resolver lts-6.15
     --install-ghc
     runghc
     --package yesod
     --package yesod-auth-1.4.15
     --package text
     --package http-conduit
     --package persistent
     --package persistent-sqlite
     --package shakespeare
     --package monad-logger
 -}

Then the workflow there is like this:

  • Click register button first. In your terminal, you will get a url printed. Go to that particular url to set new passwordl.
  • After that you can use the other buttons to do login, set password etc.

The above flow works for me. Can you see if it works for you ?

@psibi

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

Also change the needOldPassword to False.

@psibi

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

@CthulhuDen I'm agreeing with the fact that the current code needs to be changed so that we read parameters based on the content type. But right now the Email interface is working for me which is confusing me. In fact, we have a internal service running based on the json endpoints of Yesod.Email.

@CthulhuDen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2017

@psibi After changing needOldPassword it will probably work, agreed. So basically JSON flow works (because correct Content-Type prevents WAI from parsing into input fields) but normal HTML forms do not. I think parseJsonBody should check Content-Type as I wrote earlier.

@psibi

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

@CthulhuDen Yeah, I agree. Can you send a PR fixing the module ?

If not, I will fix this by next week.

@CthulhuDen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2017

@psibi If you mean fix in Yesod.Auth.Email, I'm afraid I won't be able to take care of that, and I would like to get some opinions on whether we should fix parseJsonBody instead.

@psibi

This comment has been minimized.

Copy link
Member

commented Jan 7, 2017

Okay. :-) Let's wait and see if @snoyberg and others have a opinion on fixing the parseJsonBody instead. IMO, if we decide to fix parseJsonBody itself - then I think it will break lots of Yesod handlers in the wild.

@snoyberg

This comment has been minimized.

Copy link
Member

commented Jan 8, 2017

I definitely think doing the form parsing before JSON parsing is a good, simple fix here. And we should have a function to parse a JSON body only if the content-type indicates JSON.

As far as changing the behavior of the existing functions? Probably not. I'd be OK with adding a new function, and putting out an email to the Yesod mailing list asking if we should deprecate the old one for confusing behavior.

@psibi

This comment has been minimized.

Copy link
Member

commented Jan 8, 2017

I think right now we are doing the form parsing before JSON parsing. So, we are good there.

And we should have a function to parse a JSON body only if the content-type indicates JSON.

I think it would be good to have it. I know I will be using them a lot.

@CthulhuDen

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2017

I think right now we are doing the form parsing before JSON parsing. So, we are good there.

Not in every method, at least not in password setting.

snoyberg added a commit that referenced this issue Feb 2, 2017
@snoyberg

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

I've just pushed a commit to master which I believe will address this, but I haven't tested it myself. Can someone who has been working through these issues give it a shot? If I get the thumbs up, I can make a new release of yesod-core and yesod-auth.

@psibi

This comment has been minimized.

Copy link
Member

commented Feb 2, 2017

I will verify it tonight and report back. Thanks for the fix.

@connrs

This comment has been minimized.

Copy link

commented Feb 2, 2017

Hey @snoyberg, thanks for tackling this for us.

I pulled this in to the project I've been working on (as mentioned in this Google Groups topic) and can confirm that this certainly resolves the first-time-set-password issue that I'd encountered.

@snoyberg

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

Awesome, thanks for confirming. Closing with new release to Hackage.

@snoyberg snoyberg closed this Feb 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.