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

Clear PasswordVault when resetting the app #50

Closed
DecaTec opened this issue Nov 28, 2016 · 11 comments · Fixed by #55
Closed

Clear PasswordVault when resetting the app #50

DecaTec opened this issue Nov 28, 2016 · 11 comments · Fixed by #55

Comments

@DecaTec
Copy link
Collaborator

DecaTec commented Nov 28, 2016

After app reset you can log in with any password as long as server URl and user name are the same as before the reset.

When resetting the app we should call PasswordVault.Remove.

@DecaTec
Copy link
Collaborator Author

DecaTec commented Nov 28, 2016

I've seen that this is already done in the SettingsPageViewModel. I wouldn't expect it here, think this should happen in the settings class.
The problem I ran into was that after resetting, there is only a navigation to the login page. Here you can actually insert any password and are redirected to the folder overview.

So maybe it's a good idea to terminate the app after the reset (maybe with a message box telling the user that the app will close).

@SunboX
Copy link
Member

SunboX commented Nov 28, 2016

So maybe it's a good idea to terminate the app after the reset (maybe with a message box telling the user that the app will close).

No, from a user view this is a terrible behavior. Like "you must restart your PC to finish the update". 😏
I will look into this as soon as I'm getting the time to.

But a good catch! Thanks for the report! ❤️

@DecaTec
Copy link
Collaborator Author

DecaTec commented Nov 28, 2016

Maybe also a good idea to put the vault.Retrieve in a try-catch block. I had some problems while debugging. When no credentials are available, vault.Retrieve will throw an exception which prevents the following up code to be executed.

@DecaTec
Copy link
Collaborator Author

DecaTec commented Dec 1, 2016

While working on #55, I've tracked the real problem:
When the app gets reset, the user is redirected to the login page. Entering invalid credentials, the HEAD request checking the user credentials (method CheckUserLogin), will return true due to an HTTP 200 (OK) response message.
I don't have an explanation for this behavior. Even trying not to cache any data will lead to the same result.

As a possible fix in #55, I've changed the CheckUserLogin method, so we're doing an API call for the user attributes. This will always fail when the invalid credentials are specified.

@altima
Copy link
Member

altima commented Dec 3, 2016

Is the PasswordVault working? I got errors while using the last commits on the retrive function?! I already added try catch to prevent it, but the combase.dll from the emulator throws unexpected error...

I'm using the emulator 10.0.10586, any ideas?

@DecaTec
Copy link
Collaborator Author

DecaTec commented Dec 4, 2016

No problems on 10.0.10586 (with pull request #55).
What's the exact location where you're getting problems?

@altima
Copy link
Member

altima commented Dec 4, 2016

I got this problem direct after login.

@altima
Copy link
Member

altima commented Dec 5, 2016

UPDATE: there is a maybe problem in the PasswordVault with mail addresses as username.

@SunboX
Copy link
Member

SunboX commented Dec 5, 2016

PasswordVault retrieve will fail, if there's a dot "." in the username field. I will investigate this further.

@SunboX
Copy link
Member

SunboX commented Dec 20, 2016

I can't find the root cause for this bug. But I have an idea how to change the implementation a bit, so that I can work around this.

Basically, I will retrieve all credentials for a domain and iterate over them to find the one with the right username. Should work like this.

@SunboX
Copy link
Member

SunboX commented Dec 29, 2016

@altima should work now. Was fixed with #64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants