-
Notifications
You must be signed in to change notification settings - Fork 55
Recover WIP login stashes #687
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -182,7 +182,13 @@ export function applyLoginPayload( | |
| stashTree, | ||
| stash => stash.appId === loginReply.appId, | ||
| stash => applyLoginPayloadInner(stash, loginKey, loginReply), | ||
| (stash, children) => ({ ...stash, children }) | ||
| (stash, children) => ({ | ||
| ...stash, | ||
| children, | ||
| // If we hear back from the server, that is authoritative, | ||
| // so we can discard our local work-in-progress change: | ||
| wipChange: undefined | ||
| }) | ||
| ) | ||
| } | ||
|
|
||
|
|
@@ -417,26 +423,40 @@ export async function applyKit( | |
| const { stashTree } = getStashById(ai, kit.loginId) | ||
| const { deviceDescription } = ai.props.state.login | ||
|
|
||
| // Don't make server-side changes if the server path is faked: | ||
| if (serverPath !== '') { | ||
| const childKey = decryptChildKey(stashTree, sessionKey, kit.loginId) | ||
| const request = makeAuthJson(stashTree, childKey) | ||
| if (deviceDescription != null) request.deviceDescription = deviceDescription | ||
| request.data = kit.server | ||
| await loginFetch(ai, serverMethod, serverPath, request) | ||
| } | ||
|
|
||
| const newStashTree = updateTree<LoginStash, LoginStash>( | ||
| stashTree, | ||
| stash => verifyData(stash.loginId, kit.loginId), | ||
| stash => ({ | ||
| ...stash, | ||
| ...kit.stash, | ||
| children: softCat(stash.children, kit.stash.children), | ||
| keyBoxes: softCat(stash.keyBoxes, kit.stash.keyBoxes) | ||
| keyBoxes: softCat(stash.keyBoxes, kit.stash.keyBoxes), | ||
| wipChange: undefined | ||
| }), | ||
| (stash, children) => ({ ...stash, children }) | ||
| (stash, children) => ({ ...stash, children, wipChange: undefined }) | ||
| ) | ||
|
|
||
| // Save the WIP change to disk: | ||
| if (serverPath !== '') { | ||
| stashTree.wipChange = newStashTree | ||
| await saveStash(ai, stashTree) | ||
| } | ||
|
|
||
| // Don't make server-side changes if the server path is faked: | ||
| if (serverPath !== '') { | ||
| const childKey = decryptChildKey(stashTree, sessionKey, kit.loginId) | ||
| const request = makeAuthJson(stashTree, childKey) | ||
| if (deviceDescription != null) request.deviceDescription = deviceDescription | ||
| request.data = kit.server | ||
| try { | ||
| await loginFetch(ai, serverMethod, serverPath, request) | ||
| } catch (error) { | ||
| // If we fail, try to sync to see if the server got it: | ||
| await syncLogin(ai, sessionKey).catch(() => {}) | ||
| throw error | ||
| } | ||
| } | ||
swansontec marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| await saveStash(ai, newStashTree) | ||
|
|
||
| return newStashTree | ||
|
|
@@ -586,3 +606,36 @@ export function makeAuthJson( | |
| } | ||
| throw new Error('No server authentication methods available') | ||
| } | ||
|
|
||
| export async function healLogins(ai: ApiInput): Promise<void> { | ||
| const { stashes } = ai.props.state.login | ||
| for (const stashTree of stashes) { | ||
| if (stashTree.wipChange == null) continue | ||
|
|
||
| const { syncToken } = stashTree | ||
| let isSyncedWithServer = false | ||
| if (syncToken != null) { | ||
| try { | ||
| const reply = await loginFetch(ai, 'POST', '/v2/sync', { | ||
| loginId: stashTree.loginId, | ||
| syncToken | ||
| }) | ||
| // true means we are current (our syncToken matches the server) | ||
| // false means we are stale (server has changes we don't have) | ||
| isSyncedWithServer = asBoolean(reply) | ||
| } catch (error) { | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| if (isSyncedWithServer) { | ||
| // Server never got our change, so apply wipChange locally: | ||
| const newStash = { ...stashTree.wipChange, wipChange: undefined } | ||
| await saveStash(ai, newStash) | ||
| } else { | ||
| // Server has changes (possibly our wipChange), discard local wipChange: | ||
| const newStash = { ...stashTree, wipChange: undefined } | ||
| await saveStash(ai, newStash) | ||
| } | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Inverted logic in WIP stash recovery conditionThe
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed this issue in a fixup! commit |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wrong, but it was there to figure out why it wasn't working during sanity test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@swansontec Should I keep this or should I go with another approach to solve the problem where pin-login will no longer work on the light-account when there is a wipChange and that the login server has?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you are asking. How is this line wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have said "I don't know if this is right". I don't know if it's wrong, but it solved the issue with pin-login, but I wasn't sure if this is the correct solution. I felt uncomfortable always assuming
wipChangeis the correct stash to use.Now that I think about why I felt uncomfortable, there is a case where pin-login will break if the login-server doesn't have the changes that wipChange has. In this case, pin-login will break because
wipChangeis the wrong stash according to the login server. So perhaps the correct solution is to pickwipChangedepending on if the login-server has the changes or not. This would require that we query the login-server to determine this and then drill this information down to the selector.