Conversation
| * "id": "https://client.example/6ae15297", | ||
| * "type": "Follow", | ||
| * "actor": "https://client.example/actor", | ||
| * "object": "https://www.w3.org/ns/activitystreams#Public" |
There was a problem hiding this comment.
He the relay should actually understand both, as they're semantically the same, I'd file a bug with the relay implementation
There was a problem hiding this comment.
I am not sure about this.
The object of the Follow request MUST be the fully expanded URI of the Public pseudo-collection (https://www.w3.org/ns/activitystreams#Public).
As far as I understand, but you can correct me:
https://www.w3.org/ns/activitystreams#Publicfully expandedas:publicnot fully expanded
Imagine building a new relay software: I would look at the spec and see that the object property should be equal to https://www.w3.org/ns/activitystreams#Public. So I would just check for this. This is why I think using https://www.w3.org/ns/activitystreams#Public would make it more compatible with more relay softwares. But at this point I only tested one software.
|
Would probably also make sense to abstract out functions for determining "should this be relayed" and "forward this to a relay" |
|
@dahlia can you look into the verify key issue? You can use |
It seems that Fedify fails to parse its public key is because it's encoded in PEM-PKCS#1 whereas Fedify expects PEM-SPKI, which is much more common in the fediverse (as far as I know). Hmm… should Fedify accept PEM-PKCS#1 as well? 🤔 |
I think, yes 🙃 As far as I know, it is not against the spec. Even though I think this is not the only issue with the tested relay software. But this would bring it one step further. I naively tried to patch fedify yesterday, but what I did was 100% wrong 🥲 |
|
Now Fedify accepts PEM-PKCS#1-encoded RSA public keys (besides PEM-SPKI-encoded ones), and it will be shipped in Fedify v1.5.0, the next minor release. Until then, you could give it a try Fedify v1.5.0-dev.654+b5166915, which is an unstable release. |
|
The next issue I found. This is the http signature: The keyId is set to I am not sure if this is also an incorrect behavior of fedify. https://swicg.github.io/activitypub-http-signature/#how-to-obtain-a-signature-s-public-key
Maybe the spec is meant this way: Fetch the key from keyId. If keyId specifies a specific key, use it. Otherwise, use first. Damn, activity pub is harder than I could have ever imagined 😁 Respect for implementing fedify. |
Whether it is Fedify's fault or not, Fedify should behave as liberal as possible in accordance with the robustness principle— |
|
Fedify now does the best to find the appropriate public key. If there is no fragment in |
|
I am not sure how to go forward with this PR. The most important parts are done. I may consider change it state from draft to ready. Everything else can be changed/implemented later. Otherwise this PR might stay in a infinite draft limbo. Any option? |
|
If you change it to ready, I'll review it! |
dahlia
left a comment
There was a problem hiding this comment.
I'd like to test this on my local setup. Could you let me know how to test it? Sorry, I'm not familiar with ActivityPub relays.
| export class RelayUndo extends Undo { | ||
| async toJsonLd(options?: { | ||
| format?: "compact" | "expand"; | ||
| contextLoader?: DocumentLoader; | ||
| context?: | ||
| | string | ||
| | Record<string, string> | ||
| | (string | Record<string, string>)[]; | ||
| }): Promise<unknown> { | ||
| await this.getObject(); | ||
| const json = (await super.toJsonLd(options)) as { object: unknown }; | ||
| json.object = await (await this.getObject())?.toJsonLd(); | ||
| return json; | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of having this subclass, how about hydrating the property before calling toJsonLd()?
|
|
||
| accounts.get("/", async (c) => { | ||
| const owners = await db.query.accountOwners.findMany({ | ||
| where: ne(accountOwners.handle, HOLLO_RELAY_ACTOR_USERNAME), |
There was a problem hiding this comment.
Sometimes you've compare account_owners.id with HOLLO_RELAY_ACTOR_ID, and sometimes account_owners.handle with HOLLO_RELAY_ACTOR_USERNAME. Is there any difference between both ways?
There was a problem hiding this comment.
I think this check should be using the ID here, not the username.
There was a problem hiding this comment.
This may also affect the setup page, which checks the number of actors on the server.
There was a problem hiding this comment.
Oh, it may affect the NodeInfo stats as well:
hollo/src/federation/nodeinfo.ts
Lines 18 to 28 in 65c2b11
src/pages/accounts.tsx
Outdated
| const nameEmojis = await extractCustomEmojis(db, name); | ||
| const emojis = { ...nameEmojis, ...bioResult.emojis }; | ||
| const handle = `@${username}@${fedCtx.host}`; | ||
| if (handle === `@${HOLLO_OFFICIAL_ACCOUNT}@${fedCtx.host}`) { |
There was a problem hiding this comment.
I guess it's a bug?
| if (handle === `@${HOLLO_OFFICIAL_ACCOUNT}@${fedCtx.host}`) { | |
| if (username === HOLLO_RELAY_ACTOR_USERNAME) { |
I started two test relay servers on my server.
And used a test mastodon instance. And sometimes I used some public relay that don't require approve. |
| return c.json({ error: "invalid_redirect_uri" }, 400); | ||
| } | ||
| const accountOwners = await db.query.accountOwners.findMany({ | ||
| where: not(eq(accountOwnersTable.id, HOLLO_RELAY_ACTOR_ID)), |
There was a problem hiding this comment.
We'll definitely need a test case for this, but probably after #154 lands.
| relayServerActorId: uuid("relay_server_actor_id") | ||
| .$type<Uuid>() | ||
| .references(() => accounts.id, { onDelete: "cascade" }), |
There was a problem hiding this comment.
Should this be not-null?
| .$type<Uuid>() | ||
| .references(() => accounts.id, { onDelete: "cascade" }), | ||
| state: relayStateEnum("state").notNull().default("idle"), | ||
| followRequestId: text("follow_request_id").notNull().unique(), |
There was a problem hiding this comment.
Shouldn't this reference a sent follow request? Though maybe we're not storing in the database the follow requests we've sent? cc @dahlia
There was a problem hiding this comment.
Looks like we could actually store the follow request, by doing an insert to follows for the relay actor, and having approved be null?
const followers = await db.query.follows.findMany({
where: and(eq(follows.followingId, owner.id), isNull(follows.approved)),
with: { follower: { with: { owner: true, successor: true } } },
});
There was a problem hiding this comment.
Yeah, and as the primary key of the follows table is iri, the column should be renamed to followRequestIri:
| followRequestId: text("follow_request_id").notNull().unique(), | |
| followRequestIri: text("follow_request_iri") | |
| .notNull() | |
| .unique() | |
| .references(() => follows.iri, { onDelete: "cascade" }), |
| import { exportJwk, generateCryptoKeyPair, isActor } from "@fedify/fedify"; | ||
| import { Temporal } from "@js-temporal/polyfill"; | ||
| import { count, eq, sql } from "drizzle-orm"; | ||
| import { interval, jsonb, pgTable, timestamp, uuid } from "drizzle-orm/pg-core"; |
There was a problem hiding this comment.
You've a few unused variables here.
|
|
||
| accounts.get("/", async (c) => { | ||
| const owners = await db.query.accountOwners.findMany({ | ||
| where: ne(accountOwners.handle, HOLLO_RELAY_ACTOR_USERNAME), |
There was a problem hiding this comment.
I think this check should be using the ID here, not the username.
|
|
||
| accounts.get("/", async (c) => { | ||
| const owners = await db.query.accountOwners.findMany({ | ||
| where: ne(accountOwners.handle, HOLLO_RELAY_ACTOR_USERNAME), |
There was a problem hiding this comment.
This may also affect the setup page, which checks the number of actors on the server.
| return c.redirect("/federation?error=refresh"); | ||
| }); | ||
|
|
||
| data.post("/relay", async (c) => { |
There was a problem hiding this comment.
I'd be inclined to move this to a dedicated relays page, as this file already is quite large.
| const form = await c.req.formData(); | ||
|
|
||
| const inboxUrl = form.get("inbox_url"); |
There was a problem hiding this comment.
Missing form validation.
| const existingRelay = await tx.query.relays.findFirst({ | ||
| where: eq(relays.inboxUrl, inboxUrl), | ||
| }); | ||
|
|
||
| if (existingRelay) { | ||
| throw tx.rollback(); | ||
| } |
There was a problem hiding this comment.
You could do this first, and then not need to bail on the insert.
| // Create follow request for this relay | ||
| const followRequestId = new URL( | ||
| `#relay-follows/${crypto.randomUUID()}`, | ||
| account[0].iri, | ||
| ); |
There was a problem hiding this comment.
Should this actually be a real Object ID, and not using the hash fragment?
There was a problem hiding this comment.
Perhaps we could use followAccount instead? Since that'll send the follow request and store it in the database.
|
Thanks for the review! It will take some time to address all, because time is rare at the moment for me. |
|
I'm going to mark this as a draft due to pending changes and inactivity |
In this PR I try to add mastodon style relays conforming to FEP-ae0c.
Todos
@relay@relay.example.com, but this doesn't work for FediBuzz. Sadly the spec isn't clear how to get the relay server actor from the inbox url. Most relay softwares simply have@relay@relay.example.com, but not softwares with custom inboxes for different feeds. Do we actually need the relay server actor? I may only added this, to fill out the id property ofRecipientwhen callingsendActivity. Maybe the inbox url is enough?Activity.tois required to be an array in Yukimochidisabledandenabledstate for relays to temporary disable a relay. Mastodon has this feature.https://www.w3.org/ns/activitystreams#Publictoas:public