AkkomaGang/akkoma
AkkomaGang/akkoma
16
67
Fork 93
Code Issues 176 Pull requests 15 Projects 2 Releases 16 Packages Wiki Activity

WIP: Don’t pretend internal actors have follow(er|ing) collections #856

Draft
Oneric wants to merge 2 commits from Oneric/akkoma:fetch-actor-follow-collections into develop
pull from: Oneric/akkoma:fetch-actor-follow-collections
merge into: AkkomaGang:develop
Conversation 8 Commits 2 Files changed 2 +2 -3
Oneric commented 2024-11-23 01:51:47 +00:00
Member
Copy link

Fixes: #855

Afaict this doesn’t affect anything but internal.fetch since it appears to be the only internal. actor and there’s no nill nickname actor in my DB. In particular the built-in relay actor does not have an internal. prefix and thus should just be a regular user with an unusual AP id (/relay). (Though I’m not using relay functionality)

Fixes: #855 Afaict this doesn’t affect anything but `internal.fetch` since it appears to be the only `internal.` actor and there’s no `nill` nickname actor in my DB. In particular the built-in relay actor does not have an `internal.` prefix and thus should just be a regular user with an unusual AP id (`/relay`). *(Though I’m not using relay functionality)*
Oneric added 2 commits 2024-11-23 01:51:48 +00:00 
We don’t actually handle those endpoints just serving the
default frontend’s 404 HTML page, causing other servers to grumble
about it in their logs. Spec explicitly marks these as optional so
just stop pretending we have such collections.
As for real-world interop, Mastodon’s internal instance actor omits
them too, so this is fairly unlikely to cause any issues.

Notably, our built-in relay actor is not affect by this since it has
a non-nil nickname "relay" without "internal." prefix, i.e. it’s just
a regular actor with a funny ActivityPub ID.
In fact, afaict we don't have any "internal." actors
at the moment other than "internal.fetch".

Fixes: #855
cosmetic: adapt software name in internal actor descriptions
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
Details
ci/woodpecker/pr/docs Pipeline is pending approval
Details
ci/woodpecker/pr/lint Pipeline is pending approval
Details
ci/woodpecker/pr/test Pipeline is pending approval
Details
778e0ef196
ilja commented 2024-11-28 14:13:33 +00:00
Contributor
Copy link

The relay is also an "Application" actor, and its nickname is NULL in my database (so in code I guess represented as this nickname: nil)

Line 61 in 778e0ef
def render("user.json", %{user: %User{nickname: nil} = user}),

So at first glance, this may conflict with the relay actor too. It's possible the relay actor is only created when first enabled. For some reason I vaguely remember something like that, but I could be wrong. So maybe that's why you don't see it in your DB.

The relay is also an "Application" actor, and its nickname is NULL in my database (so in code I guess represented as this `nickname: nil`) https://akkoma.dev/AkkomaGang/akkoma/src/commit/778e0ef196689d2450a5b83a7907b58189c5f26e/lib/pleroma/web/activity_pub/views/user_view.ex#L61 So at first glance, this may conflict with the relay actor too. It's possible the relay actor is only created when first enabled. For some reason I vaguely remember something like that, but I could be wrong. So maybe that's why you don't see it in your DB.
Oneric commented 2024-11-28 19:13:21 +00:00
Author
Member
Copy link

So maybe that's why you don't see it in your DB.

Nope, the thing is i checked this before and it does show up, but behold:

SELECT nickname, ap_id, local
FROM users
WHERE nickname IS NULL OR
      (local = true AND (
        nickname LIKE 'internal%'
        OR invisible = true
      ));
nickname ap_id local
internal.fetch https://<domain>/internal/fetch t
relay https://<domain>/relay t

I guess whether it gets a nickname or not changed at some point; thanks for pointing this out
Do you by chance have any more, legacy NULL nicknames in your DB?

We could limit omission to just internal.*, migrate old relay actors or just keep the omission and check whether we run into any real world problems.

> So maybe that's why you don't see it in your DB. Nope, the thing is i checked this before and it _does_ show up, but behold: ```sql SELECT nickname, ap_id, local FROM users WHERE nickname IS NULL OR (local = true AND ( nickname LIKE 'internal%' OR invisible = true )); ``` nickname | ap_id | local ----------------|-----------------------------------------------------|------- internal.fetch | https://\<domain\>/internal/fetch | t relay | https://\<domain\>/relay | t I guess whether it gets a nickname or not changed at some point; thanks for pointing this out Do you by chance have any more, legacy NULL nicknames in your DB? We could limit omission to just `internal.*`, migrate old `relay` actors or just keep the omission and check whether we run into any real world problems.
ilja commented 2024-11-28 19:44:10 +00:00
Contributor
Copy link

There's a bunch NULL nicknames, but relay is the only local one. internal.fetch and relay are also the only local actors of type "Aplication".

In case it's usefull, here's the database entries 
SELECT * FROM users
WHERE actor_type  = 'Application'
and local

id                                  |email|password_hash|name|nickname      |bio|inserted_at            |updated_at             |ap_id                            |avatar|local|follower_address                           |last_refreshed_at|tags|last_digest_emailed_at |following_address|keys      |banner|background|note_count|follower_count|following_count|is_locked|is_confirmed|password_reset_pending|confirmation_token|default_scope|blocks|domain_blocks|mutes|muted_reblogs|muted_notifications|subscribers|is_active|no_rich_text|ap_enabled|is_moderator|is_admin|show_role|uri|hide_followers_count|hide_follows_count|hide_followers|hide_follows|hide_favorites|email_notifications|mascot|emoji|pleroma_settings_store|fields|raw_fields|is_discoverable|invisible|notification_settings                                                                                          |skip_thread_containment|also_known_as|allow_following_move|actor_type |multi_factor_authentication_settings|public_key|inbox|shared_inbox|raw_bio|accepts_chat_messages|is_approved|registration_reason|last_active_at|disclose_client|pinned_objects|featured_address                                      |status_ttl_days|mastofe_settings|is_suggested|last_status_at|language|accepts_direct_messages_from|permit_followback|
------------------------------------+-----+-------------+----+--------------+---+-----------------------+-----------------------+---------------------------------+------+-----+-------------------------------------------+-----------------+----+-----------------------+-----------------+----------+------+----------+----------+--------------+---------------+---------+------------+----------------------+------------------+-------------+------+-------------+-----+-------------+-------------------+-----------+---------+------------+----------+------------+--------+---------+---+--------------------+------------------+--------------+------------+--------------+-------------------+------+-----+----------------------+------+----------+---------------+---------+---------------------------------------------------------------------------------------------------------------+-----------------------+-------------+--------------------+-----------+------------------------------------+----------+-----+------------+-------+---------------------+-----------+-------------------+--------------+---------------+--------------+------------------------------------------------------+---------------+----------------+------------+--------------+--------+----------------------------+-----------------+
00000000-0000-0000-0000-00000000011c|     |             |    |              |   |2018-10-13 15:08:01.520|2024-08-18 16:10:24.000|https://ilja.space/relay         |      |true |https://ilja.space/relay/followers         |                 |{}  |2019-11-11 05:40:25.000|                 |[REDACTED]|      |{}        |         0|            10|              0|false    |true        |false                 |                  |public       |{}    |{}           |{}   |{}           |{}                 |{}         |true     |false       |false     |false       |false   |false    |   |false               |false             |false         |false       |false         |{"digest": false}  |      |{}   |{}                    |[]    |[]        |false          |true     |{"local": true, "remote": true, "follows": true, "followers": true, "non_follows": true, "non_followers": true}|false                  |{}           |true                |Application|{}                                  |          |     |            |       |true                 |true       |                   |              |true           |{}            |https://ilja.space/relay/collections/featured         |               |                |false       |              |        |everybody                   |false            |
0000016e-58c3-eedd-3548-46c141160000|     |             |    |internal.fetch|   |2019-11-11 04:41:06.000|2019-11-11 04:43:05.000|https://ilja.space/internal/fetch|      |true |https://ilja.space/internal/fetch/followers|                 |{}  |2019-11-11 05:41:06.000|                 |[REDACTED]|{}    |{}        |         0|             2|              0|false    |true        |false                 |                  |public       |{}    |{}           |{}   |{}           |{}                 |{}         |true     |false       |false     |false       |false   |true     |   |false               |false             |false         |false       |true          |{"digest": false}  |      |{}   |{}                    |[]    |[]        |false          |true     |{"follows": true, "followers": true, "non_follows": true, "non_followers": true}                               |false                  |{}           |true                |Application|{}                                  |          |     |            |       |true                 |true       |                   |              |true           |{}            |https://ilja.space/internal/fetch/collections/featured|               |                |false       |              |        |everybody                   |false            |
There's a bunch NULL nicknames, but relay is the only local one. internal.fetch and relay are also the only local actors of type "Aplication". <details> <summary>In case it's usefull, here's the database entries</summary> ```sql SELECT * FROM users WHERE actor_type = 'Application' and local ``` ``` id |email|password_hash|name|nickname |bio|inserted_at |updated_at |ap_id |avatar|local|follower_address |last_refreshed_at|tags|last_digest_emailed_at |following_address|keys |banner|background|note_count|follower_count|following_count|is_locked|is_confirmed|password_reset_pending|confirmation_token|default_scope|blocks|domain_blocks|mutes|muted_reblogs|muted_notifications|subscribers|is_active|no_rich_text|ap_enabled|is_moderator|is_admin|show_role|uri|hide_followers_count|hide_follows_count|hide_followers|hide_follows|hide_favorites|email_notifications|mascot|emoji|pleroma_settings_store|fields|raw_fields|is_discoverable|invisible|notification_settings |skip_thread_containment|also_known_as|allow_following_move|actor_type |multi_factor_authentication_settings|public_key|inbox|shared_inbox|raw_bio|accepts_chat_messages|is_approved|registration_reason|last_active_at|disclose_client|pinned_objects|featured_address |status_ttl_days|mastofe_settings|is_suggested|last_status_at|language|accepts_direct_messages_from|permit_followback| ------------------------------------+-----+-------------+----+--------------+---+-----------------------+-----------------------+---------------------------------+------+-----+-------------------------------------------+-----------------+----+-----------------------+-----------------+----------+------+----------+----------+--------------+---------------+---------+------------+----------------------+------------------+-------------+------+-------------+-----+-------------+-------------------+-----------+---------+------------+----------+------------+--------+---------+---+--------------------+------------------+--------------+------------+--------------+-------------------+------+-----+----------------------+------+----------+---------------+---------+---------------------------------------------------------------------------------------------------------------+-----------------------+-------------+--------------------+-----------+------------------------------------+----------+-----+------------+-------+---------------------+-----------+-------------------+--------------+---------------+--------------+------------------------------------------------------+---------------+----------------+------------+--------------+--------+----------------------------+-----------------+ 00000000-0000-0000-0000-00000000011c| | | | | |2018-10-13 15:08:01.520|2024-08-18 16:10:24.000|https://ilja.space/relay | |true |https://ilja.space/relay/followers | |{} |2019-11-11 05:40:25.000| |[REDACTED]| |{} | 0| 10| 0|false |true |false | |public |{} |{} |{} |{} |{} |{} |true |false |false |false |false |false | |false |false |false |false |false |{"digest": false} | |{} |{} |[] |[] |false |true |{"local": true, "remote": true, "follows": true, "followers": true, "non_follows": true, "non_followers": true}|false |{} |true |Application|{} | | | | |true |true | | |true |{} |https://ilja.space/relay/collections/featured | | |false | | |everybody |false | 0000016e-58c3-eedd-3548-46c141160000| | | |internal.fetch| |2019-11-11 04:41:06.000|2019-11-11 04:43:05.000|https://ilja.space/internal/fetch| |true |https://ilja.space/internal/fetch/followers| |{} |2019-11-11 05:41:06.000| |[REDACTED]|{} |{} | 0| 2| 0|false |true |false | |public |{} |{} |{} |{} |{} |{} |true |false |false |false |false |true | |false |false |false |false |true |{"digest": false} | |{} |{} |[] |[] |false |true |{"follows": true, "followers": true, "non_follows": true, "non_followers": true} |false |{} |true |Application|{} | | | | |true |true | | |true |{} |https://ilja.space/internal/fetch/collections/featured| | |false | | |everybody |false | ``` </details>
Oneric commented 2024-11-28 19:52:39 +00:00
Author
Member
Copy link

Thank you it is helpful and showed yet aother thing: both relay and internal.actor also are marked as actor_type = Person in my database °~°
(internal.fetch still gets exposed as an Application though since its special endpoint doesn't even read the property)

Thank you it is helpful and showed yet aother thing: both `relay` and `internal.actor` also are marked as `actor_type = Person` in my database °~° *(`internal.fetch` still gets exposed as an `Application` though since its special endpoint doesn't even read the property)*
ilja commented 2024-11-28 20:56:08 +00:00
Contributor
Copy link

both relay and internal.actor also are marked as actor_type = Person in my database °~°

huh, that shouldn't be the case since #457

But, yeah, that change apparently didn't really do anything wrt federation for the reason you say. I only realised that today when seeing the code in lib/pleroma/web/activity_pub/views/user_view.ex.

> both `relay` and `internal.actor` also are marked as `actor_type = Person` in my database °~° huh, that shouldn't be the case since https://akkoma.dev/AkkomaGang/akkoma/pulls/457 But, yeah, that change apparently didn't really do anything wrt federation for the reason you say. I only realised that today when seeing the code in *lib/pleroma/web/activity_pub/views/user_view.ex*.
Oneric commented 2024-11-28 23:20:11 +00:00
Author
Member
Copy link

yeah, that change apparently didn't really do anything wrt federation

it might have done something for relay depending on when its nickname stopped being NULL

huh, that shouldn't be the case since #457

The db was definitely created later than this — but create_service_actor also doesn't set type: "Application" atm.

Querying git log -S reveals: ... PR #457 was accidentally reverted 5 months after it got merged as part of a translation update in eba3cce77b 🥴

> yeah, that change apparently didn't really do anything wrt federation it might have done something for `relay` depending on when its nickname stopped being `NULL` > huh, that shouldn't be the case since #457 The db was definitely created later than this — but `create_service_actor` also doesn't set `type: "Application"` atm. Querying `git log -S` reveals: ... PR #457 was accidentally reverted 5 months after it got merged as part of a translation update in eba3cce77b45e89028f7e6bfa708b469b7416914 🥴
👀 1
Oneric commented 2024-11-29 00:01:45 +00:00
Author
Member
Copy link

yeah, that change apparently didn't really do anything wrt federation

it might have done something for relay depending on when its nickname stopped being NULL

relay’s nickname stopped being NULL5 years ago in 70410dfafd (subsequently edited again by 7dc275b69b), so your PR last year did affect federation for the relay actor just not internal.fetch

I think it’s best to migrate old installs to a non-NULL nickname for consistency (and at the same time re-add #457). Changing the nickname to internal.relay while conceptually sensible, is prob a bad idea in practice given relay nicknames are already in use and afaik at least Mastodon doesn't allow for nickname changes.
If we do a data migration anyway, we could also stop adding follow collection addresses to internal* actors in the first place (delete for existing dbs) and then use values from the db during render("service.json", user) or omit the fields if NULL. This seems cleanest and allows for future internal actors with follower collections (what relay probably should’ve been) — though then again we didn't get any new internal actors ever, so it’s unlikely to matter

> > yeah, that change apparently didn't really do anything wrt federation > > it might have done something for relay depending on when its nickname stopped being NULL `relay`’s nickname stopped being `NULL`5 years ago in 70410dfafd272bd1f38602446cc4f6e83645326f (subsequently edited again by 7dc275b69bbd50e7a6944c76c5541c0a9c41a051), so your PR last year did affect federation for the `relay` actor just not `internal.fetch` I think it’s best to migrate old installs to a non-NULL nickname for consistency *(and at the same time re-add #457)*. Changing the nickname to `internal.relay` while conceptually sensible, is prob a bad idea in practice given `relay` nicknames are already in use and afaik at least Mastodon doesn't allow for nickname changes. If we do a data migration anyway, we could also stop adding follow collection addresses to `internal*` actors in the first place *(delete for existing dbs)* and then use values from the db during `render("service.json", user)` or omit the fields if `NULL`. This seems cleanest and allows for future internal actors with follower collections *(what `relay` probably should’ve been)* — though then again we didn't get any new internal actors ever, so it’s unlikely to matter
👍 1
ilja commented 2024-11-29 05:56:42 +00:00
Contributor
Copy link

Sounds sensible, yes.

This seems cleanest and allows for future internal actors with follower collections

If a goal is to use "service.json" for these type of Application actors, you could maybe change the checks

  # the instance itself is not a Person, but instead an Application
  def render("user.json", %{user: %User{nickname: nil} = user}),
    do: render("service.json", %{user: user})

  def render("user.json", %{user: %User{nickname: "internal." <> _} = user}),
    do: render("service.json", %{user: user}) |> Map.put("preferredUsername", user.nickname)

to something like

  def render("user.json", %{user: %User{type: "Application"} = user}),
    do: render("service.json", %{user: user})

This makes it more explicit and clearer from the code. It also makes it so that the relay (and possible future Application actors) will use it regardless of nickname.

Sounds sensible, yes. > This seems cleanest and allows for future internal actors with follower collections If a goal is to use "service.json" for these type of Application actors, you could maybe change the checks ``` # the instance itself is not a Person, but instead an Application def render("user.json", %{user: %User{nickname: nil} = user}), do: render("service.json", %{user: user}) def render("user.json", %{user: %User{nickname: "internal." <> _} = user}), do: render("service.json", %{user: user}) |> Map.put("preferredUsername", user.nickname) ``` to something like ``` def render("user.json", %{user: %User{type: "Application"} = user}), do: render("service.json", %{user: user}) ``` This makes it more explicit and clearer from the code. It also makes it so that the relay (and possible future Application actors) will use it regardless of nickname.
Oneric changed title from Don’t pretend internal actors have follow(er|ing) collections to WIP: Don’t pretend internal actors have follow(er|ing) collections 2025-01-03 19:37:56 +00:00
Some checks are pending
ci/woodpecker/pr/build-amd64 Pipeline is pending approval
Details
ci/woodpecker/pr/build-arm64 Pipeline is pending approval
Details
ci/woodpecker/pr/docs Pipeline is pending approval
Details
ci/woodpecker/pr/lint Pipeline is pending approval
Details
ci/woodpecker/pr/test Pipeline is pending approval
Details
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u fetch-actor-follow-collections:Oneric-fetch-actor-follow-collections
git checkout Oneric-fetch-actor-follow-collections
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
approved, awaiting change

   
bug

Something is not working

  
configuration

This requires config changes

  
documentation

This is about human-readable docs

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
extremely low priority

   
feature request

Something new?

  
Fix it yourself

Support has been attempted

  
help wanted

Need some help

  
invalid

Something is wrong

  
mastodon_api

   
needs docs

   
needs tests

   
not a bug

   
planned

   
pleroma_api

   
privacy

   
question

More information is needed

  
static_fe

   
triage

   
wontfix

This won't be fixed

No labels
approved, awaiting change
bug
configuration
documentation
duplicate
enhancement
extremely low priority
feature request
Fix it yourself
help wanted
invalid
mastodon_api
needs docs
needs tests
not a bug
planned
pleroma_api
privacy
question
static_fe
triage
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: AkkomaGang/akkoma#856
No description provided.
Delete branch "Oneric/akkoma:fetch-actor-follow-collections"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?