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

[feature] domain block wildcarding #1178

Conversation

NyaaaWhatsUpDoc
Copy link
Member

For domain blocks, it would now also block all subdomains under that given domain.

e.g. if you block example.com then the following would also be blocked

  • gts.example.com
  • pleroma.example.com
  • gts.pleroma.example.com

@NyaaaWhatsUpDoc
Copy link
Member Author

Marking as draft as there's still some discussions to be had regarding

  • separation of logic, putting this in the database feels like putting too much into the database itself
  • how many max subdomain parts is a reasonable amount?

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc marked this pull request as draft November 29, 2022 18:16
@NyaaaWhatsUpDoc
Copy link
Member Author

Thinking about this now... To help separate the logic, once the state PR #1078 has been merged, we could move this logic out of the database quite easily since the main part that holds it here right now is the access to cache.Has(...)

@tsmethurst
Copy link
Contributor

This looks good so far, very neat implementation 😍 My only concern is that it adds a lot of database calls (up to three extras, right?) during authorization, which is a very hot path called on every incoming request. I know the cacheing will help a lot but this still makes me a little nervous. What do you think?

internal/db/bundb/domain.go Outdated Show resolved Hide resolved
internal/db/bundb/domain.go Outdated Show resolved Hide resolved
@igalic
Copy link
Contributor

igalic commented Nov 30, 2022

what about reversing a domain and comparing it?

reverse('gts.example.com').startsWith(reverse('example.com') +  '.')

shouldn't matter how many domain parts there are then

maybe there's even an endsWith() function, so you don't need to reverse it

@NyaaaWhatsUpDoc
Copy link
Member Author

what about reversing a domain and comparing it?

reverse('gts.example.com').startsWith(reverse('example.com') +  '.')

shouldn't matter how many domain parts there are then

maybe there's even an endsWith() function, so you don't need to reverse it

This is helpful if we have the domain block to compare an incoming domain against. But how do we structure this as part of a database call and a hashmap (cache) lookup?

@NyaaaWhatsUpDoc
Copy link
Member Author

This looks good so far, very neat implementation heart_eyes My only concern is that it adds a lot of database calls (up to three extras, right?) during authorization, which is a very hot path called on every incoming request. I know the cacheing will help a lot but this still makes me a little nervous. What do you think?

I agree, it's definitely a work in progress. If you have any brainwaves on it let me know! I'm going to keep pondering on it myself 🤔

@NyaaaWhatsUpDoc
Copy link
Member Author

Just had a thought -- the caching side of things could be sped up with a custom lookup system using a radix-trie to do glob-base matching, similar to what I have here: https://codeberg.org/gruf/go-glob/src/branch/main/multi.go.

Still need to figure out the database though.

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc force-pushed the feature/domain-block-wildcarding branch 3 times, most recently from ab56549 to ab0e0f0 Compare December 11, 2022 21:05
@NyaaaWhatsUpDoc
Copy link
Member Author

NyaaaWhatsUpDoc commented Dec 11, 2022

I actually came up with quite a nice solution for this, not too complicated but should handle things well 😀

Only thing left is I need to write some tests for and properly comment the new domain.BlockCache{} type!

Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

This is looking really nice :)

@NyaaaWhatsUpDoc NyaaaWhatsUpDoc marked this pull request as ready for review December 12, 2022 17:42
Copy link
Contributor

@tsmethurst tsmethurst left a comment

Choose a reason for hiding this comment

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

Looks great :) Just a comment on the comprehensibility of one of the functions (arguably the most important one) :P

internal/cache/domain/domain.go Show resolved Hide resolved
Signed-off-by: kim <grufwub@gmail.com>
…ted domains to max of 5 subdomains

Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
… memory

Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
…domain block getter funcs

Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
Signed-off-by: kim <grufwub@gmail.com>
NyaaaWhatsUpDoc added a commit to NyaaaWhatsUpDoc/gotosocial that referenced this pull request Dec 13, 2022
Signed-off-by: kim <grufwub@gmail.com>
@tsmethurst tsmethurst merged commit 69dd5fe into superseriousbusiness:main Dec 14, 2022
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.

3 participants