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.

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

Automated whitelists #23985

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

Simyon264
Copy link
Contributor

@Simyon264 Simyon264 commented Jan 12, 2024

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

image
image
image

  • 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.

@Simyon264 Simyon264 marked this pull request as ready for review January 13, 2024 12:39
@github-actions github-actions bot added the Status: Needs Review This PR requires new reviews before it can be merged. label Jan 13, 2024
@Simyon264 Simyon264 changed the title Better whitelists Automated whitelists Jan 13, 2024
Copy link
Contributor

@Chief-Engineer Chief-Engineer left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.Database/Model.cs Show resolved Hide resolved
@metalgearsloth
Copy link
Contributor

@Simyon264 you working on this famalams.

@metalgearsloth 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
@Simyon264
Copy link
Contributor Author

Yes. I am still working on it.

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

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

@Simyon264
Copy link
Contributor Author

FYI, I am still working on it.

Content.Server.Database/Model.cs Show resolved Hide resolved
Resources/Prototypes/whitelists.yml Show resolved Hide resolved
Content.Server/Connection/ConnectionManager.cs Outdated Show resolved Hide resolved
Content.Server/Connection/Whitelist/WhitelistCondition.cs Outdated Show resolved Hide resolved
Content.Server/Connection/Whitelist/WhitelistPrototype.cs Outdated Show resolved Hide resolved
Resources/Prototypes/whitelists.yml Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Merge Conflict This PR currently has conflicts that need to be addressed. label Mar 18, 2024
@Simyon264 Simyon264 marked this pull request as draft March 18, 2024 23:40
@Simyon264 Simyon264 marked this pull request as ready for review March 24, 2024 00:22
@Simyon264 Simyon264 marked this pull request as draft March 24, 2024 02:51
@Simyon264 Simyon264 marked this pull request as ready for review March 26, 2024 00:51
@Simyon264 Simyon264 requested a review from PJB3005 March 26, 2024 00:52
@github-actions 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 Mar 26, 2024
Copy link
Contributor

github-actions bot commented Apr 9, 2024

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

@github-actions github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge Conflict This PR currently has conflicts that need to be addressed. Status: Needs Review This PR requires new reviews before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automated whitelists
4 participants