[dev] [cvs] commit: horde login.php horde/templates/index frames_index.inc horde/templates/login header.inc login.inc mobile.inc horde/services changepassword.php facebook.php logintasks.php twitter.php twitterapi.php horde/services/portal edit.php ...

[Michael M Slusarz]

Quoting Jan Schneider <jan at horde.org>:

> This is far too much "knowledge" that the consumer of pushApp()  
> should have. And assuming that the only exception that pushApp()  
> might throw happens if the user is not authenticated, is too short  
> sighted IMO. We need a specific exception for that case, i.e.  
> Horde_Exception_NotAuthenticated, so that with any other exception  
> the global exception handler is called, and not the user silently  
> being redirected to the login page.

You do get this information in the getCode() method of the exception -  
it will let you know if there was an authentication failure  
(Horde_Registry::AUTH_FAILURE), a permissions failure  
(Horde_Registry::PERMISSION_DENIED), or the fact that the application  
is not active (Horde_Registry::NOT_ACTIVE).  See imp for an example of  
what an app could potentially do to work with one of those types of  
errors.  But 99% of the time, applications shouldn't be dealing with  
these issues.  What's the point of a framework if the applications  
have to do all the error checking.

I am *very* against the idea of determining errors based on Exception  
classnames - that is not what classnames are for.  Not to mention it  
is overkill to define a bunch of Exception files that need to be  
included in every page, when most of these exception files don't  
extend the original class one bit.

Reading back at your post, I guess I am confused what your concern is.  
  Using Horde::authenticationFailureRefirect(), you will *never* be  
redirected to the login page without there being an explanation why  
authentication was unsuccessful (either auth issue or permissions  
issue).  And in the third error case (application not active), this  
will throw an exception that should result in a fatal error.

>>>>>> * Hooks probably don't work yet.  We may need to rethink hooks  
>>>>>> a bit.  First, there should not be need for app-specific  
>>>>>> pre/post-auth hooks.  This will all be handled by a single hook  
>>>>>> in Horde ($app will be one of the parameters passed to the  
>>>>>> hook).  Unfortunately, pre-auth hooks don't make any sense for  
>>>>>> transparent auth or for apps that don't need authentication  
>>>>>> since any value returned from the pre-auth hook is totally  
>>>>>> ignored.  I recommend the following refactoring of these hooks:
>>>>>>
>>>>>> + pre-auth hook: called only for horde-auth/apps that need  
>>>>>> authentication. This hook is solely for the purpose of altering  
>>>>>> auth credentials.
>>>>>> + post-auth hook: called for all apps after user has been  
>>>>>> authenticated to the module. App-specific setup can be handled  
>>>>>> in here. Return value of false indicates application is not  
>>>>>> available.
>>>>>
>>>>> Does that mean that post-auth is called for every module?
>>>>
>>>> Yes.  This removes the need for per-app hooks to do work after  
>>>> first login.
>>>
>>> Sorry, I still don't get this. Is this hook called for every  
>>> installed application, once the user logged in?
>>
>> It is called for every application the first time it is accessed in  
>> the session.  If you have enabled something like chora, and it is  
>> never accessed, the postauth hook is never called.
>
> That's pretty useful, but should be a different hook than postauth.  
> It might confuse others too. I expect postauth to do the same like  
> the old postauth hook, i.e. being called directly after the (Horde)  
> authentication, as a last resort to deny authentication.
> A hook that being called on first access to an application should be  
> clearly named like that, e.g. "firstaccess" or "application_init" or  
> even separate ones ("initialize") for each application in each app's  
> hook.php.

I am against this also.  We have *way* too many hooks as it is.  I  
work on this code everyday, and I don't know what half of the hooks  
do.  In the interest of slimming down configuration, which I think we  
all agree is a big goal of Horde 4, this is one of the ways to  
accomplish that.

michael

-- 
___________________________________
Michael Slusarz [slusarz at horde.org]