[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 ...

[Chuck Hagenbuch]

I am somewhat buried for various reasons, and I apologize for top posting.

I agree with Jan on both usage of exceptions (instead of a switch() on  
the getCode() method - though I do see a place for codes, at a more  
granular level - say, a class for auth exceptions, a code for the  
reason), and on having more and more granular hooks, rather than less  
hooks. I think he's done a good job of advocating for both.

Quoting Jan Schneider <jan at horde.org>:

> Zitat von Michael M Slusarz <slusarz at horde.org>:
>
>> 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.
>
> The code in IMP you referred to is a perfect example where the  
> application *does* 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.
>
> This is *exactly* what exceptions are for, and this has nothing to  
> do with class name checking:
>
> try {
>     $registry->pushApp('app');
> } catch (Horde_Exception_NotAuthenticated $e) {
>     Horde_Auth::authenticationFailureRedirect('app', $e);
> } catch (Horde_Exception_PermissionDenied $e) {
>     $notification->push('Go away');
> }
> // Any other exception handled by the global handler.
>
>> 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.
>
> I missed that the exception is being passed to  
> authenticationFailureRedirect(). But this should still not be called  
> unconditionally, no matter what exception we see.
> You say that there are only 3 possible exception cases. But that  
> could change in any point in the future. And we might not want to  
> redirect to the login page at this case.
>
>>>>>>>> * 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.
>
> I disagree. The large number of hooks is, what Horde's real power of  
> flexibility is. Being able to call hooks at all kind of places in  
> the code is major selling point, and we should rather extend this,  
> than slimming it down.
> There is no disagreement about, that those "grown" hooks need to be  
> cleaned up.
>
> Jan.
>
> -- 
> Do you need professional PHP or Horde consulting?
> http://horde.org/consulting/



-chuck