Review Request: Proper password caching when opening remote directories in KFileDialog

[Dawit Alemayehu]

> On Nov. 29, 2011, 10:40 a.m., David Faure wrote:
> > kfile/kdirselectdialog.cpp, line 273
> > <http://git.reviewboard.kde.org/r/103226/diff/3/?file=41903#file41903line273>
> >
> >     Is this really necessary? I thought KIO did this internally already, so that when creating a job we could just pass any widget as parameter.

Yes it is. Right now KIO::Scheduler makes a half-hearted attempt to find the top level window by doing widget->window(). Unfortunately that is not enough when using a file dialog because it is considered a top level window, i.e. it is an ancestor widget that has or could have a window system frame. As such, the window id of the file dialog will be used to determine the lifetime of the password instead of the one from the actual application where the file dialog was invoked.


> On Nov. 29, 2011, 10:40 a.m., David Faure wrote:
> > kfile/knewfilemenu.cpp, line 966
> > <http://git.reviewboard.kde.org/r/103226/diff/3/?file=41905#file41905line966>
> >
> >     It would really be much simpler to do this inside of setWindow() or even deeper down...

Well I have no real objections to doing that though I have a small concern. If the check for the topLevelWindow is moved to setWindow or even deeper, it would become a tiny bit more expensive for applications that were doing the right thing when setting the top level window in the first place. I am sure the impact won't be much, but nonetheless it would still be more than it is now. If the check should be moved, then it needs to be moved deeper than setWindow() because setWindow() simply does the following:

    KDialogJobUiDelegate::setWindow(w);
    KIO::Scheduler::registerWindow(w);

In order to ensure the proper logic is used at the lowest possible location,  the check IMHO needs to be implemented at the level of those two classes. I will provide an updated patch.


- Dawit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103226/#review8590
-----------------------------------------------------------


On Nov. 24, 2011, 3:16 p.m., Dawit Alemayehu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103226/
> -----------------------------------------------------------
> 
> (Updated Nov. 24, 2011, 3:16 p.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Description
> -------
> 
> The attached patch fixes the scenario outlined in bug# 179663 by making the best effort to identify and use the top most window when invoking KIO functions. That way any password information supplied by the user is cached, even if the user did not check the "Remember password" checkbox, for the duration of the application instead of just the lifetime of the file dialog. 
> 
> Right now almost all KFileDialog's KIO access does not set the widget parameter. If a site then requires authentication, no window-id information will be passed to it. That in turn results in the user supplied password being cached for only a very very short duration, ~10 secs. Afterwards, the password is removed and the user is inevitably re-prompted to supply the same credentials again.
> 
> 
> This addresses bug 179663.
>     http://bugs.kde.org/show_bug.cgi?id=179663
> 
> 
> Diffs
> -----
> 
>   kfile/kdiroperator.cpp 4c93ac9 
>   kfile/kdirselectdialog.cpp 0212e58 
>   kfile/kfilewidget.cpp 09b86d4 
>   kfile/knewfilemenu.cpp ac54041 
> 
> Diff: http://git.reviewboard.kde.org/r/103226/diff/diff
> 
> 
> Testing
> -------
> 
> Tested with the scenario outlined in the afforementioned bug report using a publicly available demo webdav server, webdav://demo.sabredav.org.
> 
> 
> Thanks,
> 
> Dawit Alemayehu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20111129/326dd4ea/attachment.htm>