On Mon, Oct 13, 2014 at 12:33 PM, Jesse Long <[email protected]> wrote:
> On 13/10/2014 09:52, Martin Grigorov wrote: > >> Hi Jesse, >> >> Thanks for identifying these problems and working on the improvements! >> >> I've added some comments about small issues to >> https://github.com/apache/wicket/commit/c63989018a8fb11a3dbbc68253e74f >> 3fd2117430 >> . >> >> I like the new setting used for the new "crypt." prefix used for the page >> expired notification. >> Is it good idea to use such settings/flags to be able to tell what to be >> encrypted (componentAndPageInfo, query strings, all) explicitly ? At the >> moment this is "calculated". >> I ask whether it is sensible to keep the current code that decides what to >> encrypt and what not and add settings which will define this explicitly ? >> >> > Hi Martin, > > Thank you for your valuable feedback. > > "OK, will do" on all your inline comments. > > RE: Settings for what to encrypt: > How about protected boolean shouldEncryptEntireUrl(Url url)? > > true -> entire url. > false -> pageComponentInfo query param. > I think this method is not needed. The developer can override #encryptUrl() and encrypt the url as (s)he wishes. > > I dont think we should try to encrypt all query parameters. Sometimes > query parameters are for state and should be encrypted, but Wicket has much > better built in state management that does not expose itself in URL. > > Other query parameters should be editable by the user, like Google "q", > and we would lose this ability. > Initially my thinking about this was: someone will come and ask why query string parameters are not encrypted and path parameters are encrypted ? But the other change in your commits is that explicitly mounted paths are also left unencrypted so path params are readable / manually overridable, so it looks consistent! > > Also, we will always have unencrypted query parameters coming in, user > modifies URL, Ajax additional params etc. When URL is manually edited, we > would get needless redirects, like /mypage?wicket=encrypted&additional=param > redirect to /mypage?wicket=LongerEncryptedThing. > This is true. And it will work without the extra redirect by default. But some user may want to have all query parameters encrypted and this redirect won't be able problem for him/her. > > What happens when a user manually adds an unencrypted param that exists > with a different value in the encrypted part? > I'd expect to have two values for this parameter name. > > Thanks, > Jesse > I agree to leave the code as it is now! I'm just trying to make it as flexible as possible to cover as many use cases as possible. Leaving the APIs open for extension should be good enough!
