Skip to content
This repository has been archived by the owner on Dec 13, 2021. It is now read-only.

Add Bot #82

Open
wants to merge 33 commits into
base: node
Choose a base branch
from
Open

Add Bot #82

wants to merge 33 commits into from

Conversation

A-Trash-Coder
Copy link
Contributor

This PR adds the ability to add a bot from the BotBlock site, to participating lists. Detailed list of things added:

  • Place on sidebar to add bot
  • Page for inputting bot ID (Snowflake and user validation)
  • Detailed form for bot information
    - Allows to select which lists to submit to
  • Status page to show if there was an error and the message returned by the bot list
  • Fields for editing a lists Add Bot url and key

If there is any suggestions please let me know. My code may not be the most efficient so please recommend changes. To my knowledge I followed the specifications stated in the #spec channel of the Add Bot category

@Andre601
Copy link

Nice thing.

Perhaps to help simplify things for bot devs could the bot invite field have an option to either select the required perms or input the raw perm number and the site would auto-generate a valid oauth URL for the bot to use.

Next should you perhaps include +applications.commands into the scope so that bots with Slash-commands work normally. Perhaps even have a toggle to choose if the dev wants that?

@A-Trash-Coder
Copy link
Contributor Author

I appreciate the feedback and suggestions. I now plan on adding a toggle for applications.commands toggle for bots that use slash commands. Also, I do like the mentioned auto generated OAuth url and will look into implementing that.

@A-Trash-Coder
Copy link
Contributor Author

Something like this @Andre601?

image

@Andre601
Copy link

Andre601 commented Jun 9, 2021

Something like this @Andre601?

image

I guess so, yeah. Can't honestly recall what I was talking about here...

@A-Trash-Coder
Copy link
Contributor Author

Any update on when this can be reviewed? This could be a major breakthrough for BotBlock and users.

@cheesycod
Copy link
Contributor

cheesycod commented Jun 20, 2021

Regarding this, would it be possible to use POST api/users/{user_id}/bots/{bot_id} as a endpoint for add bot as this is also what fates list officially uses. It is also similar to what discord does too…

also, would it be possible to customise how access token is sent, say as a header instead of in request body

@Andre601
Copy link

Regarding this, would it be possible to use POST api/users/{user_id}/bots/{bot_id} as a endpoint for add bot as this is also what fates list officially uses. It is also similar to what discord does too…

also, would it be possible to customise how access token is sent, say as a header instead of in request body

Kinda pointless imo. The bot ID is unique so there is never a real issue with a possible duplicate. Also, not everything has to follow a specific format.

@cheesycod
Copy link
Contributor

Regarding this, would it be possible to use POST api/users/{user_id}/bots/{bot_id} as a endpoint for add bot as this is also what fates list officially uses. It is also similar to what discord does too…
also, would it be possible to customise how access token is sent, say as a header instead of in request body

Kinda pointless imo. The bot ID is unique so there is never a real issue with a possible duplicate. Also, not everything has to follow a specific format.

The official API to add bots which the fates list site uses (and is stable now) is /api/users/{user_id}/bots/{bot_id} (once I deploy the latest update to it, that is). Using this format makes it far easier to use stuff like fastapi dep injection so it would be nice if botblock could support this sort of customisation somehow

@cheesycod
Copy link
Contributor

cheesycod commented Jun 20, 2021

The only way for fates to support botblock add bot then, is to make a whole new api and since this won’t be used by the official client, it may break since it won’t be as well tested and stable as the official api

@MattIPv4
Copy link
Member

The only way for fates to support botblock add bot then, is to make a whole new api and since this won’t be used by the official client, it may break since it won’t be as well tested and stable as the official api

With all due respect, oh well, that is quite the shame. Moving on...

@cheesycod
Copy link
Contributor

cheesycod commented Jun 20, 2021

The only way for fates to support botblock add bot then, is to make a whole new api and since this won’t be used by the official client, it may break since it won’t be as well tested and stable as the official api

With all due respect, oh well, that is quite the shame. Moving on...

Yeah, the only real issue actually is that the add bot api in fates needs bot id and user id in the url and not request body while botblock uses request body for this. Other than that and the bot_details thing, most other things are trivial fixes tbh

@Andre601
Copy link

The only way for fates to support botblock add bot then, is to make a whole new api and since this won’t be used by the official client, it may break since it won’t be as well tested and stable as the official api

With all due respect, oh well, that is quite the shame. Moving on...

Yeah, the only real issue actually is that the add bot api in fates needs bot id and user id in the url and not request body while botblock uses request body for this. Other than that, most other things are trivial fixes tbh

This is at most an issue on how botblock sends the info or handles bot list URLs, but not how its own URL needs to look like.
With all due respect is Fates list not the go-to role model other pages should adabt to.

I assume there is some basic verification for the bot ID on botblock to make sure the user is the actual owner of it? Because then could we most likely get the ID from the logged in user and provide it to your list's URL to make it work.

But again: Your List isn't that important or special that botblock should adabt its own URL pattern for it.

@cheesycod
Copy link
Contributor

cheesycod commented Jun 20, 2021

The only way for fates to support botblock add bot then, is to make a whole new api and since this won’t be used by the official client, it may break since it won’t be as well tested and stable as the official api

With all due respect, oh well, that is quite the shame. Moving on...

Yeah, the only real issue actually is that the add bot api in fates needs bot id and user id in the url and not request body while botblock uses request body for this. Other than that, most other things are trivial fixes tbh

This is at most an issue on how botblock sends the info or handles bot list URLs, but not how its own URL needs to look like.

Yeah, botblocks own url def does not need to look like that. I was mainly talking about how botblock handles URLs and request bodies when sending to lists

With all due respect is Fates list not the go-to role model other pages should adabt to.

The way fates does it is similar to many discord API endpoints itself (https://discord.com/developers/docs/resources/guild#add-guild-member as a example) but I agree that fates is def not a role model other lists should use

I assume there is some basic verification for the bot ID on botblock to make sure the user is the actual owner of it? Because then could we most likely get the ID from the logged in user and provide it to your list's URL to make it work.

I don’t really need basic verification since that’s something that’s handled in bot reviews (as some ppl like using alts to post their bots). The only thing I care about is that user id and bot id as path parameters and the botblock key or user token in Authorization header

But again: Your List isn't that important or special that botblock should adabt its own URL pattern for it.

True, I could likely make a PR to add support for this if you wanted…

@cheesycod
Copy link
Contributor

cheesycod commented Jun 20, 2021

@Andre601 another option would be to make a separate gunicorn+fastapi proxy server to take requests from botblock, convert it to a format that works with fates list, make the api request to add bot and then return the result. Would that work with you?

@jacobmitchell04 jacobmitchell04 self-requested a review July 3, 2021 11:41
src/Routes/Bot.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jacobmitchell04 jacobmitchell04 left a comment

Choose a reason for hiding this comment

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

90% of it looks pretty decent, good job. I've made a few reccomendations

src/Dynamic/Includes/forms/addBot.pug Outdated Show resolved Hide resolved
src/Assets/js/joiSchema.js Outdated Show resolved Hide resolved
src/Routes/Bot.js Outdated Show resolved Hide resolved
src/Routes/Bot.js Outdated Show resolved Hide resolved
src/Routes/Bot.js Outdated Show resolved Hide resolved
src/Website.js Outdated Show resolved Hide resolved
src/Dynamic/bot/add.pug Outdated Show resolved Hide resolved
src/Dynamic/Includes/forms/list.pug Outdated Show resolved Hide resolved
src/Assets/js/bot/add.js Outdated Show resolved Hide resolved
src/Assets/js/bot/add.js Outdated Show resolved Hide resolved
@A-Trash-Coder A-Trash-Coder requested a review from MattIPv4 July 4, 2021 17:54
Copy link
Member

@MattIPv4 MattIPv4 left a comment

Choose a reason for hiding this comment

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

The public API endpoints return all data in the table for lists. This results in the add bot endpoint & secret being leaked. This is not acceptable.

I would suggest separating concerns and storing secret data for lists elsewhere.

@A-Trash-Coder
Copy link
Contributor Author

A-Trash-Coder commented Jul 10, 2021

The public API endpoints return all data in the table for lists. This results in the add bot endpoint & secret being leaked. This is not acceptable.

I would suggest separating concerns and storing secret data for lists elsewhere.

So I should store them in a separate table or would it work by removing the keys and url from the data returned? Removing they key and url from the data when returned seems like a more simple solution than making a table for only those. Unless this is frowned upon.

@A-Trash-Coder A-Trash-Coder requested a review from MattIPv4 July 10, 2021 22:22
@A-Trash-Coder
Copy link
Contributor Author

The public API endpoints return all data in the table for lists. This results in the add bot endpoint & secret being leaked. This is not acceptable.

I would suggest separating concerns and storing secret data for lists elsewhere.

I ended up just deleting the add_bot and add_bot_key from the json returned when fetching from the api.

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

Successfully merging this pull request may close these issues.

5 participants