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

Instance blocks with mod log entry and expiration (fixes #2506) #5214

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

Nutomic
Copy link
Member

@Nutomic Nutomic commented Nov 19, 2024

  • Federation allowlist cannot be updated via PUT /api/v3/site anymore, instead there are new endpoints POST /api/v3/admin/block_instance and POST /api/v3/admin/allow_instance
  • Allow instances takes an optional reason and expiry time, and can be shown in mod log (similar to user bans)
  • For newly allowed instances there is also an optional reason, but no expiry as that wouldnt make sense

The main implementation is done, will wait for initial code review before making changes to js client and api tests.

@Nutomic Nutomic changed the title WIP: Instance blocks with mod log entry and expiration (fixes #2506) Instance blocks with mod log entry and expiration (fixes #2506) Nov 19, 2024
.route("/list", get().to(list_taglines)),
)
.route("block_instance", post().to(admin_block_instance))
.route("allow_instance", post().to(admin_allow_instance)),
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed useless the web:: prefix all over this file, only real change is here.

crates/api/src/site/admin_allow_instance.rs Outdated Show resolved Hide resolved
crates/api/src/site/admin_block_instance.rs Outdated Show resolved Hide resolved
crates/api/src/site/admin_block_instance.rs Show resolved Hide resolved
crates/api_common/src/site.rs Show resolved Hide resolved
crates/db_schema/src/impls/federation_allowlist.rs Outdated Show resolved Hide resolved
crates/api/src/site/admin_allow_instance.rs Outdated Show resolved Hide resolved
crates/db_schema/src/source/federation_allowlist.rs Outdated Show resolved Hide resolved
crates/db_schema/src/source/federation_blocklist.rs Outdated Show resolved Hide resolved
@Nutomic
Copy link
Member Author

Nutomic commented Nov 25, 2024

Updated

pub struct BlockInstanceResponse {
pub blocked: bool,
pub struct AdminAllowInstanceParams {
pub instance: String,
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to change these from instance_id to domain, otherwise it would be very complicated to initialize the tests. It would also be possible to allow both instance id or domain in the same field, but ts-rs would probably not understand that.

Copy link
Member

@dessalines dessalines Nov 27, 2024

Choose a reason for hiding this comment

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

That's fine, especially since you might not be connected to the instance yet anyway, so it might not have an id, and you need to read or create it in the API route.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Thx for the re-org of mod tables also, that'll be useful for my work on #2444

pub struct BlockInstanceResponse {
pub blocked: bool,
pub struct AdminAllowInstanceParams {
pub instance: String,
Copy link
Member

@dessalines dessalines Nov 27, 2024

Choose a reason for hiding this comment

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

That's fine, especially since you might not be connected to the instance yet anyway, so it might not have an id, and you need to read or create it in the API route.

blocked bool NOT NULL,
reason text,
expires timestamptz,
published timestamptz NOT NULL DEFAULT now()
Copy link
Member

Choose a reason for hiding this comment

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

All the mod tables use a when_ column instead of published. We should pry keep that uniform for now.

admin_person_id int NOT NULL REFERENCES person (id) ON UPDATE CASCADE ON DELETE CASCADE,
allowed bool NOT NULL,
reason text,
published timestamptz NOT NULL DEFAULT now()
Copy link
Member

Choose a reason for hiding this comment

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

Same

var block_instance_params: AdminBlockInstanceParams = {
instance: "lemmy-alpha",
block: true,
reason: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

You should be able to just leave off reason, esp since it should be optional.

const params: AdminAllowInstanceParams = {
instance,
allow: true,
reason: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

Same

Comment on lines +443 to +450

diesel::delete(
federation_blocklist::table.filter(federation_blocklist::expires.lt(now().nullable())),
)
.execute(&mut conn)
.await
.inspect_err(|e| error!("Failed to remove federation_blocklist expired rows: {e}"))
.ok();
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good idea to move this to its own function, since the other two are ban expires, but this is an instance block expires.

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.

2 participants