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

Schema overhaul #91

Merged
merged 12 commits into from
Mar 23, 2021
Merged

Schema overhaul #91

merged 12 commits into from
Mar 23, 2021

Conversation

cryptix
Copy link
Member

@cryptix cryptix commented Mar 19, 2021

This is needed for #90 but because there is so much renaming, I made a separate PR.

  • create consolidated schema with new members table
  • remove allow_list and replace it with members
  • update roomdb types
  • rename User to Member
  • add role enums (member, moderator, admin)
  • update insert-user
  • overhaul web/handlers/admin
  • overhaul web/handlers
  • overhaul roomsrv and tests
  • copy previous allow-list stuff and use it as deny-list
  • fix add-form layout on denied list (key|comment)
  • fix add-form layout on members list (pub_key|nick)
  • role change ui
  • pick icon for denied keys menu entry (it's https://materialdesignicons.com block-helper)

@cryptix
Copy link
Member Author

cryptix commented Mar 19, 2021

Just some thoughts as I'm jumping through these changes. Still stuff to do but thought I'd loop you in already.

Following from "allow lists is now members" comes the realization that members need to have a nickname. Assuming that no-one wants to deal with just public keys and they might not have an alias. So the current allow list form needs another input field (and styling for that... cc @staltz 😬) and a way to change the members role (not sure if we want to make this with a drop-down on the list or a separate edit page (which might be worthwhile if people want to change their nick or public key for instance).

@cryptix cryptix added the Go golang related stuff label Mar 19, 2021
@staltz
Copy link
Member

staltz commented Mar 19, 2021

Oh, yes it would make sense that the members page allows changing roles. A dropdown seems fine, to begin with. Just note that only admin should be able to change roles.

But nicknames? Why? Aliases are basically the nickname system.

@staltz
Copy link
Member

staltz commented Mar 19, 2021

Assuming that no-one wants to deal with just public keys and they might not have an alias.

Is this the reason? And in what context would they deal with public keys? Not sign-in, because that should be handled by SIWSSB (we don't want the fallback username/password system for all members. In fact we didn't even want it originally for anyone)

@cryptix
Copy link
Member Author

cryptix commented Mar 19, 2021

Is this the reason? And in what context would they deal with public keys?

if the members don't have a nickname, you just see the public key on who created the invite or who do these aliases belong to.

Aliases are basically the nickname system.

Not all users might have an alias. I might not want all my devices identified with an alias, especially since it's enumerable from the public, unless I switch the room into restricted mode, which might not be what the other room members want.

edit: this members nickname is also different from the fallback password (where the "username" is called login). This is another place where entries are linked to the members and where changing or no aliases can be confusing.

@cryptix
Copy link
Member Author

cryptix commented Mar 19, 2021

By the way: I don't think we need to spec this. It just makes sense for an implementation, I think. Simply starting from the case "what happens when you remove your only active alias" or "what do you show when there are two aliases?".

@staltz
Copy link
Member

staltz commented Mar 19, 2021

By the way: I don't think we need to spec this. It just makes sense for an implementation, I think.

That's good clarification. Thanks

if the members don't have a nickname, you just see the public key on who created the invite or who do these aliases belong to.

I see this as a possibility, but not a need, strictly. Seeing the raw public key is not pressing problem, as I see it. There are also considerations to make, such as minimizing how much the admin knows about its members (see some Security Considerations in the spec), and it's also possible to create confusion by having both nickname and alias (people might register one thinking that they wanted the other, or vice-versa). Attaching identifiable metadata to members should also ideally by signed by the member, so that's why I compared them to aliases.

@staltz
Copy link
Member

staltz commented Mar 19, 2021

Other thoughts about this

So the current allow list form needs another input field

I think it would make sense to merge the aliases and members list pages into one, and add role dropdown too. It would need some redesign, indeed. We might even need a "member details" page that lists all the details of that member and allows editing (e.g. revoke alias).

@cryptix
Copy link
Member Author

cryptix commented Mar 22, 2021

confusion by having both nickname and alias

I'd argue the welcome strings we have act like help/descriptions on the page and should do a good job there.

minimizing how much the admin knows about its members

Honestly, I don't see how moderators can effectively govern a room if they don't know who is who.

Additionally, this feels like invite consumption should then be grouped with alias creation. Last time I asked for this you were against it, claimed people might not want an invite... 🤷

Attaching identifiable metadata to members should also ideally by signed by the member

Do you want to do this now? I'd rather put this on v2.2 or later since it would need considerable re-work and generalization.

Another approach to storing these on the room could be to hook into a configured peer and query it for type:about messages of the member, to shortcut this need for more signed off-chain redundancy.

I think it would make sense to merge the aliases and members list pages into one.

This sounds reasonable but can we do it as a follow-up? There is a lot of needed changes in here.

@cryptix
Copy link
Member Author

cryptix commented Mar 22, 2021

we had a call and among other things decided to: later merge aliases with members managment/editing. We don't want to have a 2nd database of the social data that is already in ssb (type:about showing maybe in future versions) at which point nicknames might become obsolete. Generally we agreed that we shouldn't try to guess what our users might and might not want.

cryptix added 11 commits March 22, 2021 12:58
* create consolidated schema with new members table
* update interfaces
* remove allow_list and replace it with members
* update roomdb types
* rename User to Member
* add role enums
* update insert-user
copy of previous allow-list stuff
* implement SetRole on sqlite
* add dropdown form to members table
* add http endpoint for processing
* Add comment to denied keys overview
* update ban remove confirm page
@cryptix cryptix changed the base branch from sign-in-with-ssb to master March 22, 2021 12:09
@cryptix cryptix marked this pull request as ready for review March 22, 2021 12:11
@cryptix cryptix mentioned this pull request Mar 22, 2021
8 tasks
Copy link
Member

@staltz staltz left a comment

Choose a reason for hiding this comment

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

I took a quick look at all files and this seems good. :)

@cryptix
Copy link
Member Author

cryptix commented Mar 23, 2021

Also did a high-level walk through with cblgh. Merrrging now!

@cryptix cryptix merged commit 489926d into master Mar 23, 2021
@cryptix cryptix deleted the schema-overhaul branch March 23, 2021 07:08
cryptix added a commit that referenced this pull request Mar 23, 2021
@cryptix cryptix mentioned this pull request Mar 29, 2021
7 tasks
@cryptix cryptix mentioned this pull request Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Go golang related stuff
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants