Bug 4570 - Changing admin processing proposal
Changing admin processing proposal
Status: UNCONFIRMED
Product: AMX Mod X
Classification: Unclassified
Component: Base Plugins
trunk
PC All
: P3 normal
Assigned To: amxmodx-bugs@alliedmods.net
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-16 08:23 PDT by px
Modified: 2013-02-13 09:19 PST (History)
1 user (show)

See Also:


Attachments
Proposed basic patch (4.87 KB, patch)
2010-08-16 08:23 PDT, px
no flags Details | Diff | Splinter Review
Current function state (4.13 KB, text/plain)
2013-02-13 09:19 PST, px
no flags Details

Description px 2010-08-16 08:23:39 PDT
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 1 David Anderson [:dvander] 2013-02-12 23:26:44 PST
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.
Comment 2 px 2013-02-13 09:19:35 PST
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 :)

Note You need to log in before you can comment on or make changes to this bug.