Adds In Game Rank Editing (Permissions Panel Cleanup) (#91873)

## About The Pull Request

Ok there's a lot here, sorry bout that.

- Cleaned up the permissions panel backend pretty signficantly
- Added some extra security measures to said code, mostly proc call
checks
- Properly implemented filtering code jordie wrote years and years ago
for permissions logs
- Cleaned up the permissions ui generally, more bars, nicer lookin
stuff, etc
- Fixed the Management panel's relationship with combined roles, and
renamed it to Housekeeping. Its display is expanded too.
- Added tracking to rank datums on where exactly they came from
- Added a new tab to the permissions panel which allows the modification
and deletion of ranks
- Beefed up rank modification to try and avoid accidential temp rank
additions to the db

I'm doing my best to avoid perms escalation issues, tho they are always
possible right.
Also, got mad at some query cleanup handling, did a pass on it. this
isn't nearly all of em, but it's some.

## Why It's Good For The Game

I realized there is no way to, in game, cleanly edit/create ranks, and
that the way the existing system worked was quite opaque.
I'm trying to fix that here. It does mean potentially opening up DB rank
deletion/modification to bad actors, but frankly I am not overly worried
about that. Admin modification has always been a vulnerability so like.

Here's a video with my changes (mostly, it's lightly outdated)
https://file.house/XqME7KWKk0ULj4ZUkJ5reg==.mp4

## Changelog
🆑
refactor: Fucked with admin rank setup very slightly, please yell at me
if anything is wrong.
admin: Updated the permissions panel to be a good bit more user
friendly, added rank management support to it.
server: I've added code that gives the game modification/deletion perms
for the rank table, be made aware.
/🆑

---------

Co-authored-by: san7890 <the@san7890.com>

code/__DEFINES/admin.dm

@@ -105,6 +105,28 @@
 #define AHELP_CLOSED 2
 #define AHELP_RESOLVED 3
 
+// Page numbers for the Permission Panel
+#define PERMISSIONS_PAGE_PERMISSIONS 1
+#define PERMISSIONS_PAGE_RANKS 2
+#define PERMISSIONS_PAGE_LOGGING 3
+#define PERMISSIONS_PAGE_HOUSEKEEPING 4
+
+// Actions that can be logged in the admin_log table, excepting NONE
+#define PERMISSIONS_ACTION_ADMIN_ADDED "add admin"
+#define PERMISSIONS_ACTION_ADMIN_REMOVED "remove admin"
+#define PERMISSIONS_ACTION_ADMIN_RANK_CHANGED "change admin rank"
+#define PERMISSIONS_ACTION_RANK_ADDED "add rank"
+#define PERMISSIONS_ACTION_RANK_REMOVED "remove rank"
+#define PERMISSIONS_ACTION_RANK_CHANGED "change rank flags"
+#define PERMISSIONS_ACTION_NONE "none"
+
+// The types of ranks you can have
+#define RANK_SOURCE_LOCAL "rank_local"
+#define RANK_SOURCE_TXT "rank_txt"
+#define RANK_SOURCE_DB "rank_db"
+#define RANK_SOURCE_BACKUP "rank_backup"
+#define RANK_SOURCE_TEMPORARY "rank_temp"
+
 /// Amount of time after the round starts that the player disconnect report is issued.
 #define ROUNDSTART_LOGOUT_REPORT_TIME (21 MINUTES) // SKYRAT EDIT CHANGE - ORIGINAL: 10 MINUTES
 

code/__DEFINES/span.dm

@@ -4,6 +4,7 @@
 // Sorted alphabetically
 #define span_abductor(str) ("<span class='abductor'>" + str + "</span>")
 #define span_admin(str) ("<span class='admin'>" + str + "</span>")
+#define span_adminprefix(str) ("<span class='admin prefix'>" + str + "</span>")
 #define span_adminhelp(str) ("<span class='adminhelp'>" + str + "</span>")
 #define span_adminnotice(str) ("<span class='adminnotice'>" + str + "</span>")
 #define span_adminobserverooc(str) ("<span class='adminobserverooc'>" + str + "</span>")

code/controllers/subsystem/ipintel.dm

@@ -129,7 +129,7 @@ SUBSYSTEM_DEF(ipintel)
 	)
 	query.warn_execute()
 	query.sync()
-	qdel(query)
+	QDEL_NULL(query)
 
 /datum/controller/subsystem/ipintel/proc/fetch_cached_ip_intel(address)
 	if (!SSdbcore.Connect())
@@ -152,7 +152,7 @@ SUBSYSTEM_DEF(ipintel)
 
 	query.NextRow()
 	var/list/data = query.item
-	qdel(query)
+	QDEL_NULL(query)
 	if(isnull(data))
 		return null
 
@@ -191,7 +191,7 @@ SUBSYSTEM_DEF(ipintel)
 		return FALSE
 	query.NextRow()
 	. = !!query.item // if they have a row, they are whitelisted
-	qdel(query)
+	QDEL_NULL(query)
 
 
 ADMIN_VERB(ipintel_allow, R_BAN, "Whitelist Player VPN", "Allow a player to connect even if they are using a VPN.", ADMIN_CATEGORY_IPINTEL, ckey as text)
@@ -215,7 +215,7 @@ ADMIN_VERB(ipintel_allow, R_BAN, "Whitelist Player VPN", "Allow a player to conn
 	)
 	query.warn_execute()
 	query.sync()
-	qdel(query)
+	QDEL_NULL(query)
 	message_admins("IPINTEL: [key_name_admin(user)] has whitelisted '[ckey]'")
 
 ADMIN_VERB(ipintel_revoke, R_BAN, "Revoke Player VPN Whitelist", "Revoke a player's VPN whitelist.", ADMIN_CATEGORY_IPINTEL, ckey as text)
@@ -231,7 +231,7 @@ ADMIN_VERB(ipintel_revoke, R_BAN, "Revoke Player VPN Whitelist", "Revoke a playe
 	)
 	query.warn_execute()
 	query.sync()
-	qdel(query)
+	QDEL_NULL(query)
 	message_admins("IPINTEL: [key_name_admin(user)] has revoked the VPN whitelist for '[ckey]'")
 
 /client/proc/check_ip_intel()

code/datums/json_database.dm

@@ -14,7 +14,7 @@
 
 /datum/json_database/New(filepath)
 	if (IsAdminAdvancedProcCall())
-		to_chat(usr, "<span class='admin prefix'>json_database creation, linking to [html_encode(filepath)], was blocked.</span>", confidential = TRUE)
+		to_chat(usr, span_adminprefix("json_database creation, linking to [html_encode(filepath)], was blocked."), confidential = TRUE)
 		return
 
 	ASSERT(isnull(existing_json_database[filepath]), "[filepath] already has an associated json_database. You must expose it somehow and use that instead of making a new one.")

code/modules/admin/admin_ranks.dm

@@ -5,13 +5,20 @@ GLOBAL_LIST_EMPTY(protected_ranks) //admin ranks loaded from txt
 GLOBAL_PROTECT(protected_ranks)
 
 /datum/admin_rank
+	/// Rank name, key'd to the db
 	var/name = "NoRank"
+	/// Rank source, see RANK_SOURCE_TXT and friends
+	var/source = null
+	/// Our functional rights, these are what we actually use in game
 	var/rights = R_DEFAULT
-	var/exclude_rights = NONE
+	/// Rights we're allowed to use, pre filtering
 	var/include_rights = NONE
+	/// These are the rights of include_rights we aren't allowed to use. Frankly I have no idea why this exists
+	var/exclude_rights = NONE
+	/// Rights we're allowed to edit on other folks, impact of this is dependent on R_PERMISSIONS and R_DBRANKS
 	var/can_edit_rights = NONE
 
-/datum/admin_rank/New(init_name, init_rights, init_exclude_rights, init_edit_rights)
+/datum/admin_rank/New(init_name, init_source, init_rights, init_exclude_rights, init_edit_rights)
 	if(IsAdminAdvancedProcCall())
 		alert_to_permissions_elevation_attempt(usr)
 		if (name == "NoRank") //only del if this is a true creation (and not just a New() proc call), other wise trialmins/coders could abuse this to deadmin other admins
@@ -19,6 +26,10 @@ GLOBAL_PROTECT(protected_ranks)
 			CRASH("Admin proc call creation of admin datum")
 		return
 	name = init_name
+	source = init_source
+	if(!source)
+		qdel(src)
+		CRASH("Admin rank created without a source.")
 	if(!name)
 		qdel(src)
 		CRASH("Admin rank created without name.")
@@ -102,11 +113,24 @@ GLOBAL_PROTECT(protected_ranks)
 			if(3)
 				can_edit_rights |= flag
 
+/datum/admin_rank/proc/pretty_print_source()
+	switch(source)
+		if(RANK_SOURCE_LOCAL)
+			return "Localhost"
+		if(RANK_SOURCE_TXT)
+			return "admin_ranks.txt"
+		if(RANK_SOURCE_DB)
+			return "Database"
+		if(RANK_SOURCE_BACKUP)
+			return "Backup JSON"
+		if(RANK_SOURCE_TEMPORARY)
+			return "Temporary"
+
 /// Loads admin ranks.
 ///	Return a list containing the backup data if they were loaded from the database backup json
 /proc/load_admin_ranks(dbfail, no_update)
 	if(IsAdminAdvancedProcCall())
-		to_chat(usr, "<span class='admin prefix'>Admin Reload blocked: Advanced ProcCall detected.</span>", confidential = TRUE)
+		to_chat(usr, span_adminprefix("Admin Reload blocked: Advanced ProcCall detected."), confidential = TRUE)
 		return
 	GLOB.admin_ranks.Cut()
 	GLOB.protected_ranks.Cut()
@@ -115,17 +139,17 @@ GLOBAL_PROTECT(protected_ranks)
 	var/datum/admin_rank/previous_rank
 	var/regex/admin_ranks_regex = new(@"^Name\s*=\s*(.+?)\s*\n+Include\s*=\s*([\l @]*?)\s*\n+Exclude\s*=\s*([\l @]*?)\s*\n+Edit\s*=\s*([\l @]*?)\s*\n*$", "gm")
 	while(admin_ranks_regex.Find(ranks_text))
-		var/datum/admin_rank/R = new(admin_ranks_regex.group[1])
+		var/datum/admin_rank/txt_rank = new(admin_ranks_regex.group[1], RANK_SOURCE_TXT)
-		if(!R)
+		if(QDELETED(txt_rank))
 			continue
 		var/count = 1
 		for(var/i in admin_ranks_regex.group - admin_ranks_regex.group[1])
 			if(i)
-				R.process_keyword(i, count, previous_rank)
+				txt_rank.process_keyword(i, count, previous_rank)
 			count++
-		GLOB.admin_ranks += R
+		GLOB.admin_ranks += txt_rank
-		GLOB.protected_ranks += R
+		GLOB.protected_ranks += txt_rank
-		previous_rank = R
+		previous_rank = txt_rank
 	if(!CONFIG_GET(flag/admin_legacy_system) && !dbfail)
 		if(CONFIG_GET(flag/load_legacy_ranks_only))
 			if(!no_update)
@@ -144,14 +168,15 @@ GLOBAL_PROTECT(protected_ranks)
 						if(R.name == rank_name) //this rank was already loaded from txt override
 							skip = 1
 							break
-					if(!skip)
+					if(skip)
-						var/rank_flags = text2num(query_load_admin_ranks.item[2])
+						continue
-						var/rank_exclude_flags = text2num(query_load_admin_ranks.item[3])
+					var/rank_flags = text2num(query_load_admin_ranks.item[2])
-						var/rank_can_edit_flags = text2num(query_load_admin_ranks.item[4])
+					var/rank_exclude_flags = text2num(query_load_admin_ranks.item[3])
-						var/datum/admin_rank/R = new(rank_name, rank_flags, rank_exclude_flags, rank_can_edit_flags)
+					var/rank_can_edit_flags = text2num(query_load_admin_ranks.item[4])
-						if(!R)
+					var/datum/admin_rank/db_rank = new(rank_name, RANK_SOURCE_DB, rank_flags, rank_exclude_flags, rank_can_edit_flags)
-							continue
+					if(QDELETED(db_rank))
-						GLOB.admin_ranks += R
+						continue
+					GLOB.admin_ranks += db_rank
 			qdel(query_load_admin_ranks)
 	//load ranks from backup file
 	if(dbfail)
@@ -167,10 +192,10 @@ GLOBAL_PROTECT(protected_ranks)
 					skip = TRUE
 			if(skip)
 				continue
-			var/datum/admin_rank/R = new("[J]", json["ranks"]["[J]"]["include rights"], json["ranks"]["[J]"]["exclude rights"], json["ranks"]["[J]"]["can edit rights"])
+			var/datum/admin_rank/json_rank = new("[J]", RANK_SOURCE_BACKUP, json["ranks"]["[J]"]["include rights"], json["ranks"]["[J]"]["exclude rights"], json["ranks"]["[J]"]["can edit rights"])
-			if(!R)
+			if(QDELETED(json_rank))
 				continue
-			GLOB.admin_ranks += R
+			GLOB.admin_ranks += json_rank
 		return json
 	#ifdef TESTING
 	var/msg = "Permission Sets Built:\n"
@@ -307,7 +332,7 @@ GLOBAL_PROTECT(protected_ranks)
 	set waitfor = FALSE
 
 	if(IsAdminAdvancedProcCall())
-		to_chat(usr, "<span class='admin prefix'>Admin rank DB Sync blocked: Advanced ProcCall detected.</span>", confidential = TRUE)
+		to_chat(usr, span_adminprefix("Admin rank DB Sync blocked: Advanced ProcCall detected."), confidential = TRUE)
 		return
 
 	var/list/sql_ranks = list()
@@ -351,7 +376,7 @@ GLOBAL_PROTECT(protected_ranks)
 
 /proc/sync_admins_with_db()
 	if(IsAdminAdvancedProcCall())
-		to_chat(usr, "<span class='admin prefix'>Admin rank DB Sync blocked: Advanced ProcCall detected.</span>")
+		to_chat(usr, span_adminprefix("Admin rank DB Sync blocked: Advanced ProcCall detected."))
 		return
 
 	if(CONFIG_GET(flag/admin_legacy_system) || !SSdbcore.IsConnected()) //we're already using legacy system so there's nothing to save
@@ -369,7 +394,7 @@ GLOBAL_PROTECT(protected_ranks)
 
 /proc/save_admin_backup()
 	if(IsAdminAdvancedProcCall())
-		to_chat(usr, "<span class='admin prefix'>Admin rank DB Sync blocked: Advanced ProcCall detected.</span>")
+		to_chat(usr, span_adminprefix("Admin rank DB Sync blocked: Advanced ProcCall detected."))
 		return
 
 	if(CONFIG_GET(flag/admin_legacy_system)) //we're already using legacy system so there's nothing to save

code/modules/admin/known_alts.dm

@@ -48,7 +48,7 @@ GLOBAL_DATUM_INIT(known_alts, /datum/known_alts, new)
 				return
 
 			var/already_exists_row = query_already_exists.NextRow()
-			qdel(query_already_exists)
+			QDEL_NULL(query_already_exists)
 
 			if (already_exists_row)
 				alert(usr, "Those two are already in the list of known alts!")
@@ -71,7 +71,7 @@ GLOBAL_DATUM_INIT(known_alts, /datum/known_alts, new)
 				cached_known_alts = null
 				load_known_alts()
 
-			qdel(query_add_known_alt)
+			QDEL_NULL(query_add_known_alt)
 			show_panel(usr.client)
 
 			if (!is_banned_from(ckey2, "Server"))
@@ -101,7 +101,7 @@ GLOBAL_DATUM_INIT(known_alts, /datum/known_alts, new)
 				return
 
 			var/list/result = query_known_alt_info.item
-			qdel(query_known_alt_info)
+			QDEL_NULL(query_known_alt_info)
 
 			if (alert("Are you sure you want to delete the alt connection between [result[1]] and [result[2]]?",,"Yes", "No") != "Yes")
 				return
@@ -121,7 +121,7 @@ GLOBAL_DATUM_INIT(known_alts, /datum/known_alts, new)
 				cached_known_alts = null
 				load_known_alts()
 
-			qdel(query_delete_known_alt)
+			QDEL_NULL(query_delete_known_alt)
 			show_panel(usr.client)
 
 /// Returns the list of known alts, will return an empty list if the DB could not be connected to.
@@ -152,8 +152,8 @@ GLOBAL_DATUM_INIT(known_alts, /datum/known_alts, new)
 			query_known_alts.item[1],
 		))
 
+	QDEL_NULL(query_known_alts)
 	COOLDOWN_START(src, cache_cooldown, 10 SECONDS)
-	qdel(query_known_alts)
 
 	return cached_known_alts
 

code/modules/admin/sql_ban_system.dm

@@ -200,7 +200,7 @@
 		if(is_admin && !text2num(query_build_ban_cache.item[2]))
 			continue
 		ban_cache[query_build_ban_cache.item[1]] = TRUE
-	qdel(query_build_ban_cache)
+	QDEL_NULL(query_build_ban_cache)
 	if(QDELETED(player_client)) // Disconnected while working with the DB.
 		return
 	player_client.ban_cache = ban_cache

code/modules/admin/stickyban.dm

@@ -124,7 +124,7 @@
 					list("ckey" = ckey, "alt" = alt)
 				)
 				query_remove_stickyban_alt.warn_execute()
-				qdel(query_remove_stickyban_alt)
+				QDEL_NULL(query_remove_stickyban_alt)
 
 			log_admin_private("[key_name(usr)] has disassociated [alt] from [ckey]'s sticky ban")
 			message_admins(span_adminnotice("[key_name_admin(usr)] has disassociated [alt] from [ckey]'s sticky ban"))
@@ -158,7 +158,7 @@
 					list("reason" = reason, "ckey" = ckey)
 				)
 				query_edit_stickyban.warn_execute()
-				qdel(query_edit_stickyban)
+				QDEL_NULL(query_edit_stickyban)
 
 			log_admin_private("[key_name(usr)] has edited [ckey]'s sticky ban reason from [oldreason] to [reason]")
 			message_admins(span_adminnotice("[key_name_admin(usr)] has edited [ckey]'s sticky ban reason from [oldreason] to [reason]"))
@@ -208,7 +208,7 @@
 					list("ckey" = ckey, "alt" = alt)
 				)
 				query_exempt_stickyban_alt.warn_execute()
-				qdel(query_exempt_stickyban_alt)
+				QDEL_NULL(query_exempt_stickyban_alt)
 
 			log_admin_private("[key_name(usr)] has exempted [alt] from [ckey]'s sticky ban")
 			message_admins(span_adminnotice("[key_name_admin(usr)] has exempted [alt] from [ckey]'s sticky ban"))
@@ -258,7 +258,7 @@
 					list("ckey" = ckey, "alt" = alt)
 				)
 				query_unexempt_stickyban_alt.warn_execute()
-				qdel(query_unexempt_stickyban_alt)
+				QDEL_NULL(query_unexempt_stickyban_alt)
 
 			log_admin_private("[key_name(usr)] has unexempted [alt] from [ckey]'s sticky ban")
 			message_admins(span_adminnotice("[key_name_admin(usr)] has unexempted [alt] from [ckey]'s sticky ban"))

code/modules/admin/topic.dm

@@ -58,19 +58,28 @@
 	// BUBBER EDIT END
 
 	else if(href_list["editrightsbrowser"])
-		edit_admin_permissions(0)
+		edit_admin_permissions(PERMISSIONS_PAGE_PERMISSIONS)
 
-	else if(href_list["editrightsbrowserlog"])
+	else if(href_list["editrightsbrowserranks"])
-		edit_admin_permissions(1, href_list["editrightstarget"], href_list["editrightsoperation"], href_list["editrightspage"])
+		if(href_list["editrightsaddrank"])
+			add_rank()
+		else if(href_list["editrightsremoverank"])
+			remove_rank(href_list["editrightsremoverank"])
+		else if(href_list["editrightseditrank"])
+			change_rank(href_list["editrightseditrank"])
+		edit_admin_permissions(PERMISSIONS_PAGE_RANKS)
 
-	if(href_list["editrightsbrowsermanage"])
+	else if(href_list["editrightsbrowserlogging"])
+		edit_admin_permissions(PERMISSIONS_PAGE_LOGGING, href_list["editrightslogtarget"], href_list["editrightslogactor"], href_list["editrightslogoperation"], href_list["editrightslogpage"])
+
+	if(href_list["editrightsbrowserhousekeep"])
 		if(href_list["editrightschange"])
 			change_admin_rank(ckey(href_list["editrightschange"]), href_list["editrightschange"], TRUE)
 		else if(href_list["editrightsremove"])
 			remove_admin(ckey(href_list["editrightsremove"]), href_list["editrightsremove"], TRUE)
 		else if(href_list["editrightsremoverank"])
 			remove_rank(href_list["editrightsremoverank"])
-		edit_admin_permissions(2)
+		edit_admin_permissions(PERMISSIONS_PAGE_HOUSEKEEPING)
 
 	else if(href_list["editrights"])
 		edit_rights_topic(href_list)

code/modules/client/client_procs.dm

@@ -123,6 +123,7 @@ GLOBAL_LIST_INIT(blacklisted_builds, list(
 			list("id" = message_id, "player_key" = usr.ckey)
 		)
 		query_message_read.warn_execute()
+		QDEL_NULL(query_message_read)
 		return
 
 	// TGUIless adminhelp
@@ -379,7 +380,10 @@ GLOBAL_LIST_INIT(blacklisted_builds, list(
 				new /datum/admins(autoadmin_ranks, ckey)
 
 	if(CONFIG_GET(flag/enable_localhost_rank) && !connecting_admin && is_localhost())
-		var/datum/admin_rank/localhost_rank = new("!localhost!", R_EVERYTHING, R_DBRANKS, R_EVERYTHING) //+EVERYTHING -DBRANKS *EVERYTHING
+		var/datum/admin_rank/localhost_rank = new("!localhost!", RANK_SOURCE_LOCAL, R_EVERYTHING, R_DBRANKS, R_EVERYTHING) //+EVERYTHING -DBRANKS *EVERYTHING
+		if(QDELETED(localhost_rank))
+			to_chat(world, "Local admin rank creation failed, somehow?")
+			return
 		new /datum/admins(list(localhost_rank), ckey, 1, 1)
 
 	if (length(GLOB.stickybanadminexemptions))

code/modules/tutorials/_tutorial.dm

@@ -242,8 +242,7 @@
 	)
 
 	insert_tutorial_query.warn_execute()
-
+	QDEL_NULL(insert_tutorial_query)
-	qdel(insert_tutorial_query)
 
 /// Dismisses the tutorial, not marking it as completed in the database.
 /// Call `/datum/tutorial/proc/dismiss()` instead.

config/admins.txt

@@ -5,6 +5,7 @@
 #If SQL-based admin loading is enabled, admins listed here will always be loaded first and will override any duplicate entries in the database.
 
 Optimumtact = Host
+LemonInTheDark = Host
 CitrusGender = Game Master
 NewSta = Game Master
 Expletives = Game Master