Account deletion #488
Account deletion #488
Conversation
| "encoding": "application/json", | ||
| "schema": { | ||
| "type": "object", | ||
| "required": ["handle", "password", "token"], |
There was a problem hiding this comment.
Requires re-entering password & as such does not require any auth
| @@ -29,7 +29,7 @@ export default async (sc: SeedClient, mq?: MessageQueue) => { | |||
| ) | |||
| const img2 = await sc.uploadFile( | |||
| carol, | |||
| 'tests/image/fixtures/key-portrait-small.jpg', | |||
| 'tests/image/fixtures/key-alt.jpg', | |||
There was a problem hiding this comment.
I wanted to give carol a unique file that nobody else had uploaded. This changed a couple of test snapshots as well
| "encoding": "application/json", | ||
| "schema": { | ||
| "type": "object", | ||
| "required": ["handle", "password", "token"], |
There was a problem hiding this comment.
Check out #492— should we switch handle to identifier here to mirror it?
There was a problem hiding this comment.
hmmm probably...
altho actually, maybe it makes more sense to do did. to impress on the caller that you're deleting, not just the PDS concept of the user but the repo itself as well 🤔
There was a problem hiding this comment.
btw ended up switching out for did for the above reason^^
As well, I think generally, we should only use handle as an "entry point" (login/password reset/discovery) & did for everything else.
| @@ -40,6 +41,13 @@ export class ServerMailer { | |||
| }) | |||
| } | |||
|
|
|||
| async sendAccountDelete(params: { token: string }, mailOpts: Mail.Options) { | |||
There was a problem hiding this comment.
Random thought coming to mind— we could send the user their repo over email as some part of the process.
There was a problem hiding this comment.
yeah that'd be really nifty 👍
maybe we get that in the work push around account portability
packages/pds/src/db/migrations/20230124T230923517Z-user-partitioned-cids.ts
Outdated
Show resolved
Hide resolved
packages/pds/src/db/migrations/20230118T223059239Z-repo-sync-data.ts
Outdated
Show resolved
Hide resolved
| .addPrimaryKeyConstraint(`${commitBlockTable}_pkey`, [ | ||
| 'commit', | ||
| 'block', | ||
| 'creator', | ||
| ]) | ||
| .execute() | ||
| await db.schema | ||
| .createTable(commitHistoryTable) | ||
| .addColumn('commit', 'varchar', (col) => col.primaryKey()) | ||
| .addColumn('commit', 'varchar', (col) => col.notNull()) | ||
| .addColumn('prev', 'varchar') | ||
| .addColumn('creator', 'varchar', (col) => col.notNull()) | ||
| .addPrimaryKeyConstraint(`${commitHistoryTable}_pkey`, [ | ||
| 'commit', | ||
| 'creator', | ||
| ]) |
There was a problem hiding this comment.
Similar here— if we always expect to qualify these queries with a creator and want it to be cheap to enumerate commits/history by creator, then bumping creator up to the front of the index might be an easy win.
There was a problem hiding this comment.
good looks 👍
did that in the sync-revamp PR as well: 4719922
| ) | ||
| expect(found.length).toBe(0) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Only thing I might consider adding to these tests is confirmation that other users' data is still around. Touched on it a little bit in the blob test. It might look like confirming that exactly Alice's blocks are missing from the db.
There was a problem hiding this comment.
yeah good looks
I changed my approach to it a bit & also added tests for more tables as well. Fortunately didn't come across any additional bugs from it 😛
| .select([ | ||
| 'ipld_block.cid as cid', | ||
| 'ipld_block_creator.did as creator', | ||
| 'ipld_block.size as size', | ||
| 'ipld_block.content as content', | ||
| 'ipld_block.indexedAt as indexedAt', | ||
| ]), |
There was a problem hiding this comment.
Just something to be a little careful with here— I don't think the column aliases are doing what one might expect/hope them to do. I believe all that matters is the column order. To ensure these are lining-up, might consider adding .columns(...) to the query (e.g. https://koskimas.github.io/kysely/classes/InsertQueryBuilder.html#expression).
There was a problem hiding this comment.
this is actually in regards to a separate kysely thing that bit me once.
on .returning()
Note that on SQLite you need to give aliases for the expressions to avoid this bug in SQLite. For example .returning('id as id').
altho i just kinda had it in my head that i always had to supply an alias, which it turns out you don't for select()
There was a problem hiding this comment.
but i'll go ahead & add .columns() as well to be safe 👌
| .deleteFrom('repo_commit_history') | ||
| .where('creator', '=', did) | ||
| .execute(), | ||
| this.db.db.deleteFrom('repo_root').where('did', '=', did).execute(), |
|
LGTM! |
* wip * fleshing out repo storage * fleshing out sql storage * cleaning things up * fix up tests * dumb bug - commit log reversed * rm staging in favor of commiting diffs to blockstore * clean up benches * fixing up sql storage * some caching for sql repo store * pr feedback * migration * wip * migraiton test * unclear param * sql repo storage tests * rm unused code * fix up some diff code * pr feedback * enum for action types * missed some * wip * ripping out auth lib * more auth cleanup * another lurker * wip better sync primitives * wip * improving diffs & sync * tests working! * actually implemented checkout lol * simplify interface & improve error handling * writing sql storage code * fixing up tests * testing & bugfixes * checkouts return records instead of cids * one last refactor lol * missed one * handle other cid codecs on incoming car verification * tests + tricky bugs * unneeded blockstore method * trim mst on del instead of save * cleanup comment * dont resolve did for every commit * use "commit" instead of "root" * getRoot -> getHead * pr feedback * very silly bug fix * improve sync output * reorging + sync of particular records * serve & verify proofs. also rename some ipld methods * fix up sync issue in mst * find reachable records form carfile * getRecord xrpc method * pr feedback * better migration test * check migraiton result * fixing up a couple things for pg * explicit migrateTo * async exceptions * ipld car mimetype + remove updateRepo * Update module publish scripts (bluesky-social#478) * Update pds package publishing scripts * Update auth package publishing scripts * Update crypto package publishing scripts * Update did-resolver package publishing scripts * Update handle package publishing scripts * Update xrpc-server package publishing scripts * Update common package publishing scripts * Update plc package publishing scripts * Update uri package publishing scripts * Update repo package publishing scripts * Sort "suggested follows" by number of posts (bluesky-social#477) * return suggestions by post count * pr feedback * fix up PG pagination issue * partiion commit-history & commit-blocks by user did * some lexicons reworking routes request deletion flows delete actor rows migration for user-partitioned-cids move creator to be on ipld_block migration tests * delete records & repos * delete blobs * hook it up in route * pettier ignore email templates * testing & bugfixes * testing blobs & bugfixes * pr feedback * make deletion test more robust * change out handle for did on account deletion * small cleanup --------- Co-authored-by: Paul Frazee <pfrazee@gmail.com>
Account deletion
This adds api flows for deleting your account & account data.
The flow is
Here is a preview of the email template:
Account deletion does a hard delete of a user's repository, uploaded files, and indexed records. It also immediately frees up their handle (if provided by the PDS). It does not yet do anything with PLC (no tombstones implemented there yet).
One change (involving a migration) that I implemented for this is storing the creator of an ipld_block alongside the block itself. This involves storing duplicate blocks. But shared blocks between repos will be exceedingly rare.
I did not do this for blobs, but we may be interested in doing so in some future work.
Closes #474