Account deactivation #2531
Account deactivation #2531
Conversation
| actorIsNoHosted(did: string, state: HydrationState): boolean { | ||
| return ( | ||
| this.actorIsDeactivated(did, state) || this.actorIsTakendown(did, state) | ||
| ) | ||
| } |
There was a problem hiding this comment.
I saw it defined elsewhere to take into account suspensions. Do we need to do that here? From packages/bsky/src/hydration/actor.ts:
const isNoHosted =
actor.takenDown ||
['takendown', 'suspended', 'deactivated'].includes(
actor.upstreamStatus ?? '',
)There was a problem hiding this comment.
Ohp nevermind, I see it!
| "status": { | ||
| "type": "string", | ||
| "description": "Hosting status of the account. If not specified, then assume 'active'.", | ||
| "knownValues": ["active", "takendown", "suspended", "deactivated"] |
There was a problem hiding this comment.
How do we feel about the fact that active may appear here but is not a hosting status under com.atproto.sync?
I think I like the convention that a status indicates a reason for a no-host, so if you see a value you don't understand you can at least assume it still indicates no-host. One result of that is that the logic around this field could be const noHost = !!status && status !== 'active', which is pretty nice. Would it be even nicer if it were just const noHost = !!status? I notice in a fair amount of logic in this PR we check explicitly for the known values to determine a no-host.
There was a problem hiding this comment.
ended up updating this to match the sematnics of getRepoStatus with the active boolean
| @@ -18,7 +18,7 @@ export default function (server: Server, ctx: AppContext) { | |||
| calcKey: ({ auth }) => auth.credentials.did, | |||
| }, | |||
| ], | |||
| auth: ctx.authVerifier.accessCheckTakedown, | |||
| auth: ctx.authVerifier.accessFull({ checkTakedown: true }), | |||
There was a problem hiding this comment.
I saw other accessCheckTakedown become accessStandard({ checkTakedown: true }), double-checking that this one differs intentionally.
There was a problem hiding this comment.
yeah i'm not really sure this matters because ultimately you need a password to delete an account. But this request should really only be coming from a fully-permissioned session. I think we didn't the full check earlier because we didn't have an authVerifier helper to make it possible
| server.com.atproto.repo.uploadBlob({ | ||
| auth: ctx.authVerifier.accessCheckTakedown, | ||
| auth: ctx.authVerifier.accessStandard({ | ||
| checkTakedown: true, | ||
| }), |
There was a problem hiding this comment.
No biggie either way, but I imagine we could do a deactivated step too since this is part of a write flow.
There was a problem hiding this comment.
Yeah I originally did that, but uploadBlob is used in the account migration flow (during which the account is deactivated). Since uploadBlob doesn't actually produce firehose events, I figured it was okay to let it through
May be worth revisiting if we want an account that is in the process of importing their repo to actually be "deactivated"
Adds facilities for account deactivation to the PDS, appview & Ozone
statuson session routesgetProfileon a deactivated account