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

A new module (RestUtils.hs) along with a rest authenticator #4

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

adinapoli
Copy link
Contributor

Hello Oz,

days ago I had the need of writing a rest authenticator (just a fancy name for an handler combinator) to be able to "access" some routes just if "rest authenticated" (e.g. if I had a particular token in my http header). I showed the code to Doug, which seemed to like it. I've integrated a bunch of his suggestions and created this.

I thought the best place is a separate module, but I'm free to change my mind if you don't feel the need of a new specialised module. The idea is to gather everything regarding rest utilities.

Using the authenticator is trivial; it's an handler combinator, so it yields an Handler, which means can be integrated with existing combinators we have. I've attached plenty of documentation in the patch which should really make the code and the rationale behind it straightforward.

Feel free to give me feedback.
I hope it's something you might find useful as well :)

Greetings from a (very hot) Rome,

Alfredo

@ozataman
Copy link
Owner

Hi Alfredo,

First, thank you for the effort and submitting this. I just took a look at this and my main comment is that there is very little complexity being abstracted away by this interface and I'm afraid it obscures more than it abstracts/helps the developer.

It looks like if I want a token-enabled authentication scheme, all I have to do is to chain a simple "token authenticator", which is likely to be app-specific anyway, to my to-be-protected routes. In fact, I have done this a few times before using just a few lines like so:

-- | Authenticate API user in
authApi = do
  user <- T.decodeUtf8 `liftM` reqParam "user"
  token <- T.decodeUtf8 `liftM` reqParam "apiToken"
  u <- runPersist' $ selectFirst [Login == user, Token == token] []
  case u of
    Nothing -> badReq "Authentication failed."
    Just u' -> with auth $ forceLogin u'

I can then pre-chain authApi to any route point where it is necessary and it will nicely log in (using Snap's auth) people who present the right token.

Because it is so stateless, the code needed ends up being very simple and therefore flexible. Enforcing a sophisticated API here feels like an unnecessary straightjacket to me. However, I'll see if there's any value to be captured from making my above function a little more generic and perhaps putting that into snap-extras so that the case of "using a token to let users through" is covered out-of-the-box.

Given a way to resolve to a user from the Handler context using headers, tokens, whatever, we log the person in and remember the decision through Snap's auth system:

authenticateToken :: Handler b v (Maybe AuthUser) -> Handler b v (Maybe AuthUser)

this could first check if the user's already logged in and not fail if that's the case, etc.

@adinapoli
Copy link
Contributor Author

Hi Oz,

thanks for looking into my PR. Let me elaborate on that, and I will reply with more calm.
The fact is that I've slightly changed that code and I'm using a slightly enhanced version in production that I find advantageous because is not tied to any specific persistence layer (like your example).
If even that version will satisfy you, I will then be to sleep thigh at night, knowing I've submitted every viable option :)

@ozataman
Copy link
Owner

My example is actually not at all tied to ANY persistence layer, which basically is my point. In just 5 lines, it is very flexible: the with auth $ forceLogin u' part can simply be replaced with a return (), amounting to a ' let the request through' when authentication succeeds. The lookups can easily be replaced by header lookups if needed by the app, the db lookup can be replaced by some ldap/whatever lookup if needed, etc.

If I start a new app tomorrow that needs something like this, all I have to do is copy/paste and then tweak this 5-6 lines of code :-)

Still, I'm happy to see your current solution - maybe I'm missing something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants