asafdav / hapi-auth-extra

Additional auth toolbox for HapiJS including ACL support
MIT License
21 stars 34 forks source link

Status of plugin? #5

Open AdrieanKhisbe opened 8 years ago

AdrieanKhisbe commented 8 years ago

Is plugin development still active, or had this been superseeded by the fork https://github.com/toymachiner62/hapi-authorization ?

asafdav commented 8 years ago

I wasn't aware of the forks, I'd love to see some pull requests and merge them in.

AdrieanKhisbe commented 8 years ago

Oh, I'm a bit surprised. Then will be usefll to see if there a real divergence or now.

If hapi-authorization has changed of purpose.

@toymachiner62, would you mind joining the discussion ? :smiley:

toymachiner62 commented 8 years ago

Sure. When i was looking for authorization for my hapi app, I believe that hapi-auth-extra didn't provide all the functionality that we needed. So i created a fork and made hapi-authorization strictly for authorization and left authentication separately since we were using an internal authentication strategy.

I typically prefer single use libraries rather than a monolithic library so that's why hapi-authorizationis strictly authorization and does not do anything with authentication (especially since there already exist other authentication libs for hapi).

At least that's how i'm remembering the reasons why I forked hapi-authorization..

AdrieanKhisbe commented 8 years ago

I dig deeper into the two code source, running a diff.

The two main change are:

@toymachiner62, tell me if you think I'm mistaken


To me a convergence could be made, by reimplemnting a role hierarchy system. (which could really be useful), and by renaming original @asafdav repositery. hapi-authorization being more name evocation that auth-extra

@asafdav @toymachiner62, what do you think? :)

asafdav commented 8 years ago

Sounds good to me, what do you think @toymachiner62 ?

toymachiner62 commented 8 years ago

Sorry, but what's the actual question you're asking of me?

AdrieanKhisbe commented 8 years ago

@toymachiner62. I was asking to confirm the two main changes I highlighted,

removal of auth.js (delegated to other plugin, such as hapi-auth-basic cf example disabling of system of role hierarchy : cf https://github.com/toymachiner62/hapi-authorization/blob/master/lib/roles.js#L22-L27

And if you were agreeing to the convergence suggestion :smiley:

To me a convergence could be made, by reimplemnting a role hierarchy system. (which could really be useful), and by renaming original @asafdav repositery. hapi-authorization being more name evocation that auth-extra

toymachiner62 commented 8 years ago

I don't think i removed the role hierarchy. https://github.com/toymachiner62/hapi-authorization/blob/master/lib/roles.js#L13-L20

So does your proposition of merging my code back mean that @asafdav is going to drop the authentication stuff from his code?

I (and probably most other devs) prefer simple isolated packages rather than monolithic packages that perform multiple functions.

AdrieanKhisbe commented 8 years ago

So does your proposition of merging my code back mean that @asafdav is going to drop the authentication stuff from his code?

Yep, that was implicitely implied. Auth should be made with it's own module. https://github.com/dwyl/hapi-auth-jwt2 for instance

I don't think i removed the role hierarchy. https://github.com/toymachiner62/hapi-authorization/blob/master/lib/roles.js#L13-L20

Indeed, I wasn't attentive enough. (hierarchy was more thinking me of a tree structure, that just a clearance level

toymachiner62 commented 8 years ago

@asafdav Are you in agreement with the above https://github.com/asafdav/hapi-auth-extra/issues/5#issuecomment-165518086?

asafdav commented 8 years ago

@toymachiner62 Yes, we will have to create a migration plan for existing users but I agree with you, it would be better this way.

AdrieanKhisbe commented 8 years ago

:fireworks: :)