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

Allowlisted Servers Validation #3

Open
Behold3th opened this issue Nov 21, 2021 · 3 comments
Open

Allowlisted Servers Validation #3

Behold3th opened this issue Nov 21, 2021 · 3 comments

Comments

@Behold3th
Copy link
Owner

Is your feature request related to a problem? Please describe.
In MessageReactionAdd, we perform a check for if the guild the reaction event is coming from is allowlisted. While this code was maintained by DEGEN, the check was isBanklessDao, which referenced the hardcoded constant for the BanklessDao guild id. As we pursue a multitenant architecture, we will need a scalable, maintainable, and protected way to add allowlisted servers as our GTM arm onboards new customers.

As a side note, this check should likely be performed before ANY of our commands run, which I think is an upgrade to the architecture as previously maintained. However, having this check here is good for sanity - these events are processed outside of the thread of commands. For instance, currently, Bountybot will message you in DMs after you run /bounty create. When you react to the final draft embed to publish it to the 🧀-bounty-board channel, you'll notice that this code path gets triggered. Here, guild.id is null, because it is a DM.

I believe there are a few options:

  1. Add the allowlisted guilds to a constants file, and reference that file in this check
  2. Add the allowlisted guilds to an Admin mongodb collection, where we will store the allowlisted guilds in an array of strings like allowlistedServers
  3. Add the allowlisted guilds to an env variable, allowing for staging/production differentiation
  4. Don't explicitly list allowlisted guilds anywhere (constants, env vars, database), but query the customer collection to find the record with the desired guild. If there is a record matching, that guild is allowlisted. If there is not a record matching, that guild has not been allowlisted.

Describe the solution you'd like
For now, I'm going with solution 4. The drawback of this solution while we are using mongo is that it will have a linear runtime. One strong benefit of this solution is reducing our operations load - while in any of the other solutions adding a customer is a two step process, in this solution it is only one (add the customer record to the customers collection).

Solution 1 is easy to implement, but suffers from the fact that this list will now be mutable through PRs. It heightens the risk we face when merging PRs.

Solution 2 seems illogical to me - why have a collection with only one record? what would the unique id of that record be?

Solution 3, from the maintenance perspective, will get messy from time and will likely need migration in the future. How do we audit the addition of a record? In the vercel UI, fitting hundreds of items in that list will get overwhelming. Plus, there is a maximum size of 4kb in env vars in Vercel per deployment. Once we hit hundreds of customers, we will approach that limit. We don't want to make a switch in 12 -24 months.

@Behold3th Behold3th changed the title Admin Collection Allowlisted Servers Validation Nov 21, 2021
@jordaniza
Copy link

Reviewing the options in sequence:

  1. Hard Pass, this is a shit version of a database with the drawbacks you described. We'd only do this as a stopgap if we didn't have control of the codebase at some stage, which is no longer an issue.
  2. Also would pass on this as the likelihood of decoupling from the Customers collection seems quite high
  3. Basically 1 with more steps
  4. Yes, this seems eminently sensible.

Your main issues with 1, 2, and 3 are keeping the guild Ids in sync with the customers collection. As far as I can tell, we want to maintain a single source of truth for what customers we have. For me, that's the customers collection. The most sensible place to check whether or not we have a customer, for me is that collection.

What I'd recommend is the following extension of 4:

We put an RPC endpoint into Bountyboard API to check if a customer exists or not. Bounty Bot can call this before each request. You have several layers to optmise in this case:

  1. Do not optimise (probably fine for the first 5 to 10 DAOs, until DB load grows)
  2. Server side caching, park a redis cache behind the RPC call so we don't need to hit Mongo each time. We can flush the cache periodically + on any POST/PUT/DELETE API calls to /customers
  3. Bot side caching - same as (2) but add a cache layer on the Bounty Bot side. Little bit harder to sync, but we could build a something like cache flush endpoint into bounty bot that is triggered when the customers collection is updated. Advantage here is that a cache hit is going to be orders of magnitude faster than a server side cache due to no network latency.

Essentially, you're rebuilding the static file as an automated caching layer, similar to JWT authorization. I don't see this as an enormous dev effort, and the first option should serve us for a little while until we're at the 00s req/second mark.

@PaulApivat
Copy link

4. Don't explicitly list allowlisted guilds anywhere (constants, env vars, database), but query the customer collection to find the record with the desired guild. If there is a record matching, that guild is allowlisted. If there is not a record matching, that guild has not been allowlisted.

Agree with your rationale @Behold3th , it should be fairly straightforward to query the customers collection, here are several options to search if "BanklessDAO" is allowlisted:

db.customers.find({customerName: 'BanklessDAO'})

or

db.customers.find({customer_id: '834499078434979890'})

We also currently have a allowlistedRoles key attached to a customer_id.

@tescher
Copy link

tescher commented Dec 17, 2021

At the same time can you generalize the hardcoded Bankless roles? Maybe create L0, L1, L2 general categories and assign roles to those categories in the customer's collection record.

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

No branches or pull requests

4 participants