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

Allow blocking by alias #238

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Allow blocking by alias #238

wants to merge 3 commits into from

Conversation

Half-Shot
Copy link
Collaborator

Because there are rooms with 10s of thousands of users which are breaking things.

@Half-Shot Half-Shot requested a review from a team March 1, 2021 18:39
@Half-Shot Half-Shot force-pushed the hs/block-by-regex branch from 13a42ab to 117dc2d Compare March 1, 2021 18:44
Copy link
Contributor

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

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

Converting the regex strings at startup may prevent run time errors, which I think is worth the small effort.

* Room ID or alias
* Room ID or alias. Optional.
*
* Either `room` or `roomRegex` must be provided.
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered splitting this into three types?
type IConfigRoomRule = IConfigRoomRuleWithRoom | IConfigRoomRuleWithRoomRegex;

There would need to be one validation when loading the config, but after that it reduces the complexity of conditions.

public getRoomRule(roomIdOrAlias?: string) {
const aliasRule = this.roomRules.find((r) => r.room === roomIdOrAlias);
if (aliasRule && aliasRule.action === "deny") {
public getRoomRule(roomIdOrAlias: string) {
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 "first match wins" algorithm. I don't think this is expected by users when supporting regexps.

Without an annotation in the config sample (and the docs) I would expect exceptions like these to work:

/discord/ deny
"discord-jaller" allow

I think that would be .findLast() (<-- not a JS function). But we can also have the annotation saying that the first match wins.

if (r.room === roomIdOrAlias) {
return true;
}
return (r.roomRegex && roomIdOrAlias.match(r.roomRegex));
Copy link
Contributor

Choose a reason for hiding this comment

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

We have no interest in the return value. .test() may be faster and is more intentional.

Suggested change
return (r.roomRegex && roomIdOrAlias.match(r.roomRegex));
return (r.roomRegex && r.roomRegex.test(new RegExp(roomIdOrAlias)));

*/
room: string;
roomRegex?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend converting the strings to RegExp here.
A bit for performance reasons, but mainly to catch config errors (alias invalid RegExp strings) at startup.

new RegExp(string)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants