Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
4 changes: 2 additions & 2 deletions features/account.feature
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ Feature: Account API

Scenario: Get non-existent account
When an authenticated "get" request is made to "/.ghost/activitypub/account/@nonexistent@fake-external-activitypub"
Then the request is rejected with a 500
Copy link
Collaborator

@allouis allouis Apr 14, 2025

Choose a reason for hiding this comment

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

😬 Yeah I think we should never have an explicit 500 check here.

I think we should update that stepdef to throw an error if a 5xx error is passed as the expected status code - what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeh I don't think it makes sense to expect an internal server error 😅 I'll add a check into the stepdef 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in 0a05e1b

Then the request is rejected with a 404

Scenario: Get account without authentication
When an unauthenticated request is made to "/.ghost/activitypub/account/me"
Then the request is rejected with a 403
Then the request is rejected with a 403
4 changes: 4 additions & 0 deletions features/step_definitions/stepdefs.js
Original file line number Diff line number Diff line change
Expand Up @@ -1482,6 +1482,10 @@ Then('the request is rejected', function () {
});

Then('the request is rejected with a {int}', function (statusCode) {
assert(
statusCode < 500,
`Expected to check for a client error, got a server error ${statusCode}`,
);
assert(!this.response.ok);
assert.equal(this.response.status, statusCode);
});
Expand Down
6 changes: 3 additions & 3 deletions src/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ import {
handleWebhookSiteChanged,
} from './http/api';
import { AccountFollowsView } from './http/api/views/account.follows.view';
import { AccountView } from './http/api/views/account.view';
import { spanWrapper } from './instrumentation';
import { KnexKvStore } from './knex.kvstore';
import { scopeKvStore } from './kv-helpers';
Expand Down Expand Up @@ -267,6 +268,7 @@ const postService = new PostService(
fedifyContextFactory,
);

const accountView = new AccountView(client, fedifyContextFactory);
const accountFollowsView = new AccountFollowsView(client, fedifyContextFactory);
const siteService = new SiteService(client, accountService, {
getSiteSettings: getSiteSettings,
Expand Down Expand Up @@ -959,9 +961,7 @@ app.get(
app.get(
'/.ghost/activitypub/account/:handle',
requireRole(GhostRole.Owner, GhostRole.Administrator),
spanWrapper(
createGetAccountHandler(accountService, accountRepository, fedify),
),
spanWrapper(createGetAccountHandler(accountView, accountRepository)),
);
app.get(
'/.ghost/activitypub/posts/:handle',
Expand Down
80 changes: 26 additions & 54 deletions src/http/api/account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,12 @@ import type { AppContext, ContextData } from 'app';
import { isHandle } from 'helpers/activitypub/actor';
import { lookupAPIdByHandle } from 'lookup-helpers';
import type { GetProfileDataResult, PostService } from 'post/post.service';
import {
getAccountDTOByHandle,
getAccountDTOFromAccount,
} from './helpers/account';
import type { AccountDTO } from './types';
import type {
AccountFollows,
AccountFollowsView,
} from './views/account.follows.view';
import type { AccountView } from './views/account.view';

/**
* Default number of posts to return in a profile
Expand All @@ -27,78 +24,53 @@ const DEFAULT_POSTS_LIMIT = 20;
*/
const MAX_POSTS_LIMIT = 100;

/**
* Keyword to indicate a request is for the current user
*/
const CURRENT_USER_KEYWORD = 'me';

/**
* Create a handler to handle a request for an account
*
* @param accountService Account service instance
*/
export function createGetAccountHandler(
accountService: AccountService,
accountView: AccountView,
accountRepository: KnexAccountRepository,
fedify: Federation<ContextData>,
) {
/**
* Handle a request for an account
*
* @param ctx App context
*/
return async function handleGetAccount(ctx: AppContext) {
const logger = ctx.get('logger');
const site = ctx.get('site');
let account: Account | null = null;
const db = ctx.get('db');
const handle = ctx.req.param('handle');

const apCtx = fedify.createContext(ctx.req.raw as Request, {
db,
globaldb: ctx.get('globaldb'),
logger,
});
if (handle !== CURRENT_USER_KEYWORD && !isHandle(handle)) {
return new Response(null, { status: 404 });
}

const defaultAccount = await accountRepository.getBySite(
const siteDefaultAccount = await accountRepository.getBySite(
ctx.get('site'),
);

const handle = ctx.req.param('handle');
// We are using the keyword 'me', if we want to get the account of teh current user
if (handle === 'me') {
account = defaultAccount;
} else {
if (!isHandle(handle)) {
return new Response(null, { status: 404 });
}

const apId = await lookupAPIdByHandle(apCtx, handle);
if (apId) {
account = await accountRepository.getByApId(new URL(apId));
}
}
let accountDto: AccountDTO | null = null;

let accountDto: AccountDTO;
const viewContext = {
requestUserAccount: siteDefaultAccount,
};

try {
//If we found the account in our db and it's an internal account, do an internal lookup
if (account?.isInternal) {
accountDto = await getAccountDTOFromAccount(
account,
defaultAccount,
accountService,
);
} else {
//Otherwise, do a remote lookup to fetch the updated data
accountDto = await getAccountDTOByHandle(
handle,
apCtx,
site,
accountService,
);
}
} catch (error) {
logger.error('Error getting account: {error}', { error });
if (handle === CURRENT_USER_KEYWORD) {
accountDto = await accountView.viewById(
siteDefaultAccount.id!,
viewContext,
);
} else {
accountDto = await accountView.viewByHandle(handle, viewContext);
}

return new Response(null, { status: 500 });
if (accountDto === null) {
return new Response(null, { status: 404 });
}

// Return response
return new Response(JSON.stringify(accountDto), {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't check for the accountDto being null here - I think we wanna 404 in that case probs

Copy link
Member Author

Choose a reason for hiding this comment

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

updated in e67df95

headers: {
'Content-Type': 'application/json',
Expand Down
Loading