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

Multi-user support #172

Merged
merged 11 commits into from
Jun 27, 2020
Merged

Conversation

ben-kaufman
Copy link
Contributor

Allow multiple users to use the same Specter server.
For now, each user is separated from the rest.
The original user (the admin) can send registration links with OTP for others to register.
The settings page and configuration was split to admin managed settings like auth and user-specific ones like hwi_bridge_url.

@stepansnigirev
Copy link
Contributor

When I change in settings to Multiple Users I get an error:
AttributeError: 'AnonymousUserMixin' object has no attribute 'is_admin'

Where should I set up the initial admin user?

@ben-kaufman
Copy link
Contributor Author

Strange, I can't reproduce it... Though I think I saw a similar error and fixed it with the last commit (0f73225), are you using it?

Right now for simplicity and compatibility with the current RPC password as pin login option, I made the admin be the username admin and password as the RPC password. I didn't write it there because I think I should change it, but I'm not yet sure what to change it with, maybe it's good enough like that...

@stepansnigirev
Copy link
Contributor

That might be a problem - I am using cookies in Core, so there is no static username and password in my case.

@stepansnigirev
Copy link
Contributor

Maybe if multi user option is selected we could show two more inputs in the settings - admin username and password

@ben-kaufman
Copy link
Contributor Author

Makes sense. I added now in the settings that any user can change its username and password, and noted that for the admin the default is admin for both.

Copy link
Contributor

@stepansnigirev stepansnigirev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should set the user to admin if login is disabled, because otherwise current_user.is_admin doesn't work

src/cryptoadvance/specter/controller.py Show resolved Hide resolved
src/cryptoadvance/specter/controller.py Show resolved Hide resolved
@ben-kaufman
Copy link
Contributor Author

I already did that (here and here) but just noticed I missed it in one case, added it now too so it should be fine.

@stepansnigirev
Copy link
Contributor

I was able to get to the multi-user when I first switched from None to rpcpassword auth mechanism, and then to multiuser.

From None to multiuser still doesn't work.

How to reproduce:

  • Set auth to None
  • Enter incognito mode
  • Try changing auth to multiuser

@stepansnigirev
Copy link
Contributor

stepansnigirev commented Jun 26, 2020

Now works. Just tried registering a new user - works fine!

I see that the user has full access to devices that he doesn't own, in particular he can delete the device, keys of the device, or even add new keys to the device of another user.

I think we should at least disable the editing capabilities of the devices that the user doesn't own.
The next step would be to configure what keys are visible to other users.

Maybe we can also separate the list of devices to devices that we own and devices of other users that we have access to.

@ben-kaufman
Copy link
Contributor Author

That’s weird, each users should be able to access only his own devices, that’s how it is for me, so it’s probably a bug, I’ll check why it’s happening.

Later I want to add sharing and visibility to devices and wallets, but that’s more complicated.

@stepansnigirev
Copy link
Contributor

When I am creating a device I don't see any ownership fields in the device json file, so I am not sure how you determine who owns the device...

@ben-kaufman
Copy link
Contributor Author

I create a separate devices folder for each user. This is why I’m surprised you see one users devices when logged to another.
When adding sharing I will likely need to give up the separation by folders, but for a first stage doing so made it easiest.

Comment on lines 303 to 305
salt = hashlib.sha256(os.urandom(60)).hexdigest().encode('ascii')
pwdhash = hashlib.pbkdf2_hmac('sha512', password.encode('utf-8'),
salt, 10000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest making a few changes here:

  • store salt and password hash separately to avoid things like stored_password[64:], so password key would contain salt and pwdhash keys.
  • use pbkdf2_hmac('sha256',...) to make resulting hash x2 shorter, I don't see security benefits of sha512 here.
  • store bytes not as hex digits but use base64 encoding (binascii.b2a_base64 and binascii.a2b_base64) - again, shorter.

One note about b2a_base64 function - it adds \n at the end of the string for some reason, but it can be removed using .strip() on it. It doesn't influence the result of a2b_base64 but I am not sure how json will react to \n in the string

Comment on lines 42 to 45
{% if specter.config['auth'] == 'usernamepassword' %}
Specter Username:<br><input type="text" name="specter_username" type="text" value="{{ current_user.username }}"><br><br>
Specter Password:<br><input type="password" name="specter_password" type="text" placeholder="Set new password"><br>
<br><br>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add this part dynamically with javascript when the authentication method is changed.
UX will be more intuitive in this case - the user selects usernamepassword setting and can set up admin login and password right away.
When authentication method is changed to rpcpasswordaspin or none - we remove this part of the form.

@ben-kaufman
Copy link
Contributor Author

Updated based on both comments :)

@stepansnigirev stepansnigirev merged commit 6b0cab4 into cryptoadvance:master Jun 27, 2020
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