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.

Sign up for GitHub

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: Allow actor preferred username changed #124

Merged
merged 4 commits into from
Oct 22, 2024
Merged

FEATURE: Allow actor preferred username changed #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
38df3ce
Allow actor usernames to be changed
angusmcleod May 7, 2024
76c4c89
Clarify username format in description and warning
angusmcleod May 7, 2024
f55bcf4
Merge remote-tracking branch 'upstream/main' into allow_actor_preferr…
angusmcleod Oct 22, 2024
5398b97
Update spec/requests/discourse_activity_pub/admin/actor_controller_sp…
pmusaraj Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions assets/javascripts/discourse/controllers/admin-plugins-activity-pub-actor-show.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export default class AdminPluginsActivityPubActorShow extends Controller {
@tracked enabled = this.actor.enabled;
@tracked saving = false;
@tracked saveResponse = null;
@tracked actor;

modelTypes = [
{
Expand Down
48 changes: 19 additions & 29 deletions assets/javascripts/discourse/templates/admin-plugins-activity-pub-actor-show.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
@model={{this.actor.model}}
@modelType={{this.actor.model_type}}
/>
<ActivityPubHandle @actor={{this.actor}} />
<div class="activity-pub-actor-enabled-toggle">
<DToggleSwitch
@state={{this.enabled}}
Expand All @@ -55,35 +56,24 @@
<div class="activity-pub-actor-form-container">
<div class="activity-pub-actor-form">
{{#if this.showForm}}
{{#if this.actor.handle}}
<section class="activity-pub-actor-setting activity-pub-handle-field">
<label>{{i18n "admin.discourse_activity_pub.actor.handle"}}</label>
<ActivityPubHandle @actor={{this.actor}} />
<div class="activity-pub-actor-setting-description">
{{i18n "admin.discourse_activity_pub.actor.handle_description"}}
</div>
</section>
{{else}}
<section class="activity-pub-actor-setting activity-pub-username">
<label for="activity-pub-username">{{i18n
"admin.discourse_activity_pub.actor.username"
}}</label>
<div class="activity-pub-username-input">
<Input
id="activity-pub-username"
@value={{this.actor.username}}
/>
<span
class="activity-pub-host"
>@{{this.site.activity_pub_host}}</span>
</div>
<div class="activity-pub-actor-setting-notice">
<span>{{d-icon "exclamation-triangle"}}{{i18n
"admin.discourse_activity_pub.actor.username_description"
}}</span>
</div>
</section>
{{/if}}
<section class="activity-pub-actor-setting activity-pub-username">
<label for="activity-pub-username">{{i18n
"admin.discourse_activity_pub.actor.username"
}}</label>
<div class="activity-pub-username-input">
<Input id="activity-pub-username" @value={{this.actor.username}} />
<span
class="activity-pub-host"
>@{{this.site.activity_pub_host}}</span>
</div>
<div class="activity-pub-actor-setting-description">
<span>{{i18n
"admin.discourse_activity_pub.actor.username_description"
min_length=this.siteSettings.min_username_length
max_length=this.siteSettings.max_username_length
}}</span>
</div>
</section>

<section class="activity-pub-actor-setting activity-pub-name">
<label for="activity-pub-name">{{i18n
Expand Down
6 changes: 2 additions & 4 deletions config/locales/client.en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,9 @@ en:
title: Login is required by the login required site setting.
description: Only outgoing follow requests and the attributes of enabled actors are published.
username: Username
username_description: Cannot be changed once saved.
handle: Handle
handle_description: Used to identify this actor on services using Webfinger (e.g. Mastodon).
username_description: "%{min_length} to %{max_length} letters, numbers, dashes or underscores."
name: Name
name_description: Used as a display name on some services (e.g. Mastodon).
name_description: Used as a display name on some services.
default_visibility: Visibility
default_visibility_description: All posts will be published via ActivityPub with this visibility.
post_object_type: Post object type
Expand Down
3 changes: 1 addition & 2 deletions config/locales/server.en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,8 +136,7 @@ en:
cant_create_actor_for_model_type: "Cannot create an actor for %{model_type} %{model_id}"
not_allowed_to_create_actor_for_model: "Not allowed to create an actor for %{model_id}"
no_options: "No options passed to ActorHandler"
no_change_when_set: An ActivityPub username can't be changed once set.
invalid_username: "ActivityPub usernames can only contain ASCII characters."
invalid_username: "Username must be %{min_length} to %{max_length} letters, numbers, dashes or underscores."
username_taken: "That username is already taken on this server."
auth:
error:
Expand Down
13 changes: 6 additions & 7 deletions lib/discourse_activity_pub/actor_handler.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,6 @@ def unique_actor_username?(username)
DiscourseActivityPubActor.username_unique?(username, model_id: model.id)
end

def username_changed?
(opts[:username].present? && actor && actor.username.present?) &&
actor.username != opts[:username]
end

def valid_actor?
if !actor&.ap&.can_belong_to&.include?(model_type.downcase.to_sym)
add_error(
Expand Down Expand Up @@ -240,7 +235,6 @@ def valid_actor_opts?
return invalid_opt("no_options") if opts.blank?

if opts[:username].present?
return invalid_opt("no_change_when_set") if username_changed?
return invalid_opt("invalid_username") if !valid_actor_username?(opts[:username])
return invalid_opt("username_taken") if !unique_actor_username?(opts[:username])
end
Expand All @@ -249,7 +243,12 @@ def valid_actor_opts?
end

def invalid_opt(key)
add_error(I18n.t("discourse_activity_pub.actor.warning.#{key}"))
opts = {}
if key == "invalid_username"
opts[:min_length] = SiteSetting.min_username_length
opts[:max_length] = SiteSetting.max_username_length
end
add_error(I18n.t("discourse_activity_pub.actor.warning.#{key}", opts))
Copy link
Contributor

Choose a reason for hiding this comment

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

@angusmcleod should the plugin also account for username change period in core? Not sure if that raises an error, and note that the setting also blocks username changes entirely if set to 0.

Copy link
Contributor Author

@angusmcleod angusmcleod Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The username changes handled by the ActorHandler will be unaffected by username_change_period, as changes are not filtered through the UserGaurdian that applies it. Even though we leverage some User username logic for the Actor username, e.g. UsernameValidator, I don't think it'd make sense to apply the UserGuardian, or restrictions like username_change_period to Actor changes, as different considerations are in play. We may restrict Actor username changes, but it will be for different reasons, e.g. that a plurality of other ActivityPub services do not support changes beyond a certain period. To put it another way, the considerations pertinent to Actors are sometimes overlapping, but mostly different from the considerations pertinent to Users (or other models), so I think it's best to keep them separate (which is also why we have a separate model for them).

false
end

Expand Down
3 changes: 3 additions & 0 deletions plugin.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,6 +1068,9 @@
)
else
actor.stored.name = actor.json[:name] if actor.json[:name].present?
actor.stored.username = actor.json[:preferredUsername] if actor.json[
:preferredUsername
].present?

if actor.json[:icon].present?
actor.stored.icon_url = DiscourseActivityPub::JsonLd.resolve_icon_url(actor.json[:icon])
Expand Down
25 changes: 5 additions & 20 deletions spec/lib/discourse_activity_pub/actor_handler_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,26 +266,11 @@
context "with a changed username" do
let!(:original_username) { actor.username }

before { setup_logging }
after { teardown_logging }

it "does not update the username" do
opts[:username] = "new_username"
described_class.update_or_create_actor(category, opts)
expect(actor.reload.username).to eq(original_username)
expect(category.reload.activity_pub_username).to eq(original_username)
end

it "logs an error" do
it "updates the username" do
opts[:username] = "new_username"
described_class.update_or_create_actor(category, opts)
expect(@fake_logger.warnings.first).to match(
I18n.t(
"discourse_activity_pub.actor.warning.no_change_when_set",
model_id: category.id,
model_type: category.class.name,
),
)
expect(actor.reload.username).to eq("new_username")
expect(category.reload.activity_pub_username).to eq("new_username")
end
end
end
Expand All @@ -308,8 +293,8 @@
expect(@fake_logger.warnings.first).to match(
I18n.t(
"discourse_activity_pub.actor.warning.invalid_username",
model_id: category.id,
model_type: category.class.name,
min_length: SiteSetting.min_username_length,
max_length: SiteSetting.max_username_length,
),
)
end
Expand Down
16 changes: 14 additions & 2 deletions spec/lib/discourse_activity_pub/ap/actor_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def perform(extra_json = {})
expect(actor.name).to eq(json["name"])
end

it "updates an actor if optional attributes have changed" do
it "updates name if name has changed" do
perform

actor = DiscourseActivityPubActor.find_by(ap_id: json["id"])
Expand All @@ -35,7 +35,19 @@ def perform(extra_json = {})
expect(actor.name).to eq("Bob McLeod")
end

it "creates a new actor if required attributes have changed" do
it "updates username if preferredUsername has changed" do
perform

actor = DiscourseActivityPubActor.find_by(ap_id: json["id"])
expect(actor.username).to eq("angus")

perform(preferredUsername: "bob")

actor = DiscourseActivityPubActor.find_by(ap_id: json["id"])
expect(actor.username).to eq("bob")
end

it "creates a new actor if id has changed" do
perform

original_id = json["id"]
Expand Down
12 changes: 6 additions & 6 deletions spec/requests/discourse_activity_pub/admin/actor_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,15 +254,15 @@ def actor_warning(key)
end

context "with a new username" do
it "returns the right error" do
it "returns an updated actor" do
put "/admin/plugins/ap/actor/#{actor.id}.json",
params: {
actor: {
username: "new_actor",
},
}
expect(response.status).to eq(400)
expect(response.parsed_body["errors"]).to include(actor_warning("no_change_when_set"))
expect(response.status).to eq(200)
expect(response.parsed_body["actor"]["username"]).to eq("new_actor")
end
end

Expand Down Expand Up @@ -296,15 +296,15 @@ def actor_warning(key)
end

context "with a new username" do
it "returns the right error" do
it "returns an updated actor" do
put "/admin/plugins/ap/actor/#{actor.id}.json",
params: {
actor: {
username: "new_actor",
},
}
expect(response.status).to eq(400)
expect(response.parsed_body["errors"]).to include(actor_warning("no_change_when_set"))
expect(response.status).to eq(200)
expect(response.parsed_body["actor"]["username"]).to eq("new_actor")
end
end

Expand Down