travist / flatiron-passport

A Passport.js integration with the Flatiron.js web framework.
10 stars 5 forks source link

Update flatiron-passport.js #7

Closed vanthome closed 11 years ago

vanthome commented 11 years ago

Hi, please have a look a this PR. It enhances 2 things: 1.) If you don't use sessions, you can now turn it off by app.use(fipassport, {session: false});

2.) If you have auth schemes which involve more than on handler like an OAuth token endpoint for example, you need to consider the callback of flatiron and call it accordingly; otherwise only the first handler will get called.

pls. accept if you like.

Thanks, Thomas

travist commented 11 years ago

Please update your commit message to be more descriptive of what actually changed in this pull request.

You can do this using git rebase -i origin/master pretty easily and then just reword that commit message.

Other than that, this looks pretty good since even if the session setting is undefined, this will still maintain existing behavior.

vanthome commented 11 years ago

Ok, I try this way so that everyone can read this thread. I guess the session thing is clear so I'll detail the second part:

Generally there is the possibility to have several functions executed for a sing route. Unfortunately, this feature is barely documented in flatiron. For complex auth schemes like OAuth in this example app:

https://github.com/jaredhanson/oauthorize/tree/master/examples

You see that you will need several handlers. I found this is possible in flatiron like this:

      var routes = {'/oauth2/token': {
                    post: [
                           function(cb) {
                               console.log('1');
                              cb();
                              this.res.emit('next');
                           },
                           function(cb) {
                                console.log('2');
                               cb();
                               this.res.emit('next');
                            },                          
                           function(cb) {
                               console.log('3');
                              cb(false);
                              this.res.emit('next');

                           }                                             
                       ]
                  }           
            }        
            app.router.mount(routes); // Insert this  routing table into the application wide table

I've resembled this in fi-passport.

pvorb commented 11 years ago

@travist Is something still missing before you'll accept?

travist commented 11 years ago

No... looks good to me. Thanks for the update.

vanthome commented 11 years ago

Can you please push this to npm?

travist commented 11 years ago

Done. Sorry about that.