Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix multi-deletion of DHCP leases #2792

Merged
merged 5 commits into from
Jun 6, 2024
Merged

Fix multi-deletion of DHCP leases #2792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
75f54ff
Fix/simplify mass deletion of tables, simplify deleting local DNS / C…
DL6ER May 30, 2024
2d12a28
Fix multi-deletion of DHCP leases
DL6ER Jun 3, 2024
420c5a8
Toasts need to be stored globally or the variables may overwrite each…
DL6ER Jun 3, 2024
edda66f
Apply the same fixes for the messages table
DL6ER Jun 3, 2024
61acb4a
Apply suggestions from code review
DL6ER Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 27 additions & 31 deletions scripts/pi-hole/js/messages.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
* Please see LICENSE file for your rights under this license. */

/* global utils:false */
var table;
var table,
toasts = {};

$(function () {
var ignoreNonfatal = localStorage
Expand Down Expand Up @@ -98,13 +99,10 @@ $(function () {
className: "btn-sm datatable-bt deleteSelected",
action: function () {
// For each ".selected" row ...
var ids = [];
$("tr.selected").each(function () {
// ... add the row identified by "data-id".
ids.push(parseInt($(this).attr("data-id"), 10));
// ... delete the row identified by "data-id".
delMsg($(this).attr("data-id"));
});
// Delete all selected rows at once
delMsg(ids);
},
},
],
Expand Down Expand Up @@ -150,26 +148,16 @@ $.fn.dataTable.Buttons.defaults.dom.container.className = "dt-buttons";

function deleteMessage() {
// Passes the button data-del-id attribute as ID
var ids = [parseInt($(this).attr("data-del-id"), 10)];

// Check input validity
if (!Array.isArray(ids)) return;

// Exploit prevention: Return early for non-numeric IDs
for (var id in ids) {
if (Object.hasOwnProperty.call(ids, id)) {
if (typeof ids[id] !== "number") return;
delMsg(ids);
}
}
delMsg($(this).attr("data-del-id"));
}

function delMsg(ids) {
function delMsg(id) {
id = parseInt(id, 10);
utils.disableAll();
utils.showAlert("info", "", "Deleting message...");
toasts[id] = utils.showAlert("info", "", "Deleting message...", "ID: " + id, null);

$.ajax({
url: "/api/info/messages/" + ids,
url: "/api/info/messages/" + id,
method: "DELETE",
})
.done(function (response) {
Expand All @@ -178,19 +166,21 @@ function delMsg(ids) {
utils.showAlert(
"success",
"far fa-trash-alt",
"Successfully deleted " + ids.length + " message" + (ids.length > 1 ? "s" : ""),
""
"Successfully deleted message",
Copy link
Member

@rdwebdesign rdwebdesign Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ID is returned on error. I think we could also show the ID on success.

Suggested change
"Successfully deleted message",
"Successfully deleted message ID: " + id,

Without the ID, if you delete 5 messages there will be 5 toasts with the exactly same text.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two comments: If we use your suggestion, I'd suggest an additional space between : and ". Otherwise, how about putting the ID into the second line (currently null to be disabled)? This is also where we are putting domains/regex, etc. in other toasts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with both solutions:

  • space after "ID:" (suggestion updated) or
  • replacing the null parameter with ID: XX.

I just don't want to remove the string "ID:" to avoid confusion between the ID and the count of deleted items.

"ID: " + id,
toasts[id]
);
// Loop over id in case of multiple IDs
for (var id in ids) {
if (Object.hasOwnProperty.call(ids, id)) {
table.row(id).remove();
}
}
table.row(id).remove();

table.draw(false).ajax.reload(null, false);
} else {
utils.showAlert("error", "", "Error while deleting message: " + ids, response.message);
utils.showAlert(
"error",
"",
"Error while deleting message: " + id,
response.message,
toasts[id]
);
}

// Clear selection after deletion
Expand All @@ -202,7 +192,13 @@ function delMsg(ids) {
)
.fail(function (jqXHR, exception) {
utils.enableAll();
utils.showAlert("error", "", "Error while deleting message: " + ids, jqXHR.responseText);
utils.showAlert(
"error",
"",
"Error while deleting message: " + id,
jqXHR.responseText,
toasts[id]
);
console.log(exception); // eslint-disable-line no-console
});
}
50 changes: 28 additions & 22 deletions scripts/pi-hole/js/settings-dhcp.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@

/* global utils:false, setConfigValues: false, apiFailure: false */

var dhcpLeaesTable = null;
var dhcpLeaesTable = null,
toasts = {};

// DHCP leases tooltips
$(function () {
Expand Down Expand Up @@ -115,13 +116,10 @@ $(function () {
className: "btn-sm datatable-bt deleteSelected",
action: function () {
// For each ".selected" row ...
var ids = [];
$("tr.selected").each(function () {
// ... add the row identified by "data-id".
ids.push(parseInt($(this).attr("data-id"), 10));
// ... delete the row identified by "data-id".
delLease($(this).attr("data-id"));
});
// Delete all selected rows at once
delLease(ids);
},
},
],
Expand Down Expand Up @@ -161,34 +159,36 @@ $(function () {

function deleteLease() {
// Passes the button data-del-id attribute as IP
var ips = [$(this).attr("data-del-ip")];

// Check input validity
if (!Array.isArray(ips)) return;

// Exploit prevention: Return early for non-numeric IDs
for (var ip in ips) {
if (Object.hasOwnProperty.call(ips, ip)) {
delLease(ips);
}
}
delLease($(this).attr("data-del-ip"));
}

function delLease(ip) {
utils.disableAll();
utils.showAlert("info", "", "Deleting lease...");
toasts[ip] = utils.showAlert("info", "", "Deleting lease...", ip, null);

$.ajax({
url: "/api/dhcp/leases/" + ip,
url: "/api/dhcp/leases/" + encodeURIComponent(ip),
method: "DELETE",
})
.done(function (response) {
utils.enableAll();
if (response === undefined) {
utils.showAlert("success", "far fa-trash-alt", "Successfully deleted lease", "");
utils.showAlert(
"success",
"far fa-trash-alt",
"Successfully deleted lease",
ip,
toasts[ip]
);
dhcpLeaesTable.ajax.reload(null, false);
} else {
utils.showAlert("error", "", "Error while deleting lease: " + ip, response.lease);
utils.showAlert(
"error",
"",
"Error while deleting lease: " + ip,
response.lease,
toasts[ip]
);
}

// Clear selection after deletion
Expand All @@ -197,7 +197,13 @@ function delLease(ip) {
})
.fail(function (jqXHR, exception) {
utils.enableAll();
utils.showAlert("error", "", "Error while deleting lease: " + ip, jqXHR.responseText);
utils.showAlert(
"error",
"",
"Error while deleting lease: " + ip,
jqXHR.responseText,
toasts[ip]
);
console.log(exception); // eslint-disable-line no-console
});
}
Expand Down
15 changes: 2 additions & 13 deletions scripts/pi-hole/js/settings-dns-records.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,19 +141,8 @@ $(function () {
});

function deleteRecord() {
// Get the tags
var tags = [$(this).attr("data-tag")];
var types = [$(this).attr("data-type")];
// Check input validity
if (!Array.isArray(tags)) return;

// Exploit prevention: Return early for non-numeric IDs
for (var tag in tags) {
if (Object.hasOwnProperty.call(tags, tag)) {
if (types[0] === "hosts") delHosts(tags);
else delCNAME(tags);
}
}
if ($(this).attr("data-type") === "hosts") delHosts($(this).attr("data-tag"));
else delCNAME($(this).attr("data-tag"));
}

function delHosts(elem) {
Expand Down
32 changes: 22 additions & 10 deletions scripts/pi-hole/js/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ function padNumber(num) {
}

var showAlertBox = null;
function showAlert(type, icon, title, message) {
function showAlert(type, icon, title, message, toast) {
const options = {
title: "&nbsp;<strong>" + escapeHtml(title) + "</strong><br>",
message: escapeHtml(message),
Expand Down Expand Up @@ -138,16 +138,28 @@ function showAlert(type, icon, title, message) {
return;
}

if (type === "info") {
// Create a new notification for info boxes
showAlertBox = $.notify(options, settings);
} else if (showAlertBox !== null) {
// Update existing notification for other boxes (if available)
showAlertBox.update(options);
showAlertBox.update(settings);
if (toast === undefined) {
if (type === "info") {
// Create a new notification for info boxes
showAlertBox = $.notify(options, settings);
return showAlertBox;
} else if (showAlertBox !== null) {
// Update existing notification for other boxes (if available)
showAlertBox.update(options);
showAlertBox.update(settings);
return showAlertBox;
} else {
// Create a new notification for other boxes if no previous info box exists
return $.notify(options, settings);
}
} else if (toast === null) {
// Always create a new toast
return $.notify(options, settings);
} else {
// Create a new notification for other boxes if no previous info box exists
$.notify(options, settings);
// Update existing toast
toast.update(options);
toast.update(settings);
return toast;
}
}

Expand Down