Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

createUser/registerUser should throw DuplicateLogin instead of punting the error checking to the backend #46

Closed
nurpax opened this Issue · 16 comments

5 participants

@nurpax

I realized snaplet-sqlite-simple has a bug in that it doesn't throw a DuplicateLogin exception if registerUser or createUser is called twice on the same login (nurpax/snaplet-sqlite-simple#3). I suspect this bug also exists in snaplet-postgresql-simple.

Further inspection seems to suggest that this would be better fixed in Snap.Snaplet.Auth so that all backends would automatically do this right.

Solution:

Check for duplicate logins in Snap.Snaplet.Auth.Handlers.createUser by calling usernameExists and throwing DuplicateLogin as appropriate.

PS. I may get around to sending a pull request for this sometime this week.

@gregorycollins

As discussed on the other thread, if you're in the mood to fix this, please just do the work on the 0.10 branch and change the API of createUser.

@adinapoli

Sorry if I chime in, but I've personally worked on that piece of code under the guidance of @mightybyte .
I can't get if @nurpax is proposing to remove the exception throwing from createUser (aka leave only Either and let the user handle the failure) or if he is suggesting to remove the Either in favor of an exception throwing. In the latter case, I suggest to stick on what mighty said or thinks about the issue :)

@nurpax

I'm fine either (no pun) way. Consistently using Either for duplicate logins and empty password, or exceptions for both. But not mixed exceptions and Either types.

@gregorycollins speaks for Either in 1c8ad9b#commitcomment-1899898 - sounds good to me.

@mightybyte
Owner

I also tend to prefer Either. Not sure where the DuplicateException came from since it was in the comment, but not in the code.

@nurpax

@mightybyte If you meant DuplicateLogin, it's used elsewhere though. One use in Snap.Snaplet.Auth.Backends.JsonFile

@nurpax

I can send a pull request once issue #30 gets merged - that will conflict. Unless @adinapoli wants to take a crack at it in his pending pull request? I think the Either return type should not be just String but something that can indicate both types of failure.

@mightybyte
Owner

It's already been merged into the 0.10 branch. 1c8ad9b

@adinapoli

To be precise, this is the most recent commit, aka the actual code in the 0.10 branch:

(68f581c)

@ozataman
Collaborator

@nurpax In any case, it seems like a good refactoring exercise to centralize the DuplicateLogin check. I'd use the lookupByLogin function instead of usernameExists, as it requires a smaller monadic context.

@adinapoli

At least in the Auth Snaplet, with my recent patch, we should have eliminated any throwing whatsoever. If you check the code and don't spot any other dark corner I might have missed, I think we can close this :)

@nurpax

Cool, I didn't realize these were already in.

Btw - I still see some mentions of the old exceptions in documentation:

./src/Snap/Snaplet/Auth/Handlers.hs:-- May throw a "DuplicateLogin" if given username is not unique.
./src/Snap/Snaplet/Auth/Handlers.hs:-- May throw a 'BackendError' if something goes wrong.
./src/Snap/Snaplet/Auth/Handlers.hs:-- May throw a 'BackendError' if something goes wrong.
./src/Snap/Snaplet/Auth/AuthManager.hs:-- May throw a "DuplicateLogin" if given username is not unique
./src/Snap/Snaplet/Auth/AuthManager.hs:-- Backend operations may throw 'BackendError's
@nurpax nurpax referenced this issue in nurpax/snaplet-sqlite-simple
Closed

update to the latest Snap 0.10 auth backend API #4

@adinapoli

@nurpax yep, the documentation is stale and I forgot to getting rid of it, I'll do the polishing asap :)

Edit: Done!

@mightybyte
Owner

Well, we should probably still do a DuplicateLogin check in createUser.

@nurpax

Yes, this is why this bug was filed :)

@adinapoli Are you planning on adding that fix too?

@adinapoli

@mightybyte @nurpax sure! I was carried away from the refactoring and I wasn't aware of the original issue :D
It doesn't seem to difficult to check for DuplicateLogin, so I'll definitely try to fix createUser.

@nurpax

@adinapoli Looks like commit 8f5c1d8 fixes this issue. OK to close this issue?

@mightybyte mightybyte closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.