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

[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 (https://github.com/nurpax/snaplet-sqlite-simple/issues/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 https://github.com/snapframework/snap/commit/1c8ad9bea476ddf51d609ba0f621de13fdde1bf2#commitcomment-1899898 - sounds good to me.

[mightybyte]

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]

It's already been merged into the 0.10 branch. https://github.com/snapframework/snap/commit/1c8ad9bea476ddf51d609ba0f621de13fdde1bf2

[adinapoli]

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

(https://github.com/snapframework/snap/commit/68f581c8928f846ce1ac0b0720937192a1c46557)

[ozataman]

@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

[adinapoli]

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

Edit: Done!

[mightybyte]

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 8f5c1d802d95 fixes this issue. OK to close this issue?