stormpath / express-stormpath

Build simple, secure web applications with Stormpath and Express!
http://docs.stormpath.com/nodejs/express/
Apache License 2.0
325 stars 111 forks source link

Discuss: loginRequired and apiAuthenticationRequired #173

Open robertjd opened 9 years ago

robertjd commented 9 years ago

I feel like these middleware function's don't accurately describe what's going on under the hood.

I feel that loginRequired would be more appropriately named cookieAuthenticationRequired

And apiAuthenticationRequired should be broken out into oauthAuthenticationRequired and basicAuthenticationRequired

Further, I think we should create a parent middleware, authenticate which determines which authentication strategy to use.

rdegges commented 9 years ago

I'd like to see:

timothyej commented 8 years ago

This is really nice! :+1:

But regarding the names, how about something like this instead:

Maybe too long, but I don't like the ...AuthenticationRequired on all middlewares, would prefer a namespace instead :)

rdegges commented 8 years ago

Hm.. I'm torn. Cause I like namespacing in some cases too, but I never see people do it this way in Node, feels really java-esque :(

robertjd commented 8 years ago

I don't think the namespace is required at this time. P.S @timothyej :)

I'm on board with the list by @rdegges - but with the small change that oauthAuthenticationRequired should be oauth2AuthenticationRequired (I inadvertently omitted the 2 in my original comment)

rdegges commented 8 years ago

+1 for that

typerandom commented 8 years ago

+1

But what do you think about:

Personally for me this is easier to remember (and aligns more nicely) since they all start with require. Also feels more assertive, and doesn't it make more sense with require since required is past tense?

edjiang commented 8 years ago

I like this list, it's definitely a lot clearer than loginRequired and apiAuthenticationRequired

robertjd commented 8 years ago

I'm also liking the require prefix.

New question: what to do with our getUser middleware? This middleware is essentially requireAuthentication, but without the require part :) For all of these proposed authenticators, I can see how it would be useful to resolve the user, via the specified means of authentication, but not 401/error if the resolution cannot be done. Just call next() instead.

timothyej commented 8 years ago

How about requireUser? ;)

edjiang commented 8 years ago

How often do you think that people need to request a specific form of auth, vs. a user in general? If the 80% use case is resolving a user, then I'd suggest:

getUser requireUser requireApiAuthentication etc