Created attachment 358236 [details]
Reference code to download a RHEL ISO from developers.redhat.com

The developers.redhat.com website seems to be free software [1] and is supposed to have an API to programmatically access parts of it. However, documentation is hard to come by. Attached is a piece of code that documents how to download a RHEL ISO.

[1] https://github.com/redhat-developer/developers.redhat.com

I have pushed a wip/rishi/rhel branch with some initial incomplete code. I'll start attaching the patches here once it is more functional.

Created attachment 359336 [details] [review]
wizard-source: Group the sources into local files, and everything else

Created attachment 359337 [details] [review]
wizard-source: Separate the groups of sources using headings

Created attachment 359338 [details] [review]
wizard-source: Align each page separately, instead of the whole widget

Created attachment 359339 [details] [review]
wizard: Let the WizardSource expand if it wants to

Created attachment 359340 [details] [review]
downloader, wizard: Support a custom filename for downloaded media

Created attachment 359342 [details] [review]
wizard: Allow a custom filename when sanity-checking downloadable URIs

Created attachment 359343 [details] [review]
wizard: Wire the custom filename to the URI validator

Created attachment 359344 [details] [review]
wizard: Wire the custom filename to the downloader

Created attachment 359345 [details] [review]
configure: Require json-glib and webkitgtk+

Created attachment 359346 [details] [review]
configure: Require gtk+ >= 3.22.20

Created attachment 359347 [details] [review]
wizard, wizard-source: Support gratis RHEL boxes

This is quite functional now. Things that remain to be done:
(a) Use RHEL logo when downloading the ISO
(b) Move the file chooser button to match the mockups
(c) Update the management of the header bar buttons

At the moment, I am not planning to auto-enrol the installed VM through Red Hat's subscription manager. This is because there is no way to securely pass the credentials from the point where the user enters them in the web view to the post installation stage where the enrollment needs to happen. We'll have to revisit this when things change.

I was going to attach screenshots, but since I already made a screencast, let's use that: https://rishi.fedorapeople.org/gnome-boxes-rhel-00.webm

Created attachment 359853 [details] [review]
wizard-source: Group the sources into local files, and everything else

Created attachment 359854 [details] [review]
wizard-source: Separate the groups of sources using headings

Created attachment 359855 [details] [review]
wizard-source: Move the file selection button next to the heading

Created attachment 359856 [details] [review]
wizard-source: Align each page separately, instead of the whole widget

Created attachment 359857 [details] [review]
wizard: Let the WizardSource expand if it wants to

Created attachment 359858 [details] [review]
wizard, wizard-source: Don't align & margin WizardSource

Created attachment 359859 [details] [review]
downloader, wizard: Support a custom filename for downloaded media

Created attachment 359860 [details] [review]
wizard: Allow a custom filename when sanity-checking downloadable URIs

Created attachment 359861 [details] [review]
wizard: Wire the custom filename to the URI validator

Created attachment 359862 [details] [review]
wizard: Wire the custom filename to the downloader

Created attachment 359863 [details] [review]
configure: Require json-glib and webkitgtk+

Created attachment 359864 [details] [review]
configure: Require gtk+ >= 3.22.20

Created attachment 359865 [details] [review]
wizard-source: Add a WebView with progress indication

Created attachment 359866 [details] [review]
wizard, wizard-source: Support gratis RHEL boxes

(In reply to Debarshi Ray from comment #14)
> This is quite functional now. Things that remain to be done:
> (a) Use RHEL logo when downloading the ISO

Done.

> (b) Move the file chooser button to match the mockups

Done.

> (c) Update the management of the header bar buttons

To do.

I am ready for some initial feedback on the structure and layout of the patches.

Review of attachment 359853 [details] [review]:

it makes sense.

Could you please hold these commits a bit so we can push the feature as a whole?

Review of attachment 359854 [details] [review]:

sure.

Review of attachment 359855 [details] [review]:

ok

Review of attachment 359856 [details] [review]:

sure.

Review of attachment 359857 [details] [review]:

could be part of the previous commit, but don't bother. :)

Review of attachment 359858 [details] [review]:

ok

Review of attachment 359859 [details] [review]:

sure.

Review of attachment 359860 [details] [review]:

ok

Review of attachment 359861 [details] [review]:

sure.

Review of attachment 359862 [details] [review]:

sure.

Review of attachment 359863 [details] [review]:

I hope we can revert that once the developer portal has an OAuth-like authentication. Btw, did you contact them regarding that? Maybe we could open another bug for that (for the future).

Review of attachment 359864 [details] [review]:

Thanks for digging into this!

(In reply to Felipe Borges from comment #31)
> Review of attachment 359853 [details] [review] [review]:
> 
> it makes sense.
> 
> Could you please hold these commits a bit so we can push the feature as a
> whole?

Yes, of course.

Thanks for all the reviews!

(In reply to Felipe Borges from comment #35)
> Review of attachment 359857 [details] [review] [review]:
> 
> could be part of the previous commit, but don't bother. :)

Ok, I will do that.

(I split the UI file changes to make them more understandable. Sometimes I find it challenging to understand XML diffs. :)

Review of attachment 359865 [details] [review]:

* Can we somehow (maybe by injecting a stylesheet) tweak the page to:

  1. Drop the cookies notification div;
  2. Center the view into the login form;
  3. Turn the white background into black (or better, make it match the @theme_bg_color of the gtk+ theme?)

* the right click exposes a disfunctional context menu. I think webkit has an API to drop/customize it.

* If I close the wizard and open it again the webview is still showing the old page. I see that you don't want to destroy/create it every time but we could load "about:blank" when the wizard gets dismissed?!? 

* Related to the previous point, it will keep history, so right click -> [BACK] can go back to something else. This is related to the context menu I mentioned in the paragraph above.

* Could we filter the domain and restrict the browsing to login specific bits? I could choose to login with my Twitter account, click on the twitter logo at their login page and turn Boxes into a Twitter client. (maybe not a bug, but a feature ;-) )

* The back button (at the headerbar) is sensitive, but it doesn't really go back to anywhere.

(In reply to Felipe Borges from comment #41)
> Review of attachment 359863 [details] [review] [review]:
> 
> I hope we can revert that once the developer portal has an OAuth-like
> authentication. Btw, did you contact them regarding that? Maybe we could
> open another bug for that (for the future).

When I spoke with them a few months ago, they were friendly but it was difficult to get them to commit to actually do something. I am hoping that once we have an initial iteration of this feature, we can talk to them more concretely.

It is not clear to me whether they will or will not support OAuth in future. It seemed that they don't do it on purpose, but it wasn't clear. Something to bring up again, I guess.

There are a few other things we need:

(i) It would be nice if the portal didn't randomly return JSON and XML.

(ii) We need a way to auto-subscribe the installed VM through subscription-manager. Since the express installation uses a kickstart file, we cannot just put the password in plain text there. The subscription password is the same as the portal's log-in password, so it's risky. subscription-manager supports something called an activation key. It was not clear whether we can use a short lived key to subscribe the VM. The portal developers, and the subscription-manager developers are disjoint sets. So there is a communication gap to be bridged. :)

(In reply to Debarshi Ray from comment #46)
>
> (ii) We need a way to auto-subscribe the installed VM through
> subscription-manager. Since the express installation uses a kickstart file,
> we cannot just put the password in plain text there. The subscription
> password is the same as the portal's log-in password, so it's risky.
> subscription-manager supports something called an activation key. It was not
> clear whether we can use a short lived key to subscribe the VM. The portal
> developers, and the subscription-manager developers are disjoint sets. So
> there is a communication gap to be bridged. :)

To put on record what we just spoke IRL, I suggest:

  * A Spice channel for the keyring which would allow the host to expose its keyring to the guests? (some spice dev could drop a comment clarifying the feasibility of it)

  * A key exchange throughout the webdav channel (guest's agent generates a pair, passes its public key to the host which encrypts the authentication data and passes it back to the guest to feed it to the subscription-manager cli).

(In reply to Felipe Borges from comment #45)
> Review of attachment 359865 [details] [review] [review]:
> 
> * Can we somehow (maybe by injecting a stylesheet) tweak the page to:
> 
>   1. Drop the cookies notification div;
>   2. Center the view into the login form;
>   3. Turn the white background into black (or better, make it match the
> @theme_bg_color of the gtk+ theme?)
> 
> * the right click exposes a disfunctional context menu. I think webkit has
> an API to drop/customize it.

I'll look into these.

> * If I close the wizard and open it again the webview is still showing the
> old page. I see that you don't want to destroy/create it every time but we
> could load "about:blank" when the wizard gets dismissed?!? 

Good point.

> * Related to the previous point, it will keep history, so right click ->
> [BACK] can go back to something else. This is related to the context menu I
> mentioned in the paragraph above.
> 
> * Could we filter the domain and restrict the browsing to login specific
> bits? I could choose to login with my Twitter account, click on the twitter
> logo at their login page and turn Boxes into a Twitter client. (maybe not a
> bug, but a feature ;-) )
> 
> * The back button (at the headerbar) is sensitive, but it doesn't really go
> back to anywhere.

Yes, I haven't touched the header bar buttons because I wasn't sure about certain aspects of their behaviour. Thanks for clearing those up in real life. :)

(In reply to Felipe Borges from comment #45)
> Review of attachment 359865 [details] [review] [review]:
> 
> * Can we somehow (maybe by injecting a stylesheet) tweak the page to:
> 
>   1. Drop the cookies notification div;
>   2. Center the view into the login form;
>   3. Turn the white background into black (or better, make it match the
> @theme_bg_color of the gtk+ theme?)

As you pointed out, the developers.redhat.com log-in page can lead to a bunch of social media sites for alternate log-in mechanisms. Do you want to do these things for those websites too? While the Red Hat portal is somewhat under our control, I am worried about fiddling with a whole array of 3rd party sites. It might lead to random breakage.

For the cookies notification, I learnt about http://prebake.eu/ from #webkitgtk+. I don't know if we can leverage it somehow in this case, but I don't know the legal ramifications either. The portal does ask a few questions after log-in when a user downloads an image for the first time. Here is a brief explanation of the law:
  https://www.cookielaw.org/the-cookie-law/

> * the right click exposes a disfunctional context menu. I think webkit has
> an API to drop/customize it.

Yes, we can use WebKitWebView::context-menu to customize it. We probably only want to hide a few menu items (eg., back, forward, stop and reload) and not completely disable the menu. We still want copy, cut, paste, etc..

Created attachment 360205 [details] [review]
wizard-source: Add a WebView with progress indication

Hide some menu-items from the right-click context menu.

Created attachment 360483 [details] [review]
wizard: Rename a method

Created attachment 360484 [details] [review]
wizard, wizard-source: Support gratis RHEL boxes

Changes:

* Working header bar buttons.

* Used a spinner when the initial JSON/XML is downloaded.

* Removed a few non-working and undesired items from the WebView's right-click context menu.

* Cookies get cleared before the WebView is re-used.

* The page gets reset before the WebView is re-used.

I didn't do anything about restricting / disabling the links in the social media pages, or hiding the cookie law banner.

Created attachment 360489 [details] [review]
wizard, wizard-source: Support gratis RHEL boxes

I only re-attached the patches that changed to reduce noise. I hope I got them right.

They are also in gnome-boxes:wip/rishi/rhel.

(In reply to Felipe Borges from comment #47)
> (In reply to Debarshi Ray from comment #46)
> >
> > (ii) We need a way to auto-subscribe the installed VM through
> > subscription-manager. Since the express installation uses a kickstart file,
> > we cannot just put the password in plain text there. The subscription
> > password is the same as the portal's log-in password, so it's risky.
> > subscription-manager supports something called an activation key. It was not
> > clear whether we can use a short lived key to subscribe the VM. The portal
> > developers, and the subscription-manager developers are disjoint sets. So
> > there is a communication gap to be bridged. :)
> 
> To put on record what we just spoke IRL, I suggest:
> 
>   * A Spice channel for the keyring which would allow the host to expose its
> keyring to the guests? (some spice dev could drop a comment clarifying the
> feasibility of it)
> 
>   * A key exchange throughout the webdav channel (guest's agent generates a
> pair, passes its public key to the host which encrypts the authentication
> data and passes it back to the guest to feed it to the subscription-manager
> cli).

Wondering if https://libvirt.org/formatsecret.html could be leveraged to pass this data to the guest? The kickstart file could be put in an encrypted qcow2 volume with an 'ephemeral' secret (assuming the guest being able to access the unencrypted text file is not an issue).

Created attachment 360762 [details] [review]
build: Require webkitgtk+ and json-glib

This is the meson counterpart of commit b1bdcc8.

(In reply to Felipe Borges from comment #56)
> Created attachment 360762 [details] [review] [review]
> build: Require webkitgtk+ and json-glib
> 
> This is the meson counterpart of commit b1bdcc8.

Oops! I totally forgot that we can now build with Meson too.

Review of attachment 360205 [details] [review]:

It is looking good. Thanks!

Review of attachment 360483 [details] [review]:

sure!

Review of attachment 360489 [details] [review]:

Other than the nitpicks here, I think we are ready to go. We might (and probably will) come across bugs/issues here, but I think we are ready to ship this feature.

Thanks a lot for your work!

::: src/wizard-source.vala
@@ +359,3 @@
         });
+
+        var rhel_id = "http://redhat.com/rhel/7.4";

I hope this works over https.

What do we do in the future releases? Bump the version or offer to select which version (if previous version will be offered, of course)?

@@ +564,3 @@
+        var product_code = extract_xpath (contents, "string(/products/product/productCode)", true);
+        if (product_code != "rhel")
+            throw new Boxes.Error.INVALID ("Failed to parse JSON: productCode is not a VALUE");

nitpick: s/JSON/XML/

(In reply to Felipe Borges from comment #60)
> Review of attachment 360489 [details] [review] [review]:
> ::: src/wizard-source.vala
> @@ +359,3 @@
>          });
> +
> +        var rhel_id = "http://redhat.com/rhel/7.4";
> 
> I hope this works over https.

It probably doesn't matter.

The http://redhat.com/rhel/7.4 is the ID of the RHEL 7.4 operating system in osinfo-db. Specifically this file:
  /usr/share/osinfo/os/redhat.com/rhel-7.4.xml

So, it's just an ID that looks like a URI, but doesn't actually point to a network location. Explanation below.

> What do we do in the future releases? Bump the version or offer to select
> which version (if previous version will be offered, of course)?

The high level objective is to show a Shadowman in the wizard's MAIN page, and while the RHEL ISO gets downloaded from the portal. I couldn't find a way to ask libosinfo for a Shadowman logo without specifying an OsinfoOs.

For the other sources, the OsinfoOs is either retrieved from the InstallerMedia for ISOs, or it is obtained from the OsinfoDb while getting a list of all known URLs. However, we can't do that for the Red Hat developer portal. We are neither starting from a local ISO, nor do we have a well-known static URL that can be added to osinfo-db. The download URL from the portal is time-bombed, and won't work after the timeout expires.

Therefore:

(a) I hard-coded the ID of a relatively new edition of RHEL to get a Shadowman for the wizard's MAIN page.

(b) I used the ID to get a RHEL 7.4 OsinfoOs from the database. Once we have the download URL from the portal I insert a "fake" RHEL 7.4 entry in media_urls_store so that a logo can be shown during the download.

It's a bit ugly, but osinfo-db still has entries for really old OSes, and we don't care about the exact 7.4 version number either.

Let me know if there's a better way to get the logo?

> @@ +564,3 @@
> +        var product_code = extract_xpath (contents,
> "string(/products/product/productCode)", true);
> +        if (product_code != "rhel")
> +            throw new Boxes.Error.INVALID ("Failed to parse JSON:
> productCode is not a VALUE");
> 
> nitpick: s/JSON/XML/

Fixed.

Created attachment 361057 [details] [review]
wizard, wizard-source: Support gratis RHEL boxes

(In reply to Debarshi Ray from comment #61)
> 
> The high level objective is to show a Shadowman in the wizard's MAIN page,

Regarding usage of the shadowman logo, https://git.gnome.org/browse/gnome-boxes/tree/README.logos#n163 puts some conditions to its use (just fyi, I have no idea how your changes look, and if they conform to what is in that document :)

(In reply to Christophe Fergeau from comment #63)
> (In reply to Debarshi Ray from comment #61)
> > 
> > The high level objective is to show a Shadowman in the wizard's MAIN page,
> 
> Regarding usage of the shadowman logo,
> https://git.gnome.org/browse/gnome-boxes/tree/README.logos#n163 puts some
> conditions to its use (just fyi, I have no idea how your changes look, and
> if they conform to what is in that document :)

Thanks for pointing this out. I think Debarshi's implementation is compliant with these conditions.

Created attachment 361265 [details] [review]
wizard-source: Group the sources into local files, and everything else

Created attachment 361266 [details] [review]
wizard-source: Separate the groups of sources using headings

Created attachment 361267 [details] [review]
wizard-source: Move the file selection button next to the heading

Created attachment 361268 [details] [review]
wizard-source: Align each page separately, instead of the whole widget

Created attachment 361269 [details] [review]
wizard: Let the WizardSource expand if it wants to

Created attachment 361270 [details] [review]
wizard, wizard-source: Don't align & margin WizardSource

Created attachment 361271 [details] [review]
downloader, wizard: Support a custom filename for downloaded media

Created attachment 361272 [details] [review]
wizard: Allow a custom filename when sanity-checking downloadable URIs

Created attachment 361273 [details] [review]
wizard: Wire the custom filename to the URI validator

Created attachment 361274 [details] [review]
wizard: Wire the custom filename to the downloader

Created attachment 361275 [details] [review]
configure: Require json-glib and webkitgtk+

Created attachment 361276 [details] [review]
build: Require webkitgtk+ and json-glib

Created attachment 361277 [details] [review]
configure: Require gtk+ >= 3.22.20

Created attachment 361278 [details] [review]
wizard-source: Add a WebView with progress indication

Created attachment 361279 [details] [review]
wizard: Rename a method

Created attachment 361280 [details] [review]
wizard, wizard-source: Support gratis RHEL boxes

Changes:

* webkit_web_context_set_preferred_languages was used to set the Accept-Language request header to the list returned by g_get_language_names.

* If the cached JSON/XML is corrupt, then try to get a fresh copy from the server. In the rare case that (i) a server bug or network problem leads to a corrupt JSON/XML, the user can try again, or (ii) if the user manages to manually corrupt the cache, then the code will automatically override it.

That's all from my end. I don't have anything else coming up.

I have reattached all the patches so that they cleanly apply. I tried to retain the statuses of the ones that were unchanged. I hope I didn't mess it up.

Review of attachment 361276 [details] [review]:

Do you want to squash this with attachment 361275 [details] [review]? At least we need to update the commit hash in the commit message. :)

(In reply to Debarshi Ray from comment #82)
> Review of attachment 361276 [details] [review] [review]:
> 
> Do you want to squash this with attachment 361275 [details] [review] [review]? At
> least we need to update the commit hash in the commit message. :)

Done, after discussing in #boxes on GIMPNet. I am holding back from reattaching the patches to avoid bugzilla noise. Updated gnome-boxes:wip/rishi/rhel.

Review of attachment 361280 [details] [review]:

lgtm!

::: src/wizard-source.vala
@@ +375,3 @@
+        //
+        // Ideally, distributions would be a first-class object in libosinfo, so
+        //

Thanks for these comments.

Review of attachment 361278 [details] [review]:

sorry, I missed this one. Looks good!

Created attachment 361430 [details] [review]
build: Require webkitgtk+ and json-glib

Squashed attachment 361275 [details] [review] and attachment 361276 [details] [review]

I filed bug 788886 for automatically subscribing the VMs, and dumped comment 47 and comment 55 into it.

Thanks for all the reviews and suggestions!