Bugzilla – Bug 4570
Changing admin processing proposal
Last modified: 2013-02-13 09:19:35 PST
Created attachment 2297 [details] [review] Proposed basic patch Hello, I want to propose patch for "Admin Base" plugin, that change admins processing procedure. Current implementation checks admin only till first match, so it depends on lines order in .ini file or sql base, what in some cases lead to security problems. For example, we have such users.ini: "STEAM_0:0:51211529" "" "abcefipu" "ce";Admin1 "K1lleR[IPT]" "s1f3ad""abcefiup" "a"; Admin2 With current authentication algorithm, Admin1 can easily use password protected nick from Admin2, because plugin stops on first string, authenticate Admin1 as admin, and stops check. Also, if we have user with multiply authentication tokens, for example clanteg with basic rights assignment, and SteamID with extended rights, current algorithm assigns rights only from first match and do not multiply them. So, we must specify SteamID higher then clanteg, but then we back to problem above :). Yes, we can try to solve both problems with right strings order, but in that case users.ini file (or DB table) will look like a mess. Proposed patch solves all this problems, new algorithm finds all authentication tokens and checks them, user gets only supposed rights and get kick (if set) for unsuccessful authentication(s). New problem: if we have more than one authentication tokens with specified passwords, in general case we stuck with new plugin because we have only one password field. In particular case, I've solved this problem for password protected nick and clanteg pair: it is obvious that if user successfully authenticated with nick, than there is no need to check clantag password. For other cases from my point of view, best solution is to use _pw-ip and _pw-id setinfo fields, which need to be specified only in such multitokens situation. That is not implemented yet and is object to discuss
Comment on attachment 2297 [details] [review] Proposed basic patch The idea seems reasonable, but the patch is pretty hard to review. Ideally some of the highly indented logic could be split out and factored as helper functions, to avoid having too much complexity in one place.
Created attachment 3235 [details] Current function state Well, that was a long ago :) I really don't see what can be simplified here, but I'm not a developer. Anyway, I'm attaching how that function looks like at my local server's build, it works flawlessly since this bug creation. One addition, compared to old patch, is new PRIO flag, it allows to turn "old" behavior for some specified accounts (for hltv in my case). Separate setinfos for ip/steamid passwords are not implemented, but this wouldn't be hard to add, I think I can do it, but only if patch may be accepted without major rewrite :)