Fixes #34300 - correctly construct the fqdn for redirect #9047

ezr-ondrej · 2022-01-21T16:45:43Z

In host form we've used a hack in 2876578 to redirect to the new Host details page.
It relied on the primary interface being the first interface in the table.

This fixes it to always incur the correct fqdn.

theforeman-bot · 2022-01-21T16:45:46Z

Issues: #34300

ezr-ondrej · 2022-01-21T17:25:26Z

[test foreman]

ekohl · 2022-01-21T18:12:12Z

app/assets/javascripts/host_edit_interfaces.js

+function construct_fqdn() {
  var host_name = $('#host_name').val();
  var domain_name = primary_nic_form()
    .find('.interface_domain option:selected')
    .text();
+  return fqdn(host_name, domain_name);
+}


I think unmanaged hosts don't have NICs, but I can't check right now since I don't have an unmanaged host. Does this work in that case?

I'll test that out, tho I've also remembered the setting append_domain_name_for_hosts and for that = true it would not work almost for sure.
I'll test both cases and see if I can figure something better.

EDIT: issue is obviously with append_domain_name_for_hosts = false

I do think the ID of the host is much more reliable than the FQDN, which as we see here is unstable.

I'd agree, but you do not have the id at the time we now calculate the redirect.
I think we need a better approach. @amirfefer I've added test cases for both failing scenarios.
My approach is fixing the first, but as expected it fails for the second (append_domain_name_for_hosts = false)

Shouldn't the interaction be to POST and then wait for the response. That should give you the correct URL. Guessing it client side is IMHO a bad way. What if the request fails? Then it will result in a 404.

It should, but as many things in life, theory and practice do not overlap :(

Just to get this whole thing clear, even if it's just in my head. The code we're talking about is:

foreman/app/assets/javascripts/host_edit.js

Lines 161 to 201 in 2152852

function submit_with_all_params() {

var fqdn = $('.fqdn')[0].textContent;

$('form.hostresource-form input[type="submit"]').attr('disabled', true);

stop_pooling = false;

$('body').css('cursor', 'progress');

clear_errors();

animate_progress();

$.ajax({

type: 'POST',

url: $('form').attr('action'),

data: serializeForm(),

success: function(response) {

// workaround for redirecting to the new host details page

if (!response.includes('id="main"')) {

return tfm.nav.pushUrl('/new/hosts/' + fqdn);

}

$('#host-progress').hide();

$('#content').replaceWith($('#content', response));

$(document.body).trigger('ContentLoad');

if ($("[data-history-url]").exists()) {

history.pushState({}, 'Host show', $("[data-history-url]").data('history-url'));

}

},

error: function(response) {

$('#content').html(response.responseText);

},

complete: function() {

stop_pooling = true;

$('body').css('cursor', 'auto');

$('form input[type="submit"]').attr('disabled', false);

if (window.location.pathname !== tfm.tools.foremanUrl('/hosts/new')) {

// We got redirected to the show page, need to clear the title override

tfm.store.dispatch('updateBreadcrumbTitle');

}

},

});

return false;

}

And really, in particular the success bit:

foreman/app/assets/javascripts/host_edit.js

Lines 173 to 185 in 2152852

success: function(response) {

// workaround for redirecting to the new host details page

if (!response.includes('id="main"')) {

return tfm.nav.pushUrl('/new/hosts/' + fqdn);

}

$('#host-progress').hide();

$('#content').replaceWith($('#content', response));

$(document.body).trigger('ContentLoad');

if ($("[data-history-url]").exists()) {

history.pushState({}, 'Host show', $("[data-history-url]").data('history-url'));

}

},

That needs to ensure you're on the correct page. In particular the whole success bit. I still don't really see a reason we can't make sure response has a Location field and then push that URL to the state.

For the creation we have:

foreman/app/controllers/hosts_controller.rb

Line 99 in 2152852

process_success :success_redirect => current_host_details_path(@host)

Then for updating we have:

foreman/app/controllers/hosts_controller.rb

Line 117 in 2152852

process_success :success_redirect => current_host_details_path(@host)

This means both go through current_host_details_path:

foreman/app/controllers/hosts_controller.rb

Lines 926 to 928 in 2152852

def current_host_details_path(host)

Setting['host_details_ui'] ? host_details_page_path(host) : host_path(host)

end

So what's the reason we can't use Location header from that returned response? The server already knows the URL and sends it. We just have to use it.

Note that according to the jquery docs you can specify success as Type: Function( Anything data, String textStatus, jqXHR jqXHR ). The jqXHR argument has a getResponseHeader(name) method, so I think it should work to change the signature and use new_url = jqXHR.getResponseHeader('Location').

I thought it would be in the header, but apparently that object (jqXHR) has headers for the redirect, only content for the final page, I thought we could use responseURL, but this is not implemented in jquery: jquery/jquery#4339

< jqXHR.getResponseHeader('Location') > null < jqXHR.getAllResponseHeaders() > `cache-control: max-age=0, private, must-revalidate\r\ncontent-security-policy: default-src 'self' localhost:3808; child-src 'self'; connect-src 'self' ws: wss: localhost:3808; font-src 'self' data: http://localhost:3808; img-src 'self' data: localhost:3808; script-src 'unsafe-eval' 'unsafe-inline' 'self' localhost:3808; style-src 'unsafe-inline' 'self' localhost:3808\r\ncontent-type: text/html; charset=utf-8\r\netag: W/"4ae9ec52826ccc3a67a7eadbd03c41f1"\r\ntransfer-encoding: chunked\r\nx-content-type-options: nosniff\r\nx-download-options: noopen\r\nx-frame-options: sameorigin\r\nx-permitted-cross-domain-policies: none\r\nx-request-id: d6000a26-bda2-44cb-be4f-93d44f9f4469\r\nx-runtime: 0.125798\r\nx-xss-protection: 1; mode=block\r\n`

Man, software ...

Would it make sense to use native browser methods rather than jquery? It's not like the bad old days where you really wanted jquery for cross browser compatibility.

Yeah, it would most definitelly be better, but we would drop IE10 support and it would mean rewriting quite a significant portion of host_edit.js which I'd not recommend cherry-picking in 3.1

Are you thinking about the fetch API that IE10 doesn't support? Couldn't you use XmlHttpRequest, which is available for a long time?

Also, it's unclear if we actually need to support IE10. Patternfly 4 doesn't and we've included that since Foreman 2.1: theforeman/theforeman.org#1933. Perhaps we already broke it anyway?

ekohl · 2022-01-24T14:30:50Z

app/assets/javascripts/host_edit.js

    success: function(response) {
      // workaround for redirecting to the new host details page
      if (!response.includes('id="main"')) {
-        return tfm.nav.pushUrl('/new/hosts/' + fqdn);
+        return tfm.nav.pushUrl('/new/hosts/' + host_unique_name);


Please see #9047 (comment).

Unfortunatelly I've already tried that. I'm adding those parameters to indicate those were tried with no success.

As of now this fixes the issue for the two cases that I'm aware of and added tests to. It is not perfect and it is hacky, but without serious rewrite of host_edit.js I do not see a better way.

ekohl

@amirfefer could you take a look at the JS code?

ekohl · 2022-01-24T20:07:18Z

app/assets/javascripts/host_edit_interfaces.js

-function update_fqdn() {
-  var host_name = $('#host_name').val();
+function construct_host_name() {
+  var $host_name = $('#host_name')


Why $host_name? is that even valid for a variable name? My JS knowledge is lacking here.

I'm marking the jQuery holding variables like that. It is valid, but weird, so maybe better to avoid if it's causing confusion 👍

I agree that it's confusing a bit

amirfefer

Thanks @ezr-ondrej and @ekohl

I agree that we should have a better approach, I guess it would be easier when we will drop the old page.

amirfefer · 2022-01-25T14:45:25Z

app/assets/javascripts/host_edit_interfaces.js

-function update_fqdn() {
-  var host_name = $('#host_name').val();
+function construct_host_name() {
+  var $host_name = $('#host_name')


I agree that it's confusing a bit

amirfefer · 2022-01-25T14:48:23Z

app/assets/javascripts/host_edit.js

      // workaround for redirecting to the new host details page
      if (!response.includes('id="main"')) {
-        return tfm.nav.pushUrl('/new/hosts/' + fqdn);
+        return tfm.nav.pushUrl('/new/hosts/' + host_unique_name);


do we need to wrap it with foremanUrl ?

no, it is not used for anything useful, but it might in the future :)

amirfefer · 2022-01-25T16:18:54Z

Tested with managed and unmanaged hosts, creating and editing. works well 👍

In host form we've used a hack in 2876578 to redirect to the new Host details page. It relied on the primary interface being the first interface in the table. This fixes it to always incur the correct fqdn.

ezr-ondrej · 2022-01-26T12:47:51Z

[test foreman]

ezr-ondrej · 2022-01-26T14:01:06Z

The test will not finish. It's getting consistently stuck :/

ekohl · 2022-01-26T14:41:14Z

CI is stuck so I'm going to merge it now. I trust @amirfefer who said in #9047 (comment) he verified this.

ekohl · 2022-01-26T14:43:12Z

Cherry pick to 3.1: 700d707

MariaAga · 2022-02-14T16:39:34Z

Not sure how host names work but I'm having an issue with this pr where the host addie-speciale.marla-agcaoili.example.org gets redirected to addie-speciale.marla-agcaoili.example.org.marla-agcaoili.example.org and gets 404

{
    "host_name": "addie-speciale.marla-agcaoili.example.org",
    "domain_name": "marla-agcaoili.example.org",
    "construct_host_name": "addie-speciale.marla-agcaoili.example.org.marla-agcaoili.example.org"
}

theforeman-bot added Needs testing Not yet reviewed Legacy JS PRs making changes in the legacy Javascript stack labels Jan 21, 2022

ezr-ondrej requested a review from amirfefer January 21, 2022 16:45

ekohl reviewed Jan 21, 2022

View reviewed changes

ezr-ondrej force-pushed the fqdn_buiding branch 3 times, most recently from 5fa5bca to 528b9c9 Compare January 24, 2022 14:26

ekohl reviewed Jan 24, 2022

View reviewed changes

ezr-ondrej force-pushed the fqdn_buiding branch from 528b9c9 to 7f72dbb Compare January 24, 2022 15:08

ekohl reviewed Jan 24, 2022

View reviewed changes

amirfefer reviewed Jan 25, 2022

View reviewed changes

ezr-ondrej assigned amirfefer Jan 26, 2022

Fixes #34300 - correctly construct the fqdn for redirect

a2bd297

In host form we've used a hack in 2876578 to redirect to the new Host details page. It relied on the primary interface being the first interface in the table. This fixes it to always incur the correct fqdn.

ezr-ondrej force-pushed the fqdn_buiding branch from 7f72dbb to a2bd297 Compare January 26, 2022 10:46

ekohl approved these changes Jan 26, 2022

View reviewed changes

theforeman-bot removed the Not yet reviewed label Jan 26, 2022

ekohl merged commit 08a74cb into theforeman:develop Jan 26, 2022

ezr-ondrej deleted the fqdn_buiding branch January 26, 2022 20:53

amirfefer mentioned this pull request Mar 8, 2022

Fixes #34572 - fix invalid redirect after host edit #9140

Merged

ofedoren mentioned this pull request Feb 13, 2023

Fixes #36160 - Redefine append domain names setting #9613

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #34300 - correctly construct the fqdn for redirect #9047

Fixes #34300 - correctly construct the fqdn for redirect #9047

ezr-ondrej commented Jan 21, 2022

theforeman-bot commented Jan 21, 2022

ezr-ondrej commented Jan 21, 2022

ekohl Jan 21, 2022

ezr-ondrej Jan 22, 2022 •

edited

Loading

ekohl Jan 24, 2022

ezr-ondrej Jan 24, 2022

ekohl Jan 24, 2022

ekohl Jan 24, 2022

ezr-ondrej Jan 24, 2022

ekohl Jan 24, 2022

ezr-ondrej Jan 24, 2022

ekohl Jan 24, 2022

ekohl Jan 24, 2022

ezr-ondrej Jan 24, 2022

ezr-ondrej Jan 24, 2022

ekohl left a comment

ekohl Jan 24, 2022

ezr-ondrej Jan 25, 2022

amirfefer Jan 25, 2022

amirfefer left a comment

amirfefer Jan 25, 2022

amirfefer Jan 25, 2022

ezr-ondrej Jan 26, 2022

amirfefer commented Jan 25, 2022

ezr-ondrej commented Jan 26, 2022

ezr-ondrej commented Jan 26, 2022

ekohl commented Jan 26, 2022

ekohl commented Jan 26, 2022

MariaAga commented Feb 14, 2022

	function submit_with_all_params() {
	var fqdn = $('.fqdn')[0].textContent;
	$('form.hostresource-form input[type="submit"]').attr('disabled', true);
	stop_pooling = false;
	$('body').css('cursor', 'progress');
	clear_errors();
	animate_progress();

	$.ajax({
	type: 'POST',
	url: $('form').attr('action'),
	data: serializeForm(),
	success: function(response) {
	// workaround for redirecting to the new host details page
	if (!response.includes('id="main"')) {
	return tfm.nav.pushUrl('/new/hosts/' + fqdn);
	}

	$('#host-progress').hide();
	$('#content').replaceWith($('#content', response));
	$(document.body).trigger('ContentLoad');
	if ($("[data-history-url]").exists()) {
	history.pushState({}, 'Host show', $("[data-history-url]").data('history-url'));
	}
	},
	error: function(response) {
	$('#content').html(response.responseText);
	},
	complete: function() {
	stop_pooling = true;
	$('body').css('cursor', 'auto');
	$('form input[type="submit"]').attr('disabled', false);
	if (window.location.pathname !== tfm.tools.foremanUrl('/hosts/new')) {
	// We got redirected to the show page, need to clear the title override
	tfm.store.dispatch('updateBreadcrumbTitle');
	}
	},
	});

	return false;
	}

	def current_host_details_path(host)
	Setting['host_details_ui'] ? host_details_page_path(host) : host_path(host)
	end

Fixes #34300 - correctly construct the fqdn for redirect #9047

Fixes #34300 - correctly construct the fqdn for redirect #9047

Conversation

ezr-ondrej commented Jan 21, 2022

theforeman-bot commented Jan 21, 2022

ezr-ondrej commented Jan 21, 2022

Choose a reason for hiding this comment

ezr-ondrej Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amirfefer commented Jan 25, 2022

ezr-ondrej commented Jan 26, 2022

ezr-ondrej commented Jan 26, 2022

ekohl commented Jan 26, 2022

ekohl commented Jan 26, 2022

MariaAga commented Feb 14, 2022

ezr-ondrej Jan 22, 2022 •

edited

Loading