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

Add unlogtext command #2645

Closed
wants to merge 1 commit into from
Closed

Conversation

team1257Simon
Copy link

@team1257Simon team1257Simon commented Jul 26, 2016

The unlogtext command clears a banned user's messages from the room such that regular users can no longer see them (staff still can), which is useful if they leaked something sensitive/private. This was partially suggested in the issues file under "Fixes to text hiding." #2444

commands.js Outdated
@@ -1891,7 +1891,28 @@ exports.commands = {
if (userid !== toId(this.inputUsername)) this.add('|unlink|' + hidetype + toId(this.inputUsername));
},
hidetexthelp: ["/hidetext [username] - Removes a locked or banned user's messages from chat (includes users banned from the room). Requires: % (global only), @ * # & ~"],

Copy link
Contributor

Choose a reason for hiding this comment

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

don't remove this empty line

@panpawn
Copy link
Contributor

panpawn commented Jul 27, 2016

Did you test this command? Because it doesn't work...

commands.js Outdated
}
if (!targetUser) return this.errorReply("User '" + name + "' not found.");
if (!(targetUser.locked || (room.bannedUsers[toId(name)] && room.bannedIps[targetUser.latestIp]) || user.can('rangeban'))) return this.errorReply("User '" + name + "' is not banned from this room or locked.");
let roomlog = room.log.split(",");
Copy link
Contributor

@panpawn panpawn Jul 27, 2016

Choose a reason for hiding this comment

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

CRASH: TypeError: room.log.split is not a function

@panpawn
Copy link
Contributor

panpawn commented Jul 27, 2016

If this command gets added, it definitely shouldn't be given to global drivers...

Also, it doesn't modlog that someone cleared someone's messages, which is a huge security risk.

commands.js Outdated
},
unlogtexthelp: ["/unlogtext [username] - Removes a locked or banned user's messages from chat and clears them from the room's log. Requires: % (global only), @ * # & ~"],

Copy link
Contributor

Choose a reason for hiding this comment

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

there's trailing whitespace here

@team1257Simon
Copy link
Author

team1257Simon commented Jul 27, 2016

I added a modlog thing...what users should have access to it then?
Sorry about all the commits, I'm new to these coding standards...

@panpawn
Copy link
Contributor

panpawn commented Jul 27, 2016

I would say leaders and administrators probably

@panpawn
Copy link
Contributor

panpawn commented Jul 27, 2016

Also, you can probably squash these commits

@team1257Simon
Copy link
Author

Squashed

commands.js Outdated
this.splitTarget(target);
let targetUser = this.targetUser;
let name = this.targetUserName;
if (!user.can("rangeban")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not overload permissions. Add a new separate one

Copy link
Author

Choose a reason for hiding this comment

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

I based the permissions checking and other setup checks on the hidetext command, which uses user.can("roomban"). If I need to make a new permission I can but I think the two actions are related closely enough that sharing the permission should suffice.

@team1257Simon
Copy link
Author

I tested it and it worked...
As for the alts thing, with my understanding of how alts work it shouldn't matter which one I'm checking against.
As for the permissions, I didn't want to add a new permission because that would mean editing the config file to which I do not have access. The second check is to make sure the target user is banned, not to check permissions.
With the log index, I will change that, it's a remnant of when I tried a for ... in loop.

@kotarou3
Copy link
Contributor

  1. Reply to each individual line comment to keep them organised
  2. It matters in a case of user lookup count. You lookup the users once and cache that result, instead of doing it for every single log line
  3. You can edit config-example.js
  4. You have a || user.can('rangeban') at the end though

@Zarel
Copy link
Member

Zarel commented Jul 27, 2016

The way I think I originally thought of this feature was as a side effect of roomban and lock. The text is still visible in the log files, but wouldn't appear to users joining the room in the future.

One of the consequences is that it would be harder for trolls/spammers to really know if their attacks successfully went through. Maybe an /evader command so it's only done for very obvious evading spammers.

@team1257Simon
Copy link
Author

I'm actually fairly certain it won't delete the text from the log files, just the log that future users see. So basically, exactly your idea.

@kotarou3
Copy link
Contributor

kotarou3 commented Jul 28, 2016

Might want to double check how and when the logs are written (I don't know either)

@Zarel
Copy link
Member

Zarel commented Jul 28, 2016

Oh, I know your implementation doesn't.

@team1257Simon
Copy link
Author

@kotarou3 I checked, it logs to text every time a message goes through.
@Zarel if you want me to not have this be its own command (aka integrated into ban and lock) let me know, I will rename the PR

commands.js Outdated
let message = room.log[i];
let split = message.split("|");
if (Users.get(split[3]) === this.targetUser || Users.get(split[3]).getAltUsers().indexOf(this.targetUser) >= 0) {
room.log.splice(i, 1);
Copy link
Contributor

@kotarou3 kotarou3 Jul 28, 2016

Choose a reason for hiding this comment

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

You still end up skipping a log entry for each one you remove. Think carefully how room.log changes in respect to i when you splice(i, 1) it, and what happens in the next loop iteration

Copy link
Author

Choose a reason for hiding this comment

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

Oh, yeah i see what you're talking about. I'll fix that

@kotarou3
Copy link
Contributor

You still haven't properly addressed my other three comments

commands.js Outdated
if (!targetUser) return this.errorReply("User '" + name + "' not found.");
if (!(targetUser.locked || (room.bannedUsers[toId(name)] && room.bannedIps[targetUser.latestIp]) || user.can('rangeban'))) return this.errorReply("User '" + name + "' is not banned from this room or locked.");
let i = 0;
while(i < room.log.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Travis is complaining about the missing space between while and the paren

@Zarel
Copy link
Member

Zarel commented Jul 30, 2016

tbh my main issue is that I don't know exactly how this should be implemented, and there doesn't seem to be much consensus, either.

@team1257Simon
Copy link
Author

Sorry for not responding, I've been away. I'll get back to this soon...

team1257Simon pushed a commit to team1257Simon/Pokemon-Showdown-Client that referenced this pull request Aug 2, 2016
This is in tandem with smogon/pokemon-showdown#2645, and makes regular users unable to unhide text from banned users. Staff (moderator and up) retain this ability.
The unlogtext command clears a banned user's messages from the chat logs, which is useful if they leaked something sensitive/private. This was partially suggested in the issues file under "Fixes to text hiding."
let targetUser = this.targetUser;
let name = this.targetUserName;
let userid = targetUser.getLastId();
if (!user.can("rangeban")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not !this.can("rangeban")?

@panpawn
Copy link
Contributor

panpawn commented Jul 12, 2017

This PR seems to be abandoned, and I think I know how I would go about this if it'd be okay if I were to take over:

  • Command name is /evader [user]
  • Removes user's text in the room and all of their previous names in room.log
  • Sends |unlink|perma|userid for the user's name and each of the user's previous names to the room

Also, what permission should this actually bet set to? I know I had said driver was probably a bad idea, but maybe just handle it like hidetext and allow drivers to if the user has been locked or roombanned.

Thoughts on this proposal?

@Zarel
Copy link
Member

Zarel commented Jul 12, 2017

This is blocked by a database refactor; I'd suggest waiting until then.

@panpawn
Copy link
Contributor

panpawn commented Jul 12, 2017

Okay then

@Zarel
Copy link
Member

Zarel commented Dec 24, 2017

I'm just going to close this. I appreciate the effort here, but what I was looking for in Ideas for New Developers was an unlogging function to supplement existing |unlink|hide| use-cases, not an unlogging command.

@Zarel Zarel closed this Dec 24, 2017
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.

6 participants