Some updates to Mojolicious::Plugin::Authentication

[ewildgoose]

Hi, can you have a look at the latest 4 commits in my linked repo and see what you think? The main changes are driven based on the fairly mature rails auth plugins:

• Introduce a current_user method - almost everything can then be defined based on that
• Rename the user stash and user method to current_user... this is controversial and breaks existing code...
• Rename user_exists to is_user_authenticated to better match what it actually does. Can't think of a good new name for signature_exists though...
• lazy is renamed autoload_user to better represent what it does. Code simplified as a result.

Thanks

Ed W

[benvanstaveren]

Hi Ed,

I do like it, except the breaking existing code part, but I think there's a way around that, just going to take two releases so people have some time to watch the deprecation warnings and then deprecate the old names/functionality. Currently up to my eyeballs in $dayjob, so I'll probably roll up a release this weekend :)

On 02/28/2012 02:57 AM, Ed Wildgoose wrote:

Hi, can you have a look at the latest 4 commits in my linked repo and see what you think? The main changes are driven based on the fairly mature rails auth plugins:

• Introduce a current_user method - almost everything can then be defined based on that
• Rename the user stash and user method to current_user... this is controversial and breaks existing code...
• Rename user_exists to is_user_authenticated to better match what it actually does. Can't think of a good new name for signature_exists though...
• lazy is renamed autoload_user to better represent what it does. Code simplified as a result.

Thanks

Ed W

You can merge this Pull Request by running:

git pull https://github.com/ewildgoose/Mojolicious-Plugin-Authentication master

Or you can view, comment on it, or merge it online at:

https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4

-- Commit Summary --

• Change ugly API
• Add some comments
• The lazy parameter is changed to autoload_user to become a simple hook which autoloads the user into the stash for us
• Introduce a current_user function - dramatically simplifies code and almost everything else can be implemented using it.

-- File Changes --

M lib/Mojolicious/Plugin/Authentication.pm (75) M t/01-functional.t (3) M t/02-functional_lazy.t (4)

-- Patch Links --

https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4.patch https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4.diff

---

Reply to this email directly or view it on GitHub: https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4

[ewildgoose]

Hi, I added two new commits, please take a look at my master branch.

First reverts the change of stash key name (it's already under {authentication}->{} so really it can be anything we like

Second makes the current_user helper name be a configurable variable.
It still defaults to 'current_user', which I know will cause distress, but it's a very common idiom in the rails world and it also keeps the root namespace clear - I can easily think of plenty of things that could be confused with the "$app->user()" method...

I'm also itching to push a third commit which makes the autoload_user default to 0... I initially defaulted to 1 because I thought it would load the user into some easy to access attribute of the stash (but it's not really). I then thought that it could load it into a second alias in the stash of say $current_user, but that's really redundant given that the current_user() method is available in any view... So really I see no benefit in autoloading the stash given that the main interface is via the "current_user()" helper method. This nails down the public interface and dissuades people from touching the private stash methods and keeps everything clean. Thoughts?

...Actually I talked myself into commit three whilst writing this email. autoload_user should be considered deprecated and considered for removal (this is equivalent to making lazy=1 the default in the old code

• however, we have removed all references to lazy except a before_dispatch hook which basically calls a private method slightly earlier than our public helper calls the same method - this seems to have little benefit unless some user is hacking with the private stash values rather than going through the public interface... Those users can simply add their own hook, bridge or under, which just calls current_user in a void context to achieve exactly the same effect)

I think this allows you to pull that branch now with only a minor complaint (ie back compatibility) about the default name of current_user... I feel fairly strongly about the name change, so going to push for it...

Cheers

Ed W

On 27/02/2012 23:43, Ben van Staveren wrote:

Hi Ed,

I do like it, except the breaking existing code part, but I think there's a way around that, just going to take two releases so people have some time to watch the deprecation warnings and then deprecate the old names/functionality. Currently up to my eyeballs in $dayjob, so I'll probably roll up a release this weekend :)

On 02/28/2012 02:57 AM, Ed Wildgoose wrote:

Hi, can you have a look at the latest 4 commits in my linked repo and see what you think? The main changes are driven based on the fairly mature rails auth plugins:

• Introduce a current_user method - almost everything can then be defined based on that
• Rename the user stash and user method to current_user... this is controversial and breaks existing code...
• Rename user_exists to is_user_authenticated to better match what it actually does. Can't think of a good new name for signature_exists though...
• lazy is renamed autoload_user to better represent what it does. Code simplified as a result.

Thanks

Ed W

You can merge this Pull Request by running:

git pull https://github.com/ewildgoose/Mojolicious-Plugin-Authentication master

Or you can view, comment on it, or merge it online at:

https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4

-- Commit Summary --

• Change ugly API
• Add some comments
• The lazy parameter is changed to autoload_user to become a simple hook which autoloads the user into the stash for us
• Introduce a current_user function - dramatically simplifies code and almost everything else can be implemented using it.

-- File Changes --

M lib/Mojolicious/Plugin/Authentication.pm (75) M t/01-functional.t (3) M t/02-functional_lazy.t (4)

-- Patch Links --

https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4.patch
https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4.diff

---

Reply to this email directly or view it on GitHub: https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4

---

Reply to this email directly or view it on GitHub: https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4#issuecomment-4206215

[benvanstaveren]

Hi Ed,

I'm on a dose of cough syrup and some antihistamines, so I'll preface this by saying: I'm on drugs at the moment.

I think this all works out, the only thing I might do for one release is have both current_user and user do the same thing, with a deprecation warning on user(), then wait a week or two, then push a new release that removes user() altogether, that way people have a bit more warning incoming.

On 02/28/2012 05:48 PM, Ed Wildgoose wrote:

Hi, I added two new commits, please take a look at my master branch.

First reverts the change of stash key name (it's already under {authentication}->{} so really it can be anything we like

Second makes the current_user helper name be a configurable variable. It still defaults to 'current_user', which I know will cause distress, but it's a very common idiom in the rails world and it also keeps the root namespace clear - I can easily think of plenty of things that could be confused with the "$app->user()" method...

I'm also itching to push a third commit which makes the autoload_user default to 0... I initially defaulted to 1 because I thought it would load the user into some easy to access attribute of the stash (but it's not really). I then thought that it could load it into a second alias in the stash of say $current_user, but that's really redundant given that the current_user() method is available in any view... So really I see no benefit in autoloading the stash given that the main interface is via the "current_user()" helper method. This nails down the public interface and dissuades people from touching the private stash methods and keeps everything clean. Thoughts?

...Actually I talked myself into commit three whilst writing this email. autoload_user should be considered deprecated and considered for removal (this is equivalent to making lazy=1 the default in the old code

• however, we have removed all references to lazy except a before_dispatch hook which basically calls a private method slightly earlier than our public helper calls the same method - this seems to have little benefit unless some user is hacking with the private stash values rather than going through the public interface... Those users can simply add their own hook, bridge or under, which just calls current_user in a void context to achieve exactly the same effect)

I think this allows you to pull that branch now with only a minor complaint (ie back compatibility) about the default name of current_user... I feel fairly strongly about the name change, so going to push for it...

Cheers

Ed W

On 27/02/2012 23:43, Ben van Staveren wrote:

Hi Ed,

I do like it, except the breaking existing code part, but I think there's a way around that, just going to take two releases so people have some time to watch the deprecation warnings and then deprecate the old names/functionality. Currently up to my eyeballs in $dayjob, so I'll probably roll up a release this weekend :)

On 02/28/2012 02:57 AM, Ed Wildgoose wrote:

Hi, can you have a look at the latest 4 commits in my linked repo and see what you think? The main changes are driven based on the fairly mature rails auth plugins:

• Introduce a current_user method - almost everything can then be defined based on that
• Rename the user stash and user method to current_user... this is controversial and breaks existing code...
• Rename user_exists to is_user_authenticated to better match what it actually does. Can't think of a good new name for signature_exists though...
• lazy is renamed autoload_user to better represent what it does. Code simplified as a result.

Thanks

Ed W

You can merge this Pull Request by running:

 git pull https://github.com/ewildgoose/Mojolicious-Plugin-Authentication master

Or you can view, comment on it, or merge it online at:

 https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4

-- Commit Summary --

• Change ugly API
• Add some comments
• The lazy parameter is changed to autoload_user to become a simple hook which autoloads the user into the stash for us
• Introduce a current_user function - dramatically simplifies code and almost everything else can be implemented using it.

-- File Changes --

M lib/Mojolicious/Plugin/Authentication.pm (75) M t/01-functional.t (3) M t/02-functional_lazy.t (4)

-- Patch Links --

 https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4.patch
 https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4.diff

---

Reply to this email directly or view it on GitHub: https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4

---

Reply to this email directly or view it on GitHub:

https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4#issuecomment-4206215

Reply to this email directly or view it on GitHub: https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4#issuecomment-4213398

[ewildgoose]

Hi, Get well soon...!

Sure - that all works for me. Do you want me to push a commit to do the last thing you suggested? The alternative is that you can pull my branch and then stick your commit on top (or do a quick "rebase; push -f" before anyone notices...).

Just trying to make it straightforward for you to pull?

Cheers

Ed W

On 28/02/2012 11:20, Ben van Staveren wrote:

Hi Ed,

I'm on a dose of cough syrup and some antihistamines, so I'll preface this by saying: I'm on drugs at the moment.

I think this all works out, the only thing I might do for one release is have both current_user and user do the same thing, with a deprecation warning on user(), then wait a week or two, then push a new release that removes user() altogether, that way people have a bit more warning incoming.

On 02/28/2012 05:48 PM, Ed Wildgoose wrote:

Hi, I added two new commits, please take a look at my master branch.

First reverts the change of stash key name (it's already under {authentication}->{} so really it can be anything we like

Second makes the current_user helper name be a configurable variable. It still defaults to 'current_user', which I know will cause distress, but it's a very common idiom in the rails world and it also keeps the root namespace clear - I can easily think of plenty of things that could be confused with the "$app->user()" method...

I'm also itching to push a third commit which makes the autoload_user default to 0... I initially defaulted to 1 because I thought it would load the user into some easy to access attribute of the stash (but it's not really). I then thought that it could load it into a second alias in the stash of say $current_user, but that's really redundant given that the current_user() method is available in any view... So really I see no benefit in autoloading the stash given that the main interface is via the "current_user()" helper method. This nails down the public interface and dissuades people from touching the private stash methods and keeps everything clean. Thoughts?

...Actually I talked myself into commit three whilst writing this email. autoload_user should be considered deprecated and considered for removal (this is equivalent to making lazy=1 the default in the old code

• however, we have removed all references to lazy except a before_dispatch hook which basically calls a private method slightly earlier than our public helper calls the same method - this seems to have little benefit unless some user is hacking with the private stash values rather than going through the public interface... Those users can simply add their own hook, bridge or under, which just calls current_user in a void context to achieve exactly the same effect)

I think this allows you to pull that branch now with only a minor complaint (ie back compatibility) about the default name of current_user... I feel fairly strongly about the name change, so going to push for it...

Cheers

Ed W

On 27/02/2012 23:43, Ben van Staveren wrote:

Hi Ed,

I do like it, except the breaking existing code part, but I think there's a way around that, just going to take two releases so people have some time to watch the deprecation warnings and then deprecate the old names/functionality. Currently up to my eyeballs in $dayjob, so I'll probably roll up a release this weekend :)

On 02/28/2012 02:57 AM, Ed Wildgoose wrote:

Hi, can you have a look at the latest 4 commits in my linked repo and see what you think? The main changes are driven based on the fairly mature rails auth plugins:

• Introduce a current_user method - almost everything can then be defined based on that
• Rename the user stash and user method to current_user... this is controversial and breaks existing code...
• Rename user_exists to is_user_authenticated to better match what it actually does. Can't think of a good new name for signature_exists though...
• lazy is renamed autoload_user to better represent what it does. Code simplified as a result.

Thanks

Ed W

You can merge this Pull Request by running:

  git pull https://github.com/ewildgoose/Mojolicious-Plugin-Authentication master

Or you can view, comment on it, or merge it online at:

  https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4

-- Commit Summary --

• Change ugly API
• Add some comments
• The lazy parameter is changed to autoload_user to become a simple hook which autoloads the user into the stash for us
• Introduce a current_user function - dramatically simplifies code and almost everything else can be implemented using it.

-- File Changes --

M lib/Mojolicious/Plugin/Authentication.pm (75) M t/01-functional.t (3) M t/02-functional_lazy.t (4)

-- Patch Links --

  https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4.patch
  https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4.diff

---

Reply to this email directly or view it on GitHub: https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4

---

Reply to this email directly or view it on GitHub:

https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4#issuecomment-4206215

Reply to this email directly or view it on GitHub: https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4#issuecomment-4213398

---

Reply to this email directly or view it on GitHub: https://github.com/benvanstaveren/Mojolicious-Plugin-Authentication/pull/4#issuecomment-4213821