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

Trying to connect with the wrong atKeys results in a lot of errors #900

Open
cconstab opened this issue Jan 29, 2023 · 19 comments · May be fixed by #1033
Open

Trying to connect with the wrong atKeys results in a lot of errors #900

cconstab opened this issue Jan 29, 2023 · 19 comments · May be fixed by #1033
Assignees
Labels
enhancement New feature or request PR56 PR57

Comments

@cconstab
Copy link
Member

Is your feature request related to a problem? Please describe.

When an atSign has been "reset" and new keys are cut by a client, old clients break as they are using the wrong keys. This is particulary acute with atclients that send notifications first (at_talk as an example).

Describe the solution you'd like

atclient should delete entire local datastore if it detects that the public key on the server is not the same as the public key in its local datastore

Describe alternatives you've considered

No response

Additional context

No response

@gkc
Copy link
Contributor

gkc commented Feb 20, 2023

Un-assigning myself as I am already at capacity in PR56. However, this is important, so it needs another assignee for PR56.

@VJag
Copy link
Member

VJag commented Feb 20, 2023

I will work with @purnimavenkatasubbu and @srieteja on this

@purnimavenkatasubbu
Copy link
Member

The "reset" option is currently disabled in the staging and production dashboards.In order to debug the bug, We need the reset atsign option

@gkc
Copy link
Contributor

gkc commented Feb 28, 2023

You can test locally : tomorrow afternoon I will commit some scripts I use for testing which you can use to test reset scenarios

in the meanwhile - we should enable reset in staging - @athandle @naresh0689 can you do that please? (But it must remain disabled in prod for now)

@gkc
Copy link
Contributor

gkc commented Mar 1, 2023

Hi @purnimavenkatasubbu - see atsign-foundation/at_server#1231 for some scripts I use to run things locally

@gkc
Copy link
Contributor

gkc commented Mar 1, 2023

@athandle @naresh0689 Can you re-enable reset in staging please? (But it must remain disabled in prod for now)

@purnimavenkatasubbu
Copy link
Member

Hi @purnimavenkatasubbu - see atsign-foundation/at_server#1231 for some scripts I use to run things locally

Sure. I'll check

@athandle
Copy link

athandle commented Mar 1, 2023

@athandle @naresh0689 Can you re-enable reset in staging please? (But it must remain disabled in prod for now)

I have a build to deploy after that I will move. If any thing urgent @gkc then I gave reset in admin, I can give demo to you

@purnimavenkatasubbu
Copy link
Member

Hi @purnimavenkatasubbu - see atsign-foundation/at_server#1231 for some scripts I use to run things locally

Sure. I'll check

I see that this scripts are for macOS. Mine is Linux

@gkc
Copy link
Contributor

gkc commented Mar 2, 2023

@purnimavenkatasubbu Indeed. However, they are bash scripts, with nothing OS-specific in them, so they should work on Linux

@purnimavenkatasubbu
Copy link
Member

Logs after testing locally.
ErrorLogs.txt

@srieteja
Copy link
Contributor

@VJag and I are brainstorming an approach for this. Moving to the next sprint for implementation

@srieteja
Copy link
Contributor

srieteja commented Apr 3, 2023

Had to put the implementation on hold due to some unplanned work. Carrying this over to the next sprint.

@srieteja
Copy link
Contributor

srieteja commented Apr 10, 2023

My intial approach:

Introduce two methods to atClient:

  • isRemoteReset(): If the public key present in the LocalSecondary and the one on the RemoteSecondary are different the method throws an exception indicating that a reset has occurred.

  • resetLocalStorage(): This method will be the after-action for isRemoteReset(). If the remote is reset, the client-consumer(agent who has control of the client instance e.g. an app) should catch the exception and take user consent for local secondary purge. If the user consents for LocalStorage purge the directories of hive, commitLog and any other local data needs to be removed. The method resetLocalStorage() is responsible for this data removal.

When initializing an atClient instance, check if the remote has been reset. Purge LocalSecondary storage if a reset has occurred. This would be preferably in the AtClientManager.setCurrentAtsign() as this is currently used to instantiate an AtClient for an atsign.

reset_loc_sec (1)

@srieteja
Copy link
Contributor

srieteja commented May 2, 2023

The implementation of solution for this is https://github.com/atsign-foundation/at_client_sdk/tree/reset_local_secondary.
Currently testing this and discussing the validity of the approach with @VJag

@gkc
Copy link
Contributor

gkc commented Jul 9, 2023

Is this still being actively worked on?

@srieteja
Copy link
Contributor

srieteja commented Jul 9, 2023

I have an implementation ready in #1033. We need to discuss this in an arch call preferably. Will bring it up in the next arch call.

@srieteja
Copy link
Contributor

I was able to complete the fix and testing in PR68. The PR is being reviewed and this ticket will be closed once the PR has been merged.

@purnimavenkatasubbu
Copy link
Member

Tested the fix in the PR by the following steps

  1. Activated and authenticated an atSign through the onboarding_cli
  2. Call atClient?.deleteLocalSecondaryStorageWithConsent(
    userConsentToDeleteLocalStorage: true);
  3. Reset the AtSign from the dashboard
  4. call atClient?.isSecondaryReset()

isResetSecondary should return true as the atSign has been reset, but here it returns null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PR56 PR57
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants