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 Users to Join Multiple Organizations #4984

Merged
merged 66 commits into from
Jan 11, 2021
Merged

Allow Users to Join Multiple Organizations #4984

merged 66 commits into from
Jan 11, 2021

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Dec 7, 2020

Users can now join multiple organizations, admins can now invite users by email address, skipping the manual user activation step.

TODO Backend

  • move email calls to multiuser
  • only superusers should be able to change emails if multiuser has more than one identity
  • superuser switch route (by email) needs multiuser disambiguation
  • new user switch route by orga
  • organization read access query: allow multiuser indirection
  • set lastLoggedInIdentity upon login and upon switch
  • use lastLoggedInIdentity upon login
  • superusers should see all orgas
  • forbid switching to deactivated users
  • user switch route should lazily create user for superusers
  • invite table in schema, invite case class + service + DAO
  • create + save invite tokens
  • add route to get organization name for invite token
  • assert valid invite tokens in join request
  • invalidate invite tokens after they are used to join
  • also allow registering by invite (make sure logic doesn’t break single-orga behavior)
  • send new-user notification upon user join to orga admin
  • send invite emails (remove link from stdout)
  • double-check that change password still works (both reset and change while logged in)
  • repair createOrganizationWithAdmin request (create multi-user, handle loginInfo correctly)
  • add sql constraint for users that (_multiUser, _organization) should be unique
  • make GET routes POST where appropriate
  • db migration
    • adapt schema
    • insert multiusers for each user
    • connect the multiuser references
    • migrate tokens (replace user email with user id)
    • reversion?
  • adapt e2e-test db
  • re-enable unused-warnings in build.sbt
  • switch back isDemoInstance default in application.conf to false
  • switch back email.logToStdout default in application.conf to false

TODO Frontend

  • in user menu (top right of screen), offer to switch to other organizations
    • also make it a bit more obvious that there are multiple organizations
  • change invite UI, no more link, instead, enter recipients, add autoActivate checkbox (default false), send to backend
  • new view /invite/<tokenValue>
    • if logged in, show button to join organization with currently active account.
    • if logged out, enforce authentication (either, via registration which should send the inviteToken as additional form field) or by logging in (should do an explicit join afterwards)

New Routes

  • GET api/auth/joinOrganization/:inviteToken (allows the user to switchOrganization afterwards)
  • GET api/auth/switchOrganization/:organizationName (exchanges user cookie, rediercts to dashboard)
  • GET api/organizations (list all you can switch to)
  • GET api/organizations/byInvite/:inviteToken (list the organization you have been invited to)
  • POST api/auth/sendInvites expects something like {recipients: ["[email protected]”, "[email protected]"], autoActivate: false}, for development now logs the invite token to stdout

Changed Routes

  • api/auth/register has new optional form field inviteToken (it is required for instances with >1 orga). It uses organizationName only if there is no token (to support legacy invite links for the moment) and does not use organizationDisplayName anymore. (Those are still used in api/auth/createOrganizationWithAdmin though)

Steps to test:

Hint: set mail.logToStdout = true in application.conf to see email contents.

  • as demo instance (features.isDemoInstance = true)
    • create a few organizations
    • invite user
    • as logged-in user of another org, click invite link, join
    • as logged-in user in that org, click invite link, try to join, should warn that you are already in that org
    • as logged-out user go to invite link, should offer option to register, or log in and then join
    • register via invite link should get you into the matching organization
    • as a user with multiple orgs, use switch menu
    • as superuser, use switch menu, should see all orgs, should be able to switch to all orgs
  • as single-org instance
    • invite user
    • register upon invite should land you in that org
    • register without invite should also land you in that org

Issues:


@fm3 fm3 added the backend label Dec 7, 2020
@fm3 fm3 self-assigned this Dec 7, 2020
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Very nice work! Please see my initial comments. I'll add my observations from testing later today :)

app/views/mail/notifyAdminNewUser.scala.html Show resolved Hide resolved
frontend/javascripts/admin/auth/registration_view.js Outdated Show resolved Hide resolved
frontend/javascripts/admin/onboarding.js Outdated Show resolved Hide resolved
frontend/javascripts/admin/onboarding.js Outdated Show resolved Hide resolved
frontend/javascripts/libs/user_local_storage.js Outdated Show resolved Hide resolved
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

My minor observations from testing, everything worked flawlessly otherwise:

  • Visiting an invite link again, after it was used before, shows the correct message on screen but also a generic error toast: ("Couldn't find the requested resource.")
  • I wonder if we could make it a little more visible which organization a user has selected. Maybe the background color of the circle could be organization-dependent. This could allow users to easily spot that they have the wrong organization activated if they switch a lot.

Due to the complexity of this change it would probably be best, if someone else would test this as well :)

Copy link
Contributor

@youri-k youri-k left a comment

Choose a reason for hiding this comment

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

Wow, great stuff! 🎉

I did not test all functions yet.

app/controllers/Authentication.scala Outdated Show resolved Hide resolved
app/controllers/Authentication.scala Show resolved Hide resolved
app/controllers/Authentication.scala Show resolved Hide resolved
app/models/user/UserService.scala Outdated Show resolved Hide resolved
app/controllers/OrganizationController.scala Show resolved Hide resolved
app/models/user/MultiUser.scala Show resolved Hide resolved
@philippotto
Copy link
Member

Visiting an invite link again, after it was used before, shows the correct message on screen but also a generic error toast: ("Couldn't find the requested resource.")

Good point! Just pushed a fix for this.

I wonder if we could make it a little more visible which organization a user has selected. Maybe the background color of the circle could be organization-dependent. This could allow users to easily spot that they have the wrong organization activated if they switch a lot.

Cool idea, but I'm a bit hesitant about the actual usefulness of differently colored icons. First, I think that most users will stay in their "main" organization (a big reason for why we built this was that some users got into the wrong organization and switching was very complicated) and thus don't need to identify the current organization using a color. Second, I think the mapping from color to actual organization is quite hard for the user. There might be power users who switch often and then can identify the color well, too, but I feel like there should be a better way to do this. For example, one could show the full (or abbreviated) organization name in the navbar somewhere. However, getting this right (UI-wise) will be a bit tricky which is why I'd like to postpone this. Is this okay?

@daniel-wer
Copy link
Member

However, getting this right (UI-wise) will be a bit tricky which is why I'd like to postpone this. Is this okay?

Yes fine for me 👍

@fm3
Copy link
Member Author

fm3 commented Jan 4, 2021

@philippotto if you have a minute could you have a look at the conflict in yarn.lock? (comes from #5003 )

@philippotto
Copy link
Member

@philippotto if you have a minute could you have a look at the conflict in yarn.lock? (comes from #5003 )

Sure, I just pushed the resolved conflict. For the record, one can simply run yarn install and the conflict in the lock file will be resolved automatically :)

@fm3
Copy link
Member Author

fm3 commented Jan 5, 2021

@youri-k Thanks for your feedback, I implemented most of it :) Could you have another look?

Copy link
Contributor

@youri-k youri-k left a comment

Choose a reason for hiding this comment

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

I tested the described steps and everything worked flawlessly 🎉

See my small code comments

app/controllers/Authentication.scala Show resolved Hide resolved
app/models/user/UserService.scala Outdated Show resolved Hide resolved
@fm3 fm3 merged commit 0d9e69b into master Jan 11, 2021
@fm3 fm3 deleted the multiusers branch January 11, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Global users on webknossos.org
4 participants