Account migration #2179
Account migration #2179
Conversation
| @@ -136,6 +137,14 @@ export class SqlRepoReader extends ReadableBlockstore { | |||
| return builder.execute() | |||
| } | |||
|
|
|||
| async countBlocks(): Promise<number> { | |||
There was a problem hiding this comment.
Just a note that this could cause some blocking.
There was a problem hiding this comment.
Since it's only used for reads (and we're in WAL mode), I don't think it would block, but it might fail with a SQLITE_BUSY and have to retry
There was a problem hiding this comment.
Ah sorry didn't explain that well! I was picturing blocking the main app thread. E.g. if there's a second of work in sqlite to do here, the app thread doing the work will block for a second.
| const blob = await ctx.actorStore.transact(requester, (actorTxn) => { | ||
| return actorTxn.repo.blob.addUntetheredBlob(input.encoding, input.body) | ||
| }) | ||
| const blob = await ctx.actorStore.transact( |
There was a problem hiding this comment.
The changes in here underline for me how it might be beneficial to split-up the db work from the upload work. As I understand it, as long as this transaction remains open other writes to the same actor store will block/timeout. Adding a read into the transaction makes it a little higher stakes now, as I think that kicks sqlite into doing some additional locking.
There was a problem hiding this comment.
That's a good point, I'll come back to this so that it doesn't hold the txn open while uploading 👍
packages/pds/src/api/com/atproto/identity/submitPlcOperation.ts
Outdated
Show resolved
Hide resolved
| await ctx.actorStore.transact(did, (store) => | ||
| importRepo(store, input.body), | ||
| ) |
There was a problem hiding this comment.
I do think that in general—i.e. even if not given our implementation/policy—we can expect this to last long enough that it might time out the connection. And some implementations might want to plop it on a job queue and deal with it totally asynchronously.
Even we might consider just tossing the repo car on disk or into s3 then working with it streaming or in parts to make it robust to repos that don't happily fit into memory.
There was a problem hiding this comment.
(To be clear I think this is fine for our current purposes, mostly want to leave the door open to these possibilities.)
There was a problem hiding this comment.
yeah hmmm. is there a way that we can do both? like process it synchronously in most cases, but drop into async if it's large enough? Not sure how that would surface in schemas 🤔
| if (roots.length !== 1) { | ||
| throw new InvalidRequestError('expected one root') |
There was a problem hiding this comment.
In the future we might look into failing earlier for this case which could feasibly be detected up front, just a slight vector for abuse.
There was a problem hiding this comment.
yeah that's a good point 👍
we need a better CAR parsing lib first 😅
| throw new InvalidRequestError( | ||
| `Could not parse record at '${write.collection}/${write.rkey}'`, | ||
| ) |
There was a problem hiding this comment.
I think any errors in here will end-up unhandled and cause a crash. What we probably want to do is setup an AbortController and user p-queue's support for signals to thread it through and bail on work once there's a failure. One the diff.writes loop ends and the queue is determined idle we can call signal.throwIfAborted() to raise the original error if one occurred.
There was a problem hiding this comment.
oof whatdya know 🙃
i'll fix this up
There was a problem hiding this comment.
probably worth a quick peek: 153c90b
There was a problem hiding this comment.
Probably worth simulating to be sure, but looks good 👍
There was a problem hiding this comment.
yup i gave it a test & it worked how you'd expect 👍
| const user = await ctx.accountManager.getAccount(did) | ||
| const user = await ctx.accountManager.getAccount(did, { | ||
| includeDeactivated: true, | ||
| includeTakenDown: true, |
There was a problem hiding this comment.
I don't think we let takendown accounts to this point thanks to accessCheckTakedown.
| const account = await ctx.accountManager.getAccountByEmail(email) | ||
| const account = await ctx.accountManager.getAccountByEmail(email, { | ||
| includeDeactivated: true, | ||
| includeTakenDown: true, |
There was a problem hiding this comment.
Adding support for taken-down accounts to reset their password, update their email. Just double-checking we like that 👍
There was a problem hiding this comment.
Removed it from update email, but I do think it's import for resetting your password 👍
You may want to reset your password in order to export some of your private data or get a signed PLC operation for instance
| ) | ||
| } | ||
|
|
||
| if (!pdsEndpoint || pdsEndpoint !== ctx.cfg.service.publicUrl) { |
There was a problem hiding this comment.
If trailing slashes aren't significant, we might consider permitting the pds endpoint both with and without the tailing slash.
There was a problem hiding this comment.
(If we pursue this, similar logic also appears in identity.submitPlcOperation.)
There was a problem hiding this comment.
I'm inclined to just leave it more conservative for now, and we can make it more lax over time if we decide to. Easier to go that way than the other 🤔
packages/pds/src/auth-verifier.ts
Outdated
| try { | ||
| return await this.userDidAuth(reqCtx) | ||
| } catch { | ||
| return { credentials: null } | ||
| } |
There was a problem hiding this comment.
I'd recommend we require the auth be valid if it's present. It's conservative on the server side, but also usually friendly to the caller too, since if they included auth they probably intended to take action in the context of those credentials. They would also be aware that they were holding onto some bad credentials.
| adminService = async (reqCtx: ReqCtx): Promise<AdminServiceOutput> => { | ||
| const payload = await this.verifyServiceJwt(reqCtx, { | ||
| aud: this.dids.entryway ?? this.dids.pds, | ||
| iss: [this.dids.admin], |
There was a problem hiding this comment.
In a world where pdses have dids, I wonder if we'll eventually want service auth to include the pds as the issuer and the user as the subject. Not relevant to this PR, just wanted to preserve the thought before it slips away!
There was a problem hiding this comment.
yup I could totally see that 👍
* clean old temp lexicons * rm old test
Adds account migration flow based on schemas in #2170
Entryway impl: #2185
Supporting features for account migration:
Smuggled into this is the ability to activate/deactivate an atproto account. When deactivated, the account cannot make any writes to its repository, and reads for the repository are not served to any viewers (other than the account in question).