On Sun, Jul 25, 2010 at 11:45 PM, Christophe Cordenier <[email protected]> wrote: > As this concept has been introduced in 5.2, i am currently looking at the > first purpose of it. Where have you implemented the security layer in the > Tapestry pipeline ?
It's an advice to RequestExceptionHandler - whether a decorator or an advice I don't think it would matter in this case. > Should you security request filter be inserted after the > InitializeActivePageName filter solve your problem ? There's a security filter as well of course for binding the security the context and for doing url-based security etc. but the security exception can be thrown from several different levels so I don't think I could generically catch and handle the security exceptions at the filter level, leaving decorating the exception handler as the best option AFAIK. > Actually, i am not against applying your patch, but as PageResponseRenderer > is internal, as a new committer i am definitely cautious in what i am doing > and committing ;) No worries, I fully understand where you are coming from. The code worked as is for T5.1 and I already experimentally modified it to call storeActivePageName() from outside just to verify that'd be enough for T5.2. So it's not a matter of making it work, but making it work the right way. Although we both may end up spending more time on it this way, I much prefer spending my resources on validating the concept than just hacking it in. So in the above case, we don't really want to replace the RequestExceptionHandler as handling the non-security related exceptions are delegated back to the original, but if somebody replaces the DefaultRequestExceptionHandler with one that contains a form on it, it'd throw an exception as well (just putting together what you said and reading the code), right? Hopefully that proves the need for the fix. Kalle > 2010/7/26 Kalle Korhonen <[email protected]> > >> On Sun, Jul 25, 2010 at 5:35 AM, Christophe Cordenier >> <[email protected]> wrote: >> > I am currently working on this JIRA, i have seen that the >> getActivatePage() >> > is only called to pre allocate names in Form component so it should have. >> So >> > i guess the page you are returning contains a Form. >> >> Right and thanks so much Christophe for taking a look at the issue. >> >> > Despite it sound right that PageResponseRenderer should set the active >> page, >> > don't you think that in case of a security exception it would be easier >> to >> > send an HTTP error and let the application developer use standard web.xml >> > request dispatching to configure the page to display ? >> >> Specifically in the case of security exception, no, I don't think >> that'd be easier. While it's an option, I don't see what the advantage >> would be. The exception handler in tapestry-security catches the >> exception, sets the error code and renders the configured page. >> Configuring the page somewhere else (in standard web.xml) seems to me >> like an unnecessary complication compared to contributing >> configuration. >> >> Security exception aside, in the general case http error codes alone >> wouldn't be enough to differentiate all the different error cases. I'm >> not strongly against exploring alternatives, but it makes sense to me >> that PageResponseRenderer would set the active page and I don't see >> any disadvantages in doing so, do you? >> >> Kalle >> >> >> > 2010/7/20 Kalle Korhonen <[email protected]> >> >> Pretty please, any committer? It's a one-liner to fix >> >> https://issues.apache.org/jira/browse/TAP5-1201 and the patch is >> >> attached. >> >> >> >> Kalle >> >> >> >> >> >> On Mon, Jul 19, 2010 at 2:23 PM, Pierce Wetter <[email protected]> >> wrote: >> >> > >> >> > On Jul 9, 2010, at 1:40 PM, Kalle Korhonen wrote: >> >> > >> >> >> Sorry to be a pest, but how could I find a champion to apply the >> patch >> >> >> for TAP5-1201? I understand Howard's busy and his time is probably >> >> >> better spent on bigger issues than this, but I hope that some other >> >> >> committer would be able to pick this up and apply the one-line change >> >> >> that would restore functionality that existed in T5.1. It's my >> >> >> understanding its perfectly safe to do as suggested so we can't go >> >> >> that far off even if somebody wants to refactor it later. >> >> > >> >> > Ditto, since I'm using tapestry-security with 5.2-SNAPSHOT. >> >> > >> >> > >> >> > --------------------------------------------------------------------- >> >> > To unsubscribe, e-mail: [email protected] >> >> > For additional commands, e-mail: [email protected] >> >> > >> >> > >> >> >> >> --------------------------------------------------------------------- >> >> To unsubscribe, e-mail: [email protected] >> >> For additional commands, e-mail: [email protected] >> >> >> >> >> > >> > >> > -- >> > Regards, >> > Christophe Cordenier. >> > >> > Committer on Apache Tapestry 5 >> > Co-creator of wooki @wookicentral.com >> > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> >> > > > -- > Regards, > Christophe Cordenier. > > Committer on Apache Tapestry 5 > Co-creator of wooki @wookicentral.com > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
