UnseenFaith / komada

Komada: Croatian for `pieces`, is a modular bot system including reloading modules and easy to use custom commands.
MIT License
62 stars 31 forks source link

Unable to use user IDs of users not in guild #198

Closed Lovinity closed 7 years ago

Lovinity commented 7 years ago

Version of Komada

0.18.1

Are core files modified?

No

Describe the problem

When trying to specify a user ID for a command using a user parameter, the user has to exist in the guild or Komada fails to pass a user object to the command.

Expected Behaviour

When using a command that takes a user parameter, if the user does not exist in the guild, Komada will find the user and still return a user object to the command.

Actual Behaviour

Komada returns a null user object to the command if the user does not exist in the guild.

Steps to Reproduce.

Komada used to return user objects to the command even if the user was not in the guild. But it suddenly stopped doing that after I ran a custom function that scanned a bunch of user IDs from a MariaDB database and updated the database to include their username and discriminator. I was utilising Discord.js 11.1 client.FetchUser to do this.

CyberiumShadow commented 7 years ago

Can you also provide us some screenshot evidence?

I'm going to look into the source code as well.

EDIT: Just had a look at usage. We only do client.users.get for the user usage. This means that your fetchUser won't apply until the command is successfully run. Having a look at indev and bleeding edge branches to see if we fix this.

EDIT2: If you are getting a null user object, it means user isn't in your cache which is what .fetchUser is supposed to rectify on successful run of said method.

Lovinity commented 7 years ago

Okay. This is a tough call because you guys use client.users.get for the user and mention args... which by default behaviour would return nothing if the user isn't cached. Yet somehow my bot was able to return a user object even if the user has never existed in any of the bot guilds before (which I've centered my commands around that expectation). Suddenly though, it's not.

https://github.com/Lovinity/lovinibot/blob/master/commands/Moderation/log.js

command executed: https://gyazo.com/d576c057cec9637c20294d371b1feeaf

The user ID provided as a parameter has never existed before in the guild; I blacklisted them before because they caused issues for another user in a different server.

Prior to the bug, it actually worked: https://gyazo.com/64b0203b03880d7e250da9b80f3b1e32

Now, it fails to work, and my debug code at the top of the log.js file spits this out in console:

log command executed in guild 260824811268210691, channel 306909833804775425. E: Cannot read property 'id' of null Object: null [debug] MySQL connection established. [debug] MySQL connection established. ---Working on log user 95654451179687936 ---i+1 = 1 ---Pushed log info for user 95654451179687936: ItsLachy#9764 (95654451179687936) ---Working on log user 293521898392911872 ---i+1 = 2 ---Pushed log info for user 293521898392911872: DG_x98#5038 (293521898392911872) ---Working on log user 292514510244151296 ---i+1 = 3 ---Pushed log info for user 292514510244151296: dragoon#0835 (292514510244151296) ---Working on log user 287811449227575296 ---i+1 = 4 ---Pushed log info for user 287811449227575296: alicia.chandler#7710 (287811449227575296) ---Working on log user 273639871858475009 ---i+1 = 5 ---Pushed log info for user 273639871858475009: lizardlair00#2100 (273639871858475009) ---Working on log user 263559266961588234 ---i+1 = 6 ---i+1 = 7 ---Pushed log info for user 263559266961588234: Lolithia#8179 (263559266961588234) ---Working on log user 254977513514729472 ---i+1 = 8 ---Pushed log info for user 254977513514729472: ????MadHatter????#5582 (254977513514729472) ---Working on log user 253251262093590528 ---i+1 = 9 ---Pushed log info for user 253251262093590528: Stephen Tourmaline#2619 (253251262093590528) ---Working on log user 230873376057458698 ---i+1 = 10 ---Pushed log info for user 230873376057458698: lunorian#7176 (230873376057458698) ---Working on log user 218116286520098826 ---i+1 = 11 ---Pushed log info for user 218116286520098826: Jacob#6596 (218116286520098826) ---Working on log user 206854643333070850 ---i+1 = 12 ---i+1 = 13 ---i+1 = 14 ---Pushed log info for user 206854643333070850: kitsunedriid10#2592 (206854643333070850) ---Working on log user 189097770160881664 ---i+1 = 15 ---Pushed log info for user 189097770160881664: Tangento#9240 (189097770160881664) ---Working on log user 183381929788309504 ---i+1 = 16 ---Pushed log info for user 183381929788309504: Winetta Scarlet#8245 (183381929788309504) ---Working on log user 181096104174026752 ---i+1 = 17 ---Pushed log info for user 181096104174026752: Silverwing#5419 (181096104174026752) ---Working on log user 173269868076859392 ---i+1 = 18 ---i+1 = 19 ---Pushed log info for user 173269868076859392: callum_the_pen_cil#9685 (173269868076859392) ---Working on log user 149632488581496833 ---i+1 = 20 ---Pushed log info for user 149632488581496833: Na11#7922 (149632488581496833) ---message: Moderator Logs:,ItsLachy#9764 (95654451179687936) : 0 warnings, 0 mutes, and 1 bans on record.,​,DG_x98#5038 (293521898392911872) : 1 warnings, 0 mutes, and 0 bans on record.,​,dragoon#0835 (292514510244151296) : 1 warnings, 0 mutes, and 0 bans on record.,​,alicia.chandler#7710 (287811449227575296) : 0 warnings, 0 mutes, and 1 bans on record.,​,lizardlair00#2100 (273639871858475009) : 0 warnings, 0 mutes, and 1 bans on record.,​,Lolithia#8179 (263559266961588234) : 1 warnings, 0 mutes, and 1 bans on record.,​,????MadHatter????#5582 (254977513514729472) : 0 warnings, 0 mutes, and 1 bans on record.,​,Stephen Tourmaline#2619 (253251262093590528) : 0 warnings, 0 mutes, and 1 bans on record.,​,lunorian#7176 (230873376057458698) : 0 warnings, 0 mutes, and 1 bans on record.,​,Jacob#6596 (218116286520098826) : 0 warnings, 0 mutes, and 1 bans on record.,​,kitsunedriid10#2592 (206854643333070850) : 2 warnings, 1 mutes, and 0 bans on record.,​,Tangento#9240 (189097770160881664) : 0 warnings, 1 mutes, and 0 bans on record.,​,Winetta Scarlet#8245 (183381929788309504) : 0 warnings, 0 mutes, and 1 bans on record.,​,Silverwing#5419 (181096104174026752) : 1 warnings, 0 mutes, and 0 bans on record.,​,callum_the_pen_cil#9685 (173269868076859392) : 1 warnings, 0 mutes, and 1 bans on record.,​,Na11#7922 (149632488581496833) : 1 warnings, 0 mutes, and 0 bans on record.,​

CyberiumShadow commented 7 years ago

Hmm, I'm curious though. Is/was the bot in any other servers apart from your server?

Lovinity commented 7 years ago

Yes. The bot is in a few other servers. But the user matching the ID I used for the log command has never been in any of them.

CyberiumShadow commented 7 years ago

I'll be honest, the only plausible explanation is that the user happened to be in mutual server to the bot, in order to exist within the cache.

Right now we don't have any defensive code in usage to fetchUser/fetchMember(s) but I think we can add that relatively easily, however I believe Discord.JS v12 might have autoFetch.

As a stop gap measure, i suggest making a inhibitor that fetchs the user/member so that when usage is run after the inhibitors, it should have the user in cache by then.

Lovinity commented 7 years ago

Good idea. I feared I'd have to modify original code to do fetchuser. But I had forgotten about the inhibitors.

CyberiumShadow commented 7 years ago

To be fair, its not an ideal measure, since inhibitors are supposed to block a command from running, but due to the current execution of the commands, its the only part of Komada that can do this so far.

Lovinity commented 7 years ago

True. I'm looking at the inhibitors. I don't think they'll work because it does not pass arguments to inhibitors, just the message and the command.

Lovinity commented 7 years ago

I'm going to do an overwrite of the usage function until the fetchUser is pushed in an official release.

Lovinity commented 7 years ago

Wellll that won't work either. fetchUser is a Promise, which means usage will return before fetchUser is done. Have not yet learned how to force Javascript to wait for a promise.

UnseenFaith commented 7 years ago

You could just block the command if the user isn't found, then fetch it in the background. We'll add the ability to fetch any User and fetchMembers from another guild in the future

CyberiumShadow commented 7 years ago

Most likely the fix for this will appear in 0.20.0 as 0.19.0 has been staged for release for just over a week now.

Lovinity commented 7 years ago

I found a dirty hack solution by adding a usage.js override:

    case "user":
    case "mention":
      if (/^<@!?\d+>$/.test(args[i]) || args[i].length > 5) {
        if (!client.users.has(/\d+/.exec(args[i])[0])) 
        {
        client.fetchUser(/\d+/.exec(args[i])[0])
        .then((theuser) => {
            args[i] = theuser;
            validateArgs(++i);
        })
        .catch((e) => {
    if (currentUsage.type === "optional" && !repeat) {
        args.splice(i, 0, undefined);
        validateArgs(++i);
      } else {
        args.shift();
        return reject(client.funcs.newError(`${currentUsage.possibles[0].name} must be a mention or valid user id.`, 1, args));
      }
        });
        } else {
        args[i] = client.users.get(/\d+/.exec(args[i])[0]);
        validateArgs(++i);
        }
      } else if (currentUsage.type === "optional" && !repeat) {
        args.splice(i, 0, undefined);
        validateArgs(++i);
      } else {
        args.shift();
        return reject(client.funcs.newError(`${currentUsage.possibles[0].name} must be a mention or valid user id.`, 1, args));
      }
      break;
CyberiumShadow commented 7 years ago

That code will not work for selfbots as <Client>.fetchUser and <Guild>.fetchMember(s) are available to OAuth bots only. Additionally, there are multiple cases for users within usage that would have to be updated as well.

UnseenFaith commented 7 years ago

Only half of what @CyberiumShadow said was true. fetchUser is the only method not available to selfbots. fetchMember(s) is available to everyone.

Lovinity commented 7 years ago

Why is fetchUser not available to self bots?

kyranet commented 7 years ago

Ask DiscordAPI. Docs: https://discord.js.org/#/docs/main/master/class/Client?scrollTo=fetchUser

Lovinity commented 7 years ago

That's very odd to me. That means self bots cannot look up users that are not in a guild.

On Sat, Apr 29, 2017 at 2:52 PM, kyraNET notifications@github.com wrote:

Ask DiscordAPI. Docs: https://discord.js.org/#/docs/main/master/class/Client? scrollTo=fetchUser

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dirigeants/komada/issues/198#issuecomment-298187500, or mute the thread https://github.com/notifications/unsubscribe-auth/AK8KYh1r9GPG-GjUrFoGUg1Co-ta04Zeks5r04cEgaJpZM4NK4SJ .

-- Signed, Patrick Schmalstig CEO/Founder of The Lovinity Community, https://lovinity.org Web Developer for ocProducts Ltd. http://www.ocproducts.com

CyberiumShadow commented 7 years ago

Well its not supported by the Discord API given that they don't actually support selfbots.

Lovinity commented 7 years ago

Ah true I remember now, they removed that. I thought Discord made it a TOS violation to use self bots too? I could be wrong.

CyberiumShadow commented 7 years ago

It's a grey area

kyranet commented 7 years ago

This issue gets fixed on #237.

CyberiumShadow commented 7 years ago

As of now, all efforts are now primarily on Indev.

The final update to 0.19.x will be the backport-staging branch