GNOME Bugzilla – Bug 732496
Use FreeRDP API
Last modified: 2017-02-16 10:51:13 UTC
Created attachment 279611 [details] [review] Use FreeRDP API in RDP plugin Attached patch implements RDP plugin using FreeRDP API. It allows us to handle NLA authentication meaningfully (this was the main reason for implementing it). I used FreeRDP clients sources for inspiration. I've tested it with FreeRDP 1.0, 1.1 and 1.2. Marek
Review of attachment 279611 [details] [review]: Generally, looks good, thanks for the patch! I do not have any RDP server to test, but presumably you have tested against a real server and it works fine? Does this solve bug 724133, by handling authentication failures using the API (it's hinted at in your commit message, but I want to make sure)? ::: configure.ac @@ +70,3 @@ AS_IF([test "x$enable_rdp" != "xno"], + [PKG_CHECK_MODULES([RDP], + [$RDP_DEPS], This should be handled as for other optional dependencies, by using PKG_CHECK_EXISTS, and modifying RDP_DEPS in the dependencies are not found. Then, RDP_DEPS should be used in the single call to PCK_CHECK_MODULES. That also removes the need to modify Makefile.am in this patch. @@ +82,3 @@ + +AC_SUBST(RDP_CFLAGS) +AC_SUBST(RDP_LIBS) These are not needed in recent versions of pkg-config. ::: data/vinagre.ui @@ +288,3 @@ + <property name="invisible_char">●</property> + <property name="activates_default">True</property> + <property name="invisible_char_set">True</property> Does the domain need to be hidden with invisible characters, like a password field? ::: plugins/rdp/vinagre-rdp-tab.c @@ +126,2 @@ { + G_OBJECT_CLASS (vinagre_rdp_tab_parent_class)->finalize (object); There should be no need to override finalize(), if the only action is to chain up to the parent class.
(In reply to comment #1) > ::: data/vinagre.ui > @@ +288,3 @@ > + <property name="invisible_char">●</property> > + <property name="activates_default">True</property> > + <property name="invisible_char_set">True</property> > > Does the domain need to be hidden with invisible characters, like a password > field? Sorry, I thought that this implied that the characters would be hidden, but that would need "visibility" to be set to FALSE, which is not the case. Ignore that comment!
Created attachment 279666 [details] [review] Use FreeRDP API in RDP plugin Hi David, thank you for the review. (In reply to comment #1) > Review of attachment 279611 [details] [review]: > > Generally, looks good, thanks for the patch! I do not have any RDP server to > test, but presumably you have tested against a real server and it works fine? I have tested this with Windows 7 Enterprise and Windows XP. It worked fine for me. > Does this solve bug 724133, by handling authentication failures using the API > (it's hinted at in your commit message, but I want to make sure)? Unfortunately, I see that I forgot to implement support for handling of the certificates. It asks for username, password and domain right now. I have to add the dialogs for accepting/rejecting of certificates yet. It shouldn't be hard but I don't have the Windows 7 Enterprise available now (I'll try it with the Windows XP at home but I guess that XP doesn't support certificates here). > ::: configure.ac > @@ +70,3 @@ > AS_IF([test "x$enable_rdp" != "xno"], > + [PKG_CHECK_MODULES([RDP], > + [$RDP_DEPS], > > This should be handled as for other optional dependencies, by using > PKG_CHECK_EXISTS, and modifying RDP_DEPS in the dependencies are not found. > Then, RDP_DEPS should be used in the single call to PCK_CHECK_MODULES. That > also removes the need to modify Makefile.am in this patch. I've modified the patch this way. > @@ +82,3 @@ > + > +AC_SUBST(RDP_CFLAGS) > +AC_SUBST(RDP_LIBS) > > These are not needed in recent versions of pkg-config. I've removed them. > ::: plugins/rdp/vinagre-rdp-tab.c > @@ +126,2 @@ > { > + G_OBJECT_CLASS (vinagre_rdp_tab_parent_class)->finalize (object); > > There should be no need to override finalize(), if the only action is to chain > up to the parent class. You are right, I overlooked this. The patch needs to add the certificates handling yet so I'm marking it as needs-work. Regards Marek
Comment on attachment 279666 [details] [review] Use FreeRDP API in RDP plugin Thanks for making all the requested changes, and testing! As you have improved the RDP support immensely, can you push this patch to master as-is? Then, once the certificate-handling is completed, you can post that as a patch to the other bug, and close that as a separate problem. This should make it a bit easier to track the two issues in the future (when looking back at the git history). Thanks!
Comment on attachment 279666 [details] [review] Use FreeRDP API in RDP plugin (In reply to comment #4) > (From update of attachment 279666 [details] [review]) > Thanks for making all the requested changes, and testing! As you have improved > the RDP support immensely, can you push this patch to master as-is? Then, once > the certificate-handling is completed, you can post that as a patch to the > other bug, and close that as a separate problem. This should make it a bit > easier to track the two issues in the future (when looking back at the git > history). Thanks! Thank you for the re-review! I've pushed the patch to master and will continue in the bug #724133 once I have a machine against which I can test it. Marek
nice, too bad I come late, it would have been worthwhile to use and contribute to the git://git.gnome.org/gb-rdp library . That way the same gtk adaptation code could have been used by both vinagre and boxes or other projects.
*** Bug 694928 has been marked as a duplicate of this bug. ***