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

Change registrations to be disabled by default for new servers #29280

Merged
merged 3 commits into from Feb 22, 2024

Conversation

ClearlyClaire
Copy link
Contributor

@ClearlyClaire ClearlyClaire commented Feb 19, 2024

Existing servers which have never changed from the defaults will have closed registrations on update.

This also adds a short notice instructing admins to set up a moderation team before opening registrations.

image

@ClearlyClaire
Copy link
Contributor Author

ClearlyClaire commented Feb 19, 2024

Of note, this version of the PR will possibly turn registrations off for existing servers who have been running with open registrations. This is arguably a surprising change, and possibly an unwelcome one. Should we write it so that servers currently running on default settings are kept to open registrations?

EDIT: Changed to switch to open registrations if no setting has been saved to the database. Note that I'm not still 100% sure that's what we want, and also this would open registrations for instances which have disabled registrations by editing config/settings.yml rather than through the admin interface (I don't think there are a lot, but I'm sure they exist).

EDIT2: added a comment in config/settings.yml to get the attention of anyone who would have overridden this

@ClearlyClaire ClearlyClaire changed the title Change registrations to be disabled by default Change registrations to be disabled by default for new servers Feb 19, 2024
@ClearlyClaire ClearlyClaire force-pushed the registration-mode branch 3 times, most recently from 461ae54 to 4ab9e38 Compare February 19, 2024 17:29
@ClearlyClaire ClearlyClaire requested a review from a team February 20, 2024 13:20
@hatclub
Copy link

hatclub commented Feb 20, 2024

Of note, this version of the PR will possibly turn registrations off for existing servers who have been running with open registrations. This is arguably a surprising change, and possibly an unwelcome one. Should we write it so that servers currently running on default settings are kept to open registrations?

EDIT: Changed to switch to open registrations if no setting has been saved to the database. Note that I'm not still 100% sure that's what we want, and also this would open registrations for instances which have disabled registrations by editing config/settings.yml rather than through the admin interface (I don't think there are a lot, but I'm sure they exist).

EDIT2: added a comment in config/settings.yml to get the attention of anyone who would have overridden this

Is it not better that it defaults to closed where there is no obviously explicit preference and there's a note in the changelogs to prompt people to check and re-open if they really want it to still be wide open?

I get the desire to avoid "creating work" for a larger group of admins but otoh it feels like defaulting to secure is better than risking even one instance inadvertently opening (spam, financial cost, instances filling disks and breaking, etc), and will ensure people who aren't paying attention or don't understand what the change means end up with a closed instance which is probably better for everyone?

@vmstan
Copy link
Contributor

vmstan commented Feb 20, 2024

Of note, this version of the PR will possibly turn registrations off for existing servers who have been running with open registrations. This is arguably a surprising change, and possibly an unwelcome one. Should we write it so that servers currently running on default settings are kept to open registrations?

EDIT: Changed to switch to open registrations if no setting has been saved to the database. Note that I'm not still 100% sure that's what we want, and also this would open registrations for instances which have disabled registrations by editing config/settings.yml rather than through the admin interface (I don't think there are a lot, but I'm sure they exist).

EDIT2: added a comment in config/settings.yml to get the attention of anyone who would have overridden this

I'm confused why we would switch people to open after the update that changes the default for new installs to closed, or do I misunderstand what happens. If there was no setting in the database it would have already been open, and we're just preserving that?

@ThisIsMissEm
Copy link
Contributor

I'd definitely like to see closed by default or approval by default — if this temporarily affects admins during upgrade, if they upgrade, then they can always change to open or approval based registration.

We could also do a notice in the admin dashboard if no option is stored in the database maybe?

@ClearlyClaire
Copy link
Contributor Author

If there was no setting in the database it would have already been open, and we're just preserving that?

That is what this migration does!

@hatclub
Copy link

hatclub commented Feb 21, 2024

If there was no setting in the database it would have already been open, and we're just preserving that?

That is what this migration does!

Unless they had previously edited settings.yml to close it rather than setting it in the DB, right?

I do not think risking opening registrations for any admin who consciously closed them, vs closing them for admins who never indicated an explicit preference to opt in to open registrations, is better?

@ClearlyClaire
Copy link
Contributor Author

Unless they had previously edited settings.yml to close it rather than setting it in the DB, right?

In this case, the admin would face a merge conflict, and the line where this has been changed has a comment explicitly calling out the migration.

@ClearlyClaire
Copy link
Contributor Author

Changed to remove the migration and not preserve the setting if it has never been changed from the default (that is, existing instances that are open by virtue of not having changed the default setting, will have closed registrations on update).

@ThisIsMissEm
Copy link
Contributor

@ClearlyClaire could we actually change this such that open registration isn't even an option if moderator count is less than say 3 mods?

config/settings.yml Show resolved Hide resolved
@ClearlyClaire
Copy link
Contributor Author

@ClearlyClaire could we actually change this such that open registration isn't even an option if moderator count is less than say 3 mods?

I'm not very comfortable with this idea, as open registrations may mean a lot of different things based on e.g. EMAIL_DOMAIN_ALLOWLIST. I don't want to rush a change with such an arbitrary restriction.

@ThisIsMissEm
Copy link
Contributor

@ClearlyClaire could we actually change this such that open registration isn't even an option if moderator count is less than say 3 mods?

I'm not very comfortable with this idea, as open registrations may mean a lot of different things based on e.g. EMAIL_DOMAIN_ALLOWLIST. I don't want to rush a change with such an arbitrary restriction.

Perhaps we should split this off into a separate issue discussion?

Copy link
Contributor

@Saiv46 Saiv46 left a comment

Choose a reason for hiding this comment

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

LGTM, admin panel would need overhaul anyway

@ClearlyClaire ClearlyClaire added this pull request to the merge queue Feb 22, 2024
Merged via the queue into main with commit b719048 Feb 22, 2024
53 checks passed
@ClearlyClaire ClearlyClaire deleted the registration-mode branch February 22, 2024 13:33
kmycode pushed a commit to kmycode/mastodon that referenced this pull request Feb 23, 2024
skerit pushed a commit to 11ways/mastodon that referenced this pull request Feb 27, 2024
Ember-ruby pushed a commit to Ember-ruby/mastodon-glitch that referenced this pull request Mar 19, 2024
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.

None yet

6 participants