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 whitelist mode #11291

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@Gargron
Copy link
Member

commented Jul 11, 2019

Fix #11237

  • Authorized fetch mode
  • Only process accounts from allowed domains
  • Disable LD-Signatures
  • Disable app-only and unauthenticated REST API access
  • Admin UI for whitelisting domains
  • Disable activity and peers APIs
  • Disable /about and /about/more pages
  • Disable profile directory
  • Disable unauthenticated access to public account and status pages as well as feeds

@Gargron Gargron force-pushed the feature-whitelist-mode-v2 branch 2 times, most recently from d4c955b to 9d5d454 Jul 11, 2019

@ThibG
Copy link
Collaborator

left a comment

Seems mostly good, obviously still missing UI.
Also, switching an existing instance to whitelist mode should be considered. As far as I can see, the current state of this PR will still allow remote followers from non-whitelisted instances to receive local toots, but not send interactions back.

Show resolved Hide resolved app/services/concerns/payloadable.rb Outdated
@kpcyrd

This comment has been minimized.

Copy link

commented Jul 18, 2019

Wouldn't this discourage self hosted instances and instead push everybody into the few instances that are too big to fail, like mastodon.social?

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Jul 18, 2019

@kpcyrd I understand the concern, but I think a whitelist mode remain useful both for some small communities and for stuff like schools or other organizations. In both cases, “big instances that are too big to fail” such as mastodon.social are probably not what those whitelist-based instances would be interested in federating with in the first place.

@Gargron Gargron force-pushed the feature-whitelist-mode-v2 branch 8 times, most recently from 39186da to 77f2fc4 Jul 24, 2019

@Gargron Gargron marked this pull request as ready for review Jul 25, 2019

@Gargron Gargron requested a review from ThibG Jul 25, 2019

@ThibG
Copy link
Collaborator

left a comment

  • Disable activity and peers APIs
  • Disable profile directory
  • Disable unauthenticated access to public account and status pages as well as feeds

Those are all things I am not sure about and that can (or should imho, for the last one) be changed independently.

  • Disable /about and /about/more pages

I am pretty concerned about this one, people may want to have approval-based signup on a whitelisted instance, it makes sense for them to have /about and /about/more.

Also, the case of switching from a blocklist-based to a whitelist-based approach on an existing instance will not remove existing known accounts from non-whitelisted instances from search results, and it will not stop them from receiving toots they are mentioned in. I think it also lets people fetch statuses from already-known remote accounts that are not whitelisted?

Show resolved Hide resolved app/controllers/api/base_controller.rb
Show resolved Hide resolved app/controllers/concerns/account_owned_concern.rb Outdated
Show resolved Hide resolved app/helpers/domain_control_helper.rb Outdated

@Gargron Gargron force-pushed the feature-whitelist-mode-v2 branch from 77f2fc4 to 09a3b3a Jul 26, 2019

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

I am pretty concerned about this one, people may want to have approval-based signup on a whitelisted instance, it makes sense for them to have /about and /about/more.

I just don't think that "who runs it" and "how many users are there" should be published in this mode, the instance is essentially for private use only. Anything that must still be public can be put on the /terms page.

Also, the case of switching from a blocklist-based to a whitelist-based approach on an existing instance will not remove existing known accounts from non-whitelisted instances from search results, and it will not stop them from receiving toots they are mentioned in. I think it also lets people fetch statuses from already-known remote accounts that are not whitelisted?

Well, how do you imagine it should work? Do you want me to purge the accounts at boot time? That could really backfire if an admin doesn't realize it's what this environment variable does. I would put it into a tootctl command instead.

@Gargron Gargron requested a review from ThibG Jul 26, 2019

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Jul 26, 2019

I just don't think that "who runs it" and "how many users are there" should be published in this mode, the instance is essentially for private use only. Anything that must still be public can be put on the /terms page.

I am not completely convinced. For instance, https://awoo.space/about is a whitelist-based instance and does have an about page (and also user directory).

I would put it into a tootctl command instead.

Yes, that is what I would suggest as well.

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

I am not completely convinced. For instance, https://awoo.space/about is a whitelist-based instance and does have an about page (and also user directory).

Awoo.space can simply keep using whatever they're using. They don't have to use this code. This PR's objective is not to create more of awoo.space or counter.social, but to help private networks like schools and universities. Networks that do not attract random people from the internet.

@Gargron Gargron force-pushed the feature-whitelist-mode-v2 branch 2 times, most recently from 2a19ffd to 902cb42 Jul 26, 2019

@Gargron Gargron requested a review from ThibG Jul 29, 2019

@Gargron Gargron force-pushed the feature-whitelist-mode-v2 branch from 902cb42 to 84be5f0 Jul 29, 2019

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

tootctl domains purge --whitelist-mode will remove accounts from domains that aren't whitelisted

@ThibG
Copy link
Collaborator

left a comment

ActivityPub::InboxesController also inherits from Api::BaseController and should skip :require_authenticated_user!

Show resolved Hide resolved app/controllers/api/base_controller.rb Outdated

@Gargron Gargron force-pushed the feature-whitelist-mode-v2 branch from 84be5f0 to 9c9cdd1 Jul 30, 2019

@Gargron Gargron requested a review from ThibG Jul 30, 2019

@ThibG

ThibG approved these changes Jul 30, 2019

@Gargron Gargron merged commit 24552b5 into master Jul 30, 2019

2 checks passed

build-and-test Workflow: build-and-test
Details
codeclimate All good!
Details

@Gargron Gargron deleted the feature-whitelist-mode-v2 branch Jul 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.