Basic authentication stuff #117

Open
wants to merge 2 commits into from

3 participants

@carlssonia

To support user logging in snap-server (upcoming pull request).

@gregorycollins

Thanks for tackling this. First of all: I think this code should go into a new module Snap.Util.HttpAuthentication' rather than getting lumped intoSnap.Core`.

Secondly: I'm uncomfortable with providing basic authentication out of the box without any disclaimers/warning labels/restrictions on usage. Could you please change this API to:

withBasicAuthentication :: MonadSnap m => ((ByteString, ByteString) -> m a) -> m a

and check that rqIsSecure is True within the handler. I really don't think we want to make it easy for people to send cleartext passwords out over the internet in our public API. A suitable disclaimer in the comment (that we will only authenticate secure connections using this method).

If you really feel that cleartext basic authentication is something that needs to be in the API, then add the function as withUnsafeBasicAuthentication with a big disclaimer in the docstring. Personally I think we're better off just omitting it.

I think the thing we all would really like here (and I think hvr mentioned it also) is RFC 2617 digest authentication. Would you be interested in tackling that?

Restricting the API to secure connections is a good idea!

Would you be OK with skipping the continuation-passing style and simply have something like

getBasicAuthentication :: MonadSnap m => m (Maybe (ByteString, ByteString))

where the function would return Nothing in case of insecure connection, or missing/invalid header? That would be much easier to use, I think.

Regarding digest authentication: I'm afraid I don't have a need for that at the moment...

@gregorycollins
Snap Framework member

Hi,

Re: the continuation passing style -- I think it's cleaner to do it that way, personally. When implementing this stuff, you are always going to need a function like:

checkBasicAuthenticationCredentials :: MonadSnap m => (ByteString, ByteString) -> m ()

If you just make the type

getBasicAuthenticationCredentials :: MonadSnap m => m (ByteString, ByteString)

then you are still going to need to check what they gave you somehow anyways. One of the reasons I like "with" style here is that it's clear to the user what he's expected to provide to use the API.

Another thing I'd say is that if we do it your way, you don't want the return type to be a "Maybe" here, just use mzero if the authentication process fails.

@meiersi meiersi added a commit to meiersi/snap-core that referenced this pull request Apr 11, 2013
@meiersi meiersi Fix #117: compatible Eq and Ord instances for Method. 3cf5b9d
@meiersi

Hi @gregorycollins , I think this branch was accidentally closed because of my mistake in the in the issue number reference.

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