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

Allow remembering password #26

Merged

Conversation

beatles1
Copy link
Contributor

@beatles1 beatles1 commented Jan 23, 2024

Fair warning: I've never worked with any modern frameworks or really written much code recently so it's entirely possible I've done something the wrong way. I just think this is a great project and I wanted to try and contribute. If this is no good I won't be offended!

This change adds a "Remember password" option to the password screen which writes the password to localStorage and loads it again when the page is refreshed. It would likely help with #16 as well as generally being useful.

In my mind this is an acceptable level of security as the password is only saved locally on the device and accessible by the same origin however it is in plaintext in localStorage. Presumably the password is also stored in the mobile apps in a similar way.

If it's unacceptable I could see an improvement being the server storing a unique key for the user which it sends and is used to encrypt/decrypt the password. That would mean that if not logged into Nextcloud or otherwise offline the password would be inaccessible. This seemed slightly beyond my initial attempt (working out DB migrations etc) but maybe is required?

Another option would be just saving in sessionStorage rather than localStorage.

@matteo-convertino
Copy link
Owner

I like the implementation with localstorage. Even if it is clear, we must take into account that Nextcloud authentication is present in the first place anyway. If any hacker manages to get to the extension there is probably another type of problem. I also really like the fact that if you log out of Nextcloud, the localStorage is also cleaned.

However the only thing I did was fix the syntax a little but otherwise it was perfect. Thank you so much for contributing, I hope for more PR too.

@matteo-convertino matteo-convertino merged commit 9347650 into matteo-convertino:github Jan 26, 2024
@beatles1 beatles1 deleted the feature-save-password branch February 5, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants