Skip to content
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

LastLoggedIn still broken 😮‍💨 #2588

Closed
taoeffect opened this issue Feb 3, 2025 · 4 comments · Fixed by #2633
Closed

LastLoggedIn still broken 😮‍💨 #2588

taoeffect opened this issue Feb 3, 2025 · 4 comments · Fixed by #2633
Assignees
Labels
App:Frontend Kind:Bug Kind:Core Anything that changes or affects the fundamental core data structures & design of the application. Priority:High

Comments

@taoeffect
Copy link
Member

taoeffect commented Feb 3, 2025

Problem

Image

The uCHELONIA_STATE:

Image

The other state:

Image

One other potential problem is that we use new Date() to get this value:

    const now = new Date().toISOString()

The user's clock could be wrong.

Solution

  1. Figure out why it's not being updated correctly and fix it.
  2. Use new Date(sbp('chelonia/time')) instead of naked new Date()
@taoeffect taoeffect added App:Frontend Kind:Bug Kind:Core Anything that changes or affects the fundamental core data structures & design of the application. Priority:High labels Feb 3, 2025
@taoeffect
Copy link
Member Author

Image

Notice the Error updating lastLoggedIn Error: kv/set invalid response status: 401

And there are Invalid signature time range messages on the server, so maybe this is the cause?

@taoeffect
Copy link
Member Author

taoeffect commented Feb 7, 2025

We've narrowed this down to likely being related to ?.data:

return (await sbp('chelonia/kv/get', contractID, KV_KEYS.LAST_LOGGED_IN))?.data || Object.create(null)

And related code, e.g.:

    get data () {
      if (!encryptedData) return
      if (isSignedData(encryptedData.data)) {
        return encryptedData.data.valueOf()
      }
      return encryptedData.data
    },

Note there's the same issue in identity-kv.js:

return (await sbp('chelonia/kv/get', identityContractID, KV_KEYS.UNREAD_MESSAGES))?.data || {}

And who knows, maybe that's related to fixing #2442 too?

corrideat added a commit that referenced this issue Feb 7, 2025
Changes to `kv/get` to propagate decryption errors (related to #2588).

Closes #2589.
taoeffect pushed a commit that referenced this issue Feb 7, 2025
* Add `wsOnly` parameter to avoid broadcasting KV updates.

Changes to `kv/get` to propagate decryption errors (related to #2588).

Closes #2589.

* Feedback
@corrideat
Copy link
Member

I've been investigating this issue, and, while I can see on groupincome.app that it's apparently happening, the mechanism for it seems unclear.

The update happens by calling 'gi.actions/group/kv/updateLastLoggedIn', which has the following logic:

      const getUpdatedLastLoggedIn = async (contractID) => {
        const current = await sbp('gi.actions/group/kv/fetchLastLoggedIn', { contractID })
        return { ...current, [identityContractID]: nowString }
      }

      const data = await getUpdatedLastLoggedIn(contractID)
      await sbp('chelonia/kv/set', contractID, KV_KEYS.LAST_LOGGED_IN, data, {
        encryptionKeyId: await sbp('chelonia/contract/currentKeyIdByName', contractID, 'cek'),
        signingKeyId: await sbp('chelonia/contract/currentKeyIdByName', contractID, 'csk'),
        onconflict: getUpdatedLastLoggedIn
      })

This is supposed to be append-only, so no entries can be lost. There is an exception. If we look at: gi.actions/group/kv/fetchLastLoggedIn, we have:

  'gi.actions/group/kv/fetchLastLoggedIn': async ({ contractID }: { contractID: string }) => {
    const kvData = await sbp('chelonia/kv/get', contractID, KV_KEYS.LAST_LOGGED_IN)
    // kvData could be falsy if the server returns 404
    if (kvData) {
      // Note: this could throw an exception if there's a signature or decryption
      // issue
      return kvData.data
    }
    return Object.create(null)
  },

Note that fetchLastLoggedIn could return an empty object if kvData is falsy, or, if kvData.data is an empty object or falsy.

Before, it was possible for kvData.data to end up being falsy if there was a decryption error. This seemed like the right course of action at the time, but, to work on this issue, #2592 introduced changes that will treat decryption errors as errors. So, this means that either kvData.data can be decrypted and is falsy / an empty object, or the server returns a 404.

I've tried to reproduce this issue locally, and decryption (or signature) errors do indeed result in an exception being thrown (which effectively would stop updates to lastLoggedIn rather than overwriting it).

@taoeffect
Copy link
Member Author

taoeffect commented Feb 17, 2025

We've figured out that #2633 is in fact the fix for this issue due to connectionURL being null because Chelonia code is run before it's initialized, resulting in a 404 when the key is fetched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App:Frontend Kind:Bug Kind:Core Anything that changes or affects the fundamental core data structures & design of the application. Priority:High
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants