Automated whitelists #23985

Simyon264 · 2024-01-12T20:52:53Z

About the PR

This PR adds automatic whitelists based on conditions defined in a prototype.

Why / Balance

Resolves #19606.

Technical details

This PR adds a blacklist table to the DB and the coresponding commands for managing the blacklist.
I also added a new prototype PlayerConnectionWhitelist which is used to define whitelists. That prototype contains an Array of WhitelistCondition conditions. Every WhitelistCondition contains 2 DataFields and a Function to check if the player is allowed to connect. The 2 DataFields are the following:
BreakIfConditionSuccess: If the condition passes, the next conditions will not be checked and the player will be able to connect.
BreakIfConditionFail: If the condition fails, the player will be denied and the next conditions will not be checked.

Media

I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

This removes some CVars related for whitelists, the legacy whitelists are used by default when whitelist.enabled is set to true. Use whitelist.prototypeList to define which whitelists should be used. Currently implemented whitelists are: mrpWhitelist and basicWhitelist.

Only people that are added to the whitelist with the addwhitelist command will be able to join. I call this the "legacy" whitelist

…eck fails

Default whitelist is now the "legacy" whitelist.

Chief-Engineer

Overall, it looks like it would work for WizDen's needs, but changes could be made to improve readability of the prototype, and to make the system more flexible, potentially allowing it to be easier to use for other things in the future.

Adding support for playtime based ranges for the note condition is probably the thing that'd have the biggest impact on the effectiveness of the system for WizDen

Chief-Engineer · 2024-01-14T05:25:34Z

Content.Server/Connection/Whitelist/WhitelistPrototype.cs

This is a terminology nitpick, but to keep things flexible, I'd avoid using the term whitelist/blacklist. It's all just conditions or rules that contribute to an overall blacklist or whitelist system, using terminology that doesn't describe any of the conditions as a whitelist or blacklist can encourage people who make changes in the future to keep the design flexible.

It's not the "Minimum number of players required for this whitelist to be active", it's the "Minimum number of players required for this condition to match", because if the condition can be used flexibly enough, then it can be used as a whitelist or a blacklist

Content.Server/Connection/Whitelist/WhitelistCondition.cs

Content.Server/Connection/Whitelist/Conditions/ConditionNotes.cs

Content.Server.Database/Model.cs

Content.Server/Connection/Whitelist/Conditions/ConditionNotes.cs

metalgearsloth · 2024-02-01T11:33:30Z

@Simyon264 you working on this famalams.

Simyon264 · 2024-02-01T18:26:52Z

Yes. I am still working on it.

github-actions · 2024-02-20T09:16:08Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Content.Server/Connection/ConnectionManager.cs

Chief-Engineer

Sorry for not catching this earlier. It seems like there are some quirks with the manual whitelist/blacklist conditions.

It's very possible that I'm missing something, partly because I'm just looking at the changes on GitHub, but it looks like the manual whitelist/blacklist conditions have opposite behavior based on someone's status in the list. I understand why this would have been done, but I think both of these should act as a "is the person on the list" check instead for two reasons:

A long term goal would be to allow arbitrary lists to be made so that servers can create as many manual lists as they want for whatever purpose they have, potentially even using those lists for other things unrelated to connections. Having conditions work as being list membership now will prevent having to make changes in the future that either break YAML configs or require workarounds.
I think it makes the rules clearer in the YAML. ConditionManualBlacklist allow seems weird. It might also help to append Membership to the conditions, but that'd increase the name length quite a bit.

Resources/Prototypes/whitelists.yml

Resources/Prototypes/wizardsDenWhitelists.yml

Simyon264 · 2024-07-06T10:24:20Z

A long term goal would be to allow arbitrary lists to be made so that servers can create as many manual lists as they want for whatever purpose they have, potentially even using those lists for other things unrelated to connections. Having conditions work as being list membership now will prevent having to make changes in the future that either break YAML configs or require workarounds.

Something like this? @Chief-Engineer

  - !type:ConditionMembership
    list: whitelist # Which list the player needs to be a part of
    requiredMembershipTime: 120 # Must be a member for 2 hours
    action: Allow
  - !type:ConditionAlwaysMatch
    action: Deny # Not a member, deny connection

Chief-Engineer · 2024-07-08T06:38:22Z

A long term goal would be to allow arbitrary lists to be made so that servers can create as many manual lists as they want for whatever purpose they have, potentially even using those lists for other things unrelated to connections. Having conditions work as being list membership now will prevent having to make changes in the future that either break YAML configs or require workarounds.

Something like this? @Chief-Engineer
  - !type:ConditionMembership
    list: whitelist # Which list the player needs to be a part of
    requiredMembershipTime: 120 # Must be a member for 2 hours
    action: Allow
  - !type:ConditionAlwaysMatch
    action: Deny # Not a member, deny connection

Yes, except I'm not sure that requiredMembershipTime would be too useful unless the lists were automatically generated

github-actions · 2024-07-11T01:10:38Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2024-08-09T06:23:42Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

PJB3005

I'll fix these issues myself real quick.

PJB3005 · 2024-08-27T15:19:39Z

Content.Server/Connection/ConnectionManager.Whitelist.cs

+        Dictionary<NetUserId, List<IAdminRemarksRecord>> cacheRemarks = new();
+        Dictionary<NetUserId, List<PlayTime>> cachePlaytime = new();


Why are you storing this keyed on user ID if this function only runs for a single user at once?

PJB3005 · 2024-08-27T15:22:16Z

Content.Server/Connection/ConnectionManager.Whitelist.cs

+        return await _db.GetBlacklistStatusAsync(data.UserId);
+    }
+
+    private async Task<bool> CheckConditionNotesDateRange(ConditionNotesDateRange conditionNotes, List<IAdminRemarksRecord> remarks)


These don't need to be async

PJB3005 · 2024-08-27T15:22:55Z

Content.Server/Connection/ConnectionManager.cs

+            if (bans.Count > 0)
+            {
+                var firstBan = bans[0];
+                var message = firstBan.FormatBanMessage(_cfg, _loc);
+                return (ConnectionDenyReason.Ban, message, bans);
+            }


Screwed up merge xd

Simyon264 added 17 commits January 12, 2024 15:27

Beginnings of making the breadmemes jobs easier

1f81797

stuff

81cc771

stuff pt. 2

c6c83ee

Merge remote-tracking branch 'upstream/master' into better-whitelists

e09cb3b

Stuff pt.3

2421f07

Stuff I forgot last time

15cc74e

Basic whitelist

a538164

Only people that are added to the whitelist with the addwhitelist command will be able to join. I call this the "legacy" whitelist

Remove always deny condition in favor of just breaking if playtime ch…

65ab320

…eck fails

Change default whitelist

cb7ab30

Default whitelist is now the "legacy" whitelist.

localization

21c7285

Admin check

3e259e3

minor spelling change

e0c38e3

Fix build

2c5ee46

Whitelist message

ffd6ab9

Fix vars not being datafield and spelling mistakes

36b5c7a

Minor spelling mistake

36b039e

Change config for salamander

e2ab192

Simyon264 marked this pull request as ready for review January 13, 2024 12:39

Simyon264 requested review from PJB3005, DrSmugleaf and Chief-Engineer as code owners January 13, 2024 12:39

github-actions bot added the Status: Needs Review This PR requires new reviews before it can be merged. label Jan 13, 2024

Simyon264 changed the title ~~Better whitelists~~ Automated whitelists Jan 13, 2024

Chief-Engineer reviewed Jan 14, 2024

View reviewed changes

metalgearsloth added Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. and removed Status: Needs Review This PR requires new reviews before it can be merged. labels Feb 1, 2024

github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Feb 20, 2024

PJB3005 requested changes Jun 7, 2024

View reviewed changes

metalgearsloth added Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. and removed Status: Needs Review This PR requires new reviews before it can be merged. labels Jun 19, 2024

Merge branch 'master' into better-whitelists

297914c

github-actions bot removed the Merge Conflict This PR currently has conflicts that need to be addressed. label Jun 26, 2024

Simyon264 added 2 commits July 5, 2024 13:13

Reviews

a1345c9

whoops

c94c03f

Simyon264 requested a review from PJB3005 July 5, 2024 11:25

github-actions bot added Status: Needs Review This PR requires new reviews before it can be merged. and removed Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. labels Jul 5, 2024

Chief-Engineer reviewed Jul 6, 2024

View reviewed changes

Resources/Prototypes/whitelists.yml Outdated Show resolved Hide resolved

Resources/Prototypes/wizardsDenWhitelists.yml Outdated Show resolved Hide resolved

Resources/Prototypes/wizardsDenWhitelists.yml Outdated Show resolved Hide resolved

Simyon264 added 3 commits July 6, 2024 12:15

Explain a bit more how whitelist checking works

b2f6e58

Apply CE's review

25b8cd4

Append Membership to Blacklist and Whitelist conditions

2907daa

github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Jul 11, 2024

github-actions bot removed the Merge Conflict This PR currently has conflicts that need to be addressed. label Jul 11, 2024

github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Aug 9, 2024

PJB3005 reviewed Aug 27, 2024

View reviewed changes

Merge branch 'master' into better-whitelists

3cff3a3

github-actions bot removed the Merge Conflict This PR currently has conflicts that need to be addressed. label Aug 27, 2024

Fix review comments

b0211d7

PJB3005 approved these changes Aug 27, 2024

View reviewed changes

PJB3005 and others added 3 commits August 27, 2024 17:34

Uncapitalize playerConnectionWhitelist, add to ignored client prototypes

3bfdcd7

Make note count field work

af1526f

Fix cvar for thingy

46e0d13

PJB3005 merged commit f92ef41 into space-wizards:master Aug 27, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automated whitelists #23985

Automated whitelists #23985

Simyon264 commented Jan 12, 2024 •

edited

Loading

Chief-Engineer left a comment

Chief-Engineer Jan 14, 2024

metalgearsloth commented Feb 1, 2024

Simyon264 commented Feb 1, 2024

github-actions bot commented Feb 20, 2024

Chief-Engineer left a comment

Simyon264 commented Jul 6, 2024

Chief-Engineer commented Jul 8, 2024

github-actions bot commented Jul 11, 2024

github-actions bot commented Aug 9, 2024

PJB3005 left a comment

PJB3005 Aug 27, 2024

PJB3005 Aug 27, 2024

PJB3005 Aug 27, 2024

		Dictionary<NetUserId, List<IAdminRemarksRecord>> cacheRemarks = new();
		Dictionary<NetUserId, List<PlayTime>> cachePlaytime = new();

Automated whitelists #23985

Automated whitelists #23985

Conversation

Simyon264 commented Jan 12, 2024 • edited Loading

About the PR

Why / Balance

Technical details

Media

Breaking changes

Chief-Engineer left a comment

Choose a reason for hiding this comment

Chief-Engineer Jan 14, 2024

Choose a reason for hiding this comment

metalgearsloth commented Feb 1, 2024

Simyon264 commented Feb 1, 2024

github-actions bot commented Feb 20, 2024

Chief-Engineer left a comment

Choose a reason for hiding this comment

Simyon264 commented Jul 6, 2024

Chief-Engineer commented Jul 8, 2024

github-actions bot commented Jul 11, 2024

github-actions bot commented Aug 9, 2024

PJB3005 left a comment

Choose a reason for hiding this comment

PJB3005 Aug 27, 2024

Choose a reason for hiding this comment

PJB3005 Aug 27, 2024

Choose a reason for hiding this comment

PJB3005 Aug 27, 2024

Choose a reason for hiding this comment

Simyon264 commented Jan 12, 2024 •

edited

Loading