Do not allow empty username and password when create user #30

Closed
freizl opened this Issue May 19, 2012 · 27 comments

Comments

Projects
None yet
7 participants
@freizl

freizl commented May 19, 2012

Turns out that Snap.Snaplet.Auth.Handlers.createUser allow empty username and password.
I think it is better to dis-allow it than ask API user to do that.

Thanks.

@ozataman

This comment has been minimized.

Show comment
Hide comment
@ozataman

ozataman Aug 1, 2012

Member

Good point. We leave validation to the API user, but we should certainly not allow blank username/password entries. We'll need to fix this.

Member

ozataman commented Aug 1, 2012

Good point. We leave validation to the API user, but we should certainly not allow blank username/password entries. We'll need to fix this.

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Aug 1, 2012

Member

I'm not so sure about this. It seems to me like there might be a case for empty passwords. Maybe not for usernames though.

Member

mightybyte commented Aug 1, 2012

I'm not so sure about this. It seems to me like there might be a case for empty passwords. Maybe not for usernames though.

@gregorycollins

This comment has been minimized.

Show comment
Hide comment
@gregorycollins

gregorycollins Aug 14, 2012

Member

Definitely not usernames.

Member

gregorycollins commented Aug 14, 2012

Definitely not usernames.

@Pedromdrp

This comment has been minimized.

Show comment
Hide comment
@Pedromdrp

Pedromdrp Sep 10, 2012

The minimum password length is being ignored/not used.

The minimum password length is being ignored/not used.

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 13, 2012

Member

Hi guys, sorry for the OT. I would like to contribute, but the quantity of code is overwhelming. I've forked the project and created a (probably naive) patch for this issue, but I'm not sure which is the right branch to commit into: 0.10 perhaps? What are the guidelines? I would also like to know why there is no trace for the createUser function inside the test sources. Is it ever tested? Sorry for polluting the discussion but I was unable to find this information elsewhere (and btw your google groups seems to have attracted spammers :( )

Cheers,
Alfredo

Member

adinapoli commented Sep 13, 2012

Hi guys, sorry for the OT. I would like to contribute, but the quantity of code is overwhelming. I've forked the project and created a (probably naive) patch for this issue, but I'm not sure which is the right branch to commit into: 0.10 perhaps? What are the guidelines? I would also like to know why there is no trace for the createUser function inside the test sources. Is it ever tested? Sorry for polluting the discussion but I was unable to find this information elsewhere (and btw your google groups seems to have attracted spammers :( )

Cheers,
Alfredo

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Sep 13, 2012

Member

If the patch does not change the external API at all, then put the change on master and I'll merge it into 0.10 afterwards. The Haskell PVP generally governs our versioning policy.

Member

mightybyte commented Sep 13, 2012

If the patch does not change the external API at all, then put the change on master and I'll merge it into 0.10 afterwards. The Haskell PVP generally governs our versioning policy.

@ozataman

This comment has been minimized.

Show comment
Hide comment
@ozataman

ozataman Sep 13, 2012

Member

And we would certainly be grateful if you were kind enough to add some tests while you're at it :-)

-Oz

On Thursday, September 13, 2012 at 9:51 AM, adinapoli wrote:

Hi guys, sorry for the OT. I would like to contribute, but the quantity of code is overwhelming. I've forked the project and created a (probably naive) patch for this issue, but I'm not sure which is the right branch to commit into: 0.10 perhaps? What are the guidelines? I would also like to know why there is no trace for the createUser function inside the test sources. Is it ever tested? Sorry for polluting the discussion but I was unable to find this information elsewhere (and btw your google groups seems to have attracted spammers :( )
Cheers,
Alfredo


Reply to this email directly or view it on GitHub (#30 (comment)).

Member

ozataman commented Sep 13, 2012

And we would certainly be grateful if you were kind enough to add some tests while you're at it :-)

-Oz

On Thursday, September 13, 2012 at 9:51 AM, adinapoli wrote:

Hi guys, sorry for the OT. I would like to contribute, but the quantity of code is overwhelming. I've forked the project and created a (probably naive) patch for this issue, but I'm not sure which is the right branch to commit into: 0.10 perhaps? What are the guidelines? I would also like to know why there is no trace for the createUser function inside the test sources. Is it ever tested? Sorry for polluting the discussion but I was unable to find this information elsewhere (and btw your google groups seems to have attracted spammers :( )
Cheers,
Alfredo


Reply to this email directly or view it on GitHub (#30 (comment)).

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Sep 13, 2012

Member

I second ozataman. In my mind the test suite is an underappreciated way to get familiar with Snap. This is why I included literate Haskell tutorials as part of the test suite for the new Heist functionality.

Member

mightybyte commented Sep 13, 2012

I second ozataman. In my mind the test suite is an underappreciated way to get familiar with Snap. This is why I included literate Haskell tutorials as part of the test suite for the new Heist functionality.

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 13, 2012

Member

It was on the cards to add tests for createUser, now even more :) From the little I've seen you test not only the pure code with Quickcheck / HUnit, but you also have sandboxes you use inside the test to simulate an entire "running" session of Snap, am I right? I'm referring to the code inside snap/test and the cabal-dev wizardry.

If I'm correct, I guess I should put createUser tests inside a new sandbox testing its behaviour, am I right?
Sorry for the newbie questions but as you already guessed this is my first "serious" project contribution :)

Cheers,
A.

Member

adinapoli commented Sep 13, 2012

It was on the cards to add tests for createUser, now even more :) From the little I've seen you test not only the pure code with Quickcheck / HUnit, but you also have sandboxes you use inside the test to simulate an entire "running" session of Snap, am I right? I'm referring to the code inside snap/test and the cabal-dev wizardry.

If I'm correct, I guess I should put createUser tests inside a new sandbox testing its behaviour, am I right?
Sorry for the newbie questions but as you already guessed this is my first "serious" project contribution :)

Cheers,
A.

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Sep 13, 2012

Member

Both the createUser and saveUser will probably need new checks--both for a non-empty username and that the new password meets the minimum length requirements. We have a minPasswordLen field in the AuthManager data structure, but currently we're not using it anywhere. This is clearly an oversight on our part and probably a good reason to change the external API to make createUser, saveUser, and probably also registerUser return a Maybe or Either String. I'm leaning towards Either String AuthUser because we probably want to provide an error message indicating the reason for the failure. Maybe ozataman can chime in here with any other thoughts.

Member

mightybyte commented Sep 13, 2012

Both the createUser and saveUser will probably need new checks--both for a non-empty username and that the new password meets the minimum length requirements. We have a minPasswordLen field in the AuthManager data structure, but currently we're not using it anywhere. This is clearly an oversight on our part and probably a good reason to change the external API to make createUser, saveUser, and probably also registerUser return a Maybe or Either String. I'm leaning towards Either String AuthUser because we probably want to provide an error message indicating the reason for the failure. Maybe ozataman can chime in here with any other thoughts.

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 13, 2012

Member

Perfect, it sounds reasonable! I'll dive into the code tomorrow morning, and I'll keep you posted :)

Member

adinapoli commented Sep 13, 2012

Perfect, it sounds reasonable! I'll dive into the code tomorrow morning, and I'll keep you posted :)

mightybyte added a commit that referenced this issue Sep 16, 2012

Fixed issues #30 and #38, and add test
Modified createUser to check for empty usernames.
Fixed content tags in project template.
@nurpax

This comment has been minimized.

Show comment
Hide comment
@nurpax

nurpax Sep 25, 2012

Contributor

Please see my comment 1c8ad9b#commitcomment-1899642 on error handling consistency. Is it ok to mix both exceptions (DuplicateLogin) and Either? I would argue that we shouldn't mix Either and exceptions. Throwing an exception also wouldn't break the existing API.

Contributor

nurpax commented Sep 25, 2012

Please see my comment 1c8ad9b#commitcomment-1899642 on error handling consistency. Is it ok to mix both exceptions (DuplicateLogin) and Either? I would argue that we shouldn't mix Either and exceptions. Throwing an exception also wouldn't break the existing API.

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Sep 25, 2012

Member

I'd go with Either unless ozataman had a specific reason for exceptions.

Member

mightybyte commented Sep 25, 2012

I'd go with Either unless ozataman had a specific reason for exceptions.

@ozataman

This comment has been minimized.

Show comment
Hide comment
@ozataman

ozataman Sep 25, 2012

Member

My reason here for using exceptions rather than Either was that I had imagined it would be hard to ascertain all potential exceptions a backend may raise ahead of time. Some exceptions can certainly be defined at the Auth level, like DuplicateLogin, but others can certainly be backend specific - like "MySQL server has gone away".

Also, using Either means that we would be forcing the API user to handle exceptions explicitly everywhere they may happen. I originally went with exceptions to keep things a bit more lax. Admittedly, that approach tends to stuff things under the carpet.

I'd support using Either as long as the change is made pervasively in the API. It's definitely not OK to mix the two as that'd make for a very confusing API. Essentially, someone needs to go in and change every mention of "throw" to something that returns an Either type.

I'd also suggest that we collect all possible save/update exceptions under a single sum type to use with Either instead of a String. The BackendError seems like a good candidate here; we can rename it as appropriate and add cases for empty/insufficient passwords, etc. BackendError String case can cover the unknown cases.

Should we also wrap every other possible backend exception with a catch and stuff it into the BackendError String case? This way these Handlers can never raise an exception, which hopefully would simplify the API.

Member

ozataman commented Sep 25, 2012

My reason here for using exceptions rather than Either was that I had imagined it would be hard to ascertain all potential exceptions a backend may raise ahead of time. Some exceptions can certainly be defined at the Auth level, like DuplicateLogin, but others can certainly be backend specific - like "MySQL server has gone away".

Also, using Either means that we would be forcing the API user to handle exceptions explicitly everywhere they may happen. I originally went with exceptions to keep things a bit more lax. Admittedly, that approach tends to stuff things under the carpet.

I'd support using Either as long as the change is made pervasively in the API. It's definitely not OK to mix the two as that'd make for a very confusing API. Essentially, someone needs to go in and change every mention of "throw" to something that returns an Either type.

I'd also suggest that we collect all possible save/update exceptions under a single sum type to use with Either instead of a String. The BackendError seems like a good candidate here; we can rename it as appropriate and add cases for empty/insufficient passwords, etc. BackendError String case can cover the unknown cases.

Should we also wrap every other possible backend exception with a catch and stuff it into the BackendError String case? This way these Handlers can never raise an exception, which hopefully would simplify the API.

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Sep 25, 2012

Member

Thanks for weighing in, Oz. I'm not categorically opposed to exceptions, but as I've said before I prefer Either or Maybe as a general rule. Switching all error conditions to Either BackendError a sounds good to me. I've already added a dependency on tekmo's awesome errors package, so we should definitely leverage that API to implement this stuff. It would be great if someone else could pick this up as I'm quite busy with finishing up Heist for the 0.10 release.

Member

mightybyte commented Sep 25, 2012

Thanks for weighing in, Oz. I'm not categorically opposed to exceptions, but as I've said before I prefer Either or Maybe as a general rule. Switching all error conditions to Either BackendError a sounds good to me. I've already added a dependency on tekmo's awesome errors package, so we should definitely leverage that API to implement this stuff. It would be great if someone else could pick this up as I'm quite busy with finishing up Heist for the 0.10 release.

@ozataman

This comment has been minimized.

Show comment
Hide comment
@ozataman

ozataman Sep 25, 2012

Member

I'm happy to chime in and review commits but have 0 cycles at the moment to do any of the work...

On Sep 25, 2012, at 1:03 PM, mightybyte notifications@github.com wrote:

Thanks for weighing in, Oz. I'm not categorically opposed to exceptions, but as I've said before I prefer Either or Maybe as a general rule. Switching all error conditions to Either BackendError a sounds good to me. I've already added a dependency on tekmo's awesome errors package, so we should definitely leverage that API to implement this stuff. It would be great if someone else could pick this up as I'm quite busy with finishing up Heist for the 0.10 release.


Reply to this email directly or view it on GitHub.

Member

ozataman commented Sep 25, 2012

I'm happy to chime in and review commits but have 0 cycles at the moment to do any of the work...

On Sep 25, 2012, at 1:03 PM, mightybyte notifications@github.com wrote:

Thanks for weighing in, Oz. I'm not categorically opposed to exceptions, but as I've said before I prefer Either or Maybe as a general rule. Switching all error conditions to Either BackendError a sounds good to me. I've already added a dependency on tekmo's awesome errors package, so we should definitely leverage that API to implement this stuff. It would be great if someone else could pick this up as I'm quite busy with finishing up Heist for the 0.10 release.


Reply to this email directly or view it on GitHub.

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 26, 2012

Member

With appropriate guidance I'll be happy to help :)

Member

adinapoli commented Sep 26, 2012

With appropriate guidance I'll be happy to help :)

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 27, 2012

Member

Should we put the new sum type under src/Snaplet/Auth/Types.hs or under a new file, namely
src/Snaplet/Auth/Backends/Types.hs ?

The latter seems to provide more semantic cohesion, at least in my opinion.

Member

adinapoli commented Sep 27, 2012

Should we put the new sum type under src/Snaplet/Auth/Types.hs or under a new file, namely
src/Snaplet/Auth/Backends/Types.hs ?

The latter seems to provide more semantic cohesion, at least in my opinion.

@ozataman

This comment has been minimized.

Show comment
Hide comment
@ozataman

ozataman Sep 27, 2012

Member

I would start by putting under Types for simplicity and we can later refactor if there is enough justification for it.

On Sep 27, 2012, at 5:30 AM, adinapoli notifications@github.com wrote:

Should we put the new sum type under src/Snaplet/Auth/Types.hs or under a new file, namely
src/Snaplet/Auth/Backends/Types.hs ?

The latter seems to provide more semantic cohesion, at least in my opinion.


Reply to this email directly or view it on GitHub.

Member

ozataman commented Sep 27, 2012

I would start by putting under Types for simplicity and we can later refactor if there is enough justification for it.

On Sep 27, 2012, at 5:30 AM, adinapoli notifications@github.com wrote:

Should we put the new sum type under src/Snaplet/Auth/Types.hs or under a new file, namely
src/Snaplet/Auth/Backends/Types.hs ?

The latter seems to provide more semantic cohesion, at least in my opinion.


Reply to this email directly or view it on GitHub.

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 28, 2012

Member

I began working on the refactoring. Consider this function, which has the old signature:

registerUser
  :: ByteString            -- ^ Login field
  -> ByteString            -- ^ Password field
  -> Handler b (AuthManager b) (Either String AuthUser)
registerUser lf pf = do
    l <- fmap decodeUtf8 <$> getParam lf
    p <- getParam pf
    case liftM2 (,) l p of
      Nothing         -> throw PasswordMissing
      Just (lgn, pwd) -> createUser lgn pwd

The refactoring should suggest registerUser to return a Either AuthFailure AuthUser, because the failure is at the "Auth" level. There are two problems, though:

a) registerUser throws a PasswordMissing exception, but this is against our new guideline; the problem can be fixed removing Exception and Error typeclasses from AuthFailure, and simply write the function like this:

registerUser
  :: ByteString            -- ^ Login field
  -> ByteString            -- ^ Password field
  -> Handler b (AuthManager b) (Either AuthFailure AuthUser)
registerUser lf pf = do
    l <- fmap decodeUtf8 <$> getParam lf
    p <- getParam pf
    case liftM2 (,) l p of
      Nothing         -> return $ Left PasswordMissing
      Just (lgn, pwd) -> createUser lgn pwd   --- Discordant exception type!

As you may notice, createUser has now the following signature:

createUser :: Text              -- ^ Username
           -> ByteString        -- ^ Password
           -> Handler b (AuthManager b) (Either BackendError AuthUser)

This is due to the fact createUser relies on the backend, which can fail, so it does make sense to return that type of error. What do you suggest? It seems like we can merge the two data types or at least provide a guideline here about how to redeem the question.

Sorry for the lengthy comment.

Member

adinapoli commented Sep 28, 2012

I began working on the refactoring. Consider this function, which has the old signature:

registerUser
  :: ByteString            -- ^ Login field
  -> ByteString            -- ^ Password field
  -> Handler b (AuthManager b) (Either String AuthUser)
registerUser lf pf = do
    l <- fmap decodeUtf8 <$> getParam lf
    p <- getParam pf
    case liftM2 (,) l p of
      Nothing         -> throw PasswordMissing
      Just (lgn, pwd) -> createUser lgn pwd

The refactoring should suggest registerUser to return a Either AuthFailure AuthUser, because the failure is at the "Auth" level. There are two problems, though:

a) registerUser throws a PasswordMissing exception, but this is against our new guideline; the problem can be fixed removing Exception and Error typeclasses from AuthFailure, and simply write the function like this:

registerUser
  :: ByteString            -- ^ Login field
  -> ByteString            -- ^ Password field
  -> Handler b (AuthManager b) (Either AuthFailure AuthUser)
registerUser lf pf = do
    l <- fmap decodeUtf8 <$> getParam lf
    p <- getParam pf
    case liftM2 (,) l p of
      Nothing         -> return $ Left PasswordMissing
      Just (lgn, pwd) -> createUser lgn pwd   --- Discordant exception type!

As you may notice, createUser has now the following signature:

createUser :: Text              -- ^ Username
           -> ByteString        -- ^ Password
           -> Handler b (AuthManager b) (Either BackendError AuthUser)

This is due to the fact createUser relies on the backend, which can fail, so it does make sense to return that type of error. What do you suggest? It seems like we can merge the two data types or at least provide a guideline here about how to redeem the question.

Sorry for the lengthy comment.

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Sep 28, 2012

Member

I guess we should probably collapse BackendError and AuthFailure into a single type that we use everywhere. That gives us slightly less type safety, but seems better in terms of simplicity. Any objections Oz?

Member

mightybyte commented Sep 28, 2012

I guess we should probably collapse BackendError and AuthFailure into a single type that we use everywhere. That gives us slightly less type safety, but seems better in terms of simplicity. Any objections Oz?

@ozataman

This comment has been minimized.

Show comment
Hide comment
@ozataman

ozataman Sep 28, 2012

Member

Yeah, this idea had occurred to me too but I went with type safety at the time since we were relying on exceptions anyway. Combining them sounds OK to me too; no need to go crazy with types here.

Also while we're at it, can we define a manual Show instance for AuthFailure? Something that the average web app author can feel comfortable showing the user signing up. So that

show DuplicateUser = "This user already exists; please choose a different username."

Otherwise, I see web app authors by hand defining a function:

humanReadableAuthError :: AuthFailure -> String

On Friday, September 28, 2012 at 11:38 AM, mightybyte wrote:

I guess we should probably collapse BackendError and AuthFailure into a single type that we use everywhere. That gives us slightly less type safety, but seems better in terms of simplicity. Any objections Oz?


Reply to this email directly or view it on GitHub (#30 (comment)).

Member

ozataman commented Sep 28, 2012

Yeah, this idea had occurred to me too but I went with type safety at the time since we were relying on exceptions anyway. Combining them sounds OK to me too; no need to go crazy with types here.

Also while we're at it, can we define a manual Show instance for AuthFailure? Something that the average web app author can feel comfortable showing the user signing up. So that

show DuplicateUser = "This user already exists; please choose a different username."

Otherwise, I see web app authors by hand defining a function:

humanReadableAuthError :: AuthFailure -> String

On Friday, September 28, 2012 at 11:38 AM, mightybyte wrote:

I guess we should probably collapse BackendError and AuthFailure into a single type that we use everywhere. That gives us slightly less type safety, but seems better in terms of simplicity. Any objections Oz?


Reply to this email directly or view it on GitHub (#30 (comment)).

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 28, 2012

Member

Yep, collapsing was a thing I considered, so I'll stick with it!
@ozataman sure I can :)

Member

adinapoli commented Sep 28, 2012

Yep, collapsing was a thing I considered, so I'll stick with it!
@ozataman sure I can :)

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 29, 2012

Member

@ozataman Thinking more deeply about that I spot a nuisance: defining the show author that way will hardcode the error message to the English language, which is no good for site that needs internationalization.
I don't know the support that Snap has for internationalization, but we should definitely rely on it or make the mechanism scalable and easily adaptable to any context. What you reckon?

Member

adinapoli commented Sep 29, 2012

@ozataman Thinking more deeply about that I spot a nuisance: defining the show author that way will hardcode the error message to the English language, which is no good for site that needs internationalization.
I don't know the support that Snap has for internationalization, but we should definitely rely on it or make the mechanism scalable and easily adaptable to any context. What you reckon?

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Sep 29, 2012

Member

That's why we are using Either AuthFailure a instead of Either String a. The latter would lock them in to a particular error message. A Show instance for AuthFailure doesn't lock them into anything. It just provides a nice sensible default. If they need internationalization, then they'll just define their own showFailure function.

Member

mightybyte commented Sep 29, 2012

That's why we are using Either AuthFailure a instead of Either String a. The latter would lock them in to a particular error message. A Show instance for AuthFailure doesn't lock them into anything. It just provides a nice sensible default. If they need internationalization, then they'll just define their own showFailure function.

@adinapoli

This comment has been minimized.

Show comment
Hide comment
@adinapoli

adinapoli Sep 29, 2012

Member

Yep, although I think we must be very careful not to call any show AuthFailure inside our Auth snaplet, therefore dooming foreign users to hack the internals in order to overwrite the behavior :)

ps. Just to be sure, have we choosen a type yet? Who will triumph between AuthFailure and BackendError ?

Member

adinapoli commented Sep 29, 2012

Yep, although I think we must be very careful not to call any show AuthFailure inside our Auth snaplet, therefore dooming foreign users to hack the internals in order to overwrite the behavior :)

ps. Just to be sure, have we choosen a type yet? Who will triumph between AuthFailure and BackendError ?

@mightybyte

This comment has been minimized.

Show comment
Hide comment
@mightybyte

mightybyte Sep 29, 2012

Member

Correct.

Member

mightybyte commented Sep 29, 2012

Correct.

adinapoli added a commit to adinapoli/snap that referenced this issue Sep 30, 2012

adinapoli added a commit to adinapoli/snap that referenced this issue Sep 30, 2012

mightybyte added a commit that referenced this issue Sep 30, 2012

Merge pull request #47 from adinapoli/0.10
[0.10] Done type refactoring as discussed in issue #30

@mightybyte mightybyte closed this Oct 8, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment