Re: [Catalyst] Plugin::Authentication overrides $c-req-user
On 2009-02-21 01:49:51 +, Tomas Doran wrote: Attached is a set of patches to add support for $c-req-remote_user, including a basic test. Good stuff, thanks. I've branched 5.80 trunk and applied your Runtime change, and then I've fiddled the 'do we warn' logic to be a bit safer. Have a look and let me know what you think? Yeah, the 'do we warn' logic was a bit hairy. I used caller because the engines would trigger the warning when they call $c-req-user (every request), even with an empty REMOTE_USER environment variable. Is anything in the current test suite triggering the new warning? If so, can you switch it over to be calling -remote_user instead, and can you add a call to read -user which provokes the warning, and test you get the expected warning (see t/deprecated.t r9354 - you could just add the warning test here/to that app which already has its global logger overridden?) There were no tests for $c-req-user. I've committed a test to t/deprecated.t that currently fails due to the problem above (we should only get one warning); please let me know if you have any suggestions. Thanks, -- Daniel Westermann-Clark ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Plugin::Authentication overrides $c-req-user
On 20 Feb 2009, at 22:57, Daniel Westermann-Clark wrote: On 2009-02-11 21:53:48 +, Tomas Doran wrote: Why not just add a remote_user() method on $c-req instead? It's a little more typing, but is more explicit about where the value comes from and doesn't potentially break any existing apps. Patches on 5.80 welcome :) Attached is a set of patches to add support for $c-req-remote_user, including a basic test. Good stuff, thanks. I've branched 5.80 trunk and applied your Runtime change, and then I've fiddled the 'do we warn' logic to be a bit safer. Have a look and let me know what you think? http://dev.catalystframework.org/repos/Catalyst/Catalyst-Runtime/5.80/ branches/deprecate_req_user/ Test for the new functionality looks fine. There is also a deprecation warning for non-Catalyst packages calling $c-req-user. Is anything in the current test suite triggering the new warning? If so, can you switch it over to be calling -remote_user instead, and can you add a call to read -user which provokes the warning, and test you get the expected warning (see t/deprecated.t r9354 - you could just add the warning test here/to that app which already has its global logger overridden?) I'm not sure about the engine patches, but it seemed like a forward-compatible way to add support now for the new method. They look totally sane to me. Lets get Runtime right and a nod from andyg before applying them however :) Feel free to supply another patch against the branch, or hop on irc and grab a commit bit so you can commit yourself? Cheers t0m ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Plugin::Authentication overrides $c-req-user
On Tue, Feb 10, 2009 at 9:46 PM, Daniel Westermann-Clark d...@pobox.comwrote:
Hi,
At work we use, among other things, the value of REMOTE_USER in the
request environment to authenticate users using our single-sign on
system. We access this via $c-req-user, which the various engines
set using the data available to them.
However, Catalyst::Plugin::Authentication-set_authenticate overrides
this value with the blessed user object in addition to setting
$c-user. Looking at the commit logs, it looks like this has been
true since 2005 but I'm having a hard time finding a reason why.
Also, Catalyst::Request lists $c-req-user as deprecated. If it were
to be removed, how would one access the value in an engine-agnostic
way?
I'm not familiar with $c-req-user, but isn't REMOTE_USER a header you can
read with $c-req-header('remote_user'), or whatever header name is being
passed around?
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Plugin::Authentication overrides $c-req-user
On 2009-02-11 10:06:42 +0100, Rodrigo wrote:
I'm not familiar with $c-req-user, but isn't REMOTE_USER a header
you can read with $c-req-header('remote_user'), or whatever header
name is being passed around?
Some authentication schemes might provide headers containing the
username, but REMOTE_USER is different. It's not available as a
header.
Regardless, is there a reason it should be a string at first and then
an object? It's inconsistent and causes a loss of information about
the request.
If no one is using this behavior, I'd be happy to provide patches to
deprecate or remove it.
--
Daniel Westermann-Clark
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Plugin::Authentication overrides $c-req-user
Daniel Westermann-Clark wrote on 02/11/2009 02:53 PM:
On 2009-02-11 10:06:42 +0100, Rodrigo wrote:
I'm not familiar with $c-req-user, but isn't REMOTE_USER a header
you can read with $c-req-header('remote_user'), or whatever header
name is being passed around?
Some authentication schemes might provide headers containing the
username, but REMOTE_USER is different. It's not available as a
header.
Regardless, is there a reason it should be a string at first and then
an object? It's inconsistent and causes a loss of information about
the request.
If no one is using this behavior, I'd be happy to provide patches to
deprecate or remove it.
Why not just add a remote_user() method on $c-req instead? It's a
little more typing, but is more explicit about where the value comes
from and doesn't potentially break any existing apps.
--
Peter Karman . pe...@peknet.com . http://peknet.com/
___
List: Catalyst@lists.scsys.co.uk
Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst
Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/
Dev site: http://dev.catalyst.perl.org/
Re: [Catalyst] Plugin::Authentication overrides $c-req-user
On 11 Feb 2009, at 21:37, Peter Karman wrote: Daniel Westermann-Clark wrote on 02/11/2009 02:53 PM: If no one is using this behavior, I'd be happy to provide patches to deprecate or remove it. Why not just add a remote_user() method on $c-req instead? It's a little more typing, but is more explicit about where the value comes from and doesn't potentially break any existing apps. +1 for remote_user Catalyst::Request's user method has been marked as deprecated in the docs since 2006 please make calling it issue a warning once per- request, so that it can killed outright some time in the future. Patches on 5.80 welcome :) Cheers t0m ___ List: Catalyst@lists.scsys.co.uk Listinfo: http://lists.scsys.co.uk/cgi-bin/mailman/listinfo/catalyst Searchable archive: http://www.mail-archive.com/catalyst@lists.scsys.co.uk/ Dev site: http://dev.catalyst.perl.org/
