Skip to content

Conversation

@sysvinit
Copy link
Contributor

@sysvinit sysvinit commented Sep 13, 2022

https://wearezeta.atlassian.net/browse/SQPIT-1368

This PR extends the payload of the /team/:tid/invitation endpoints in brig to optionally return invitation URLs, so team admins can distribute them by means other than email. This functionality is gated behind a team feature flag in galley. As invitation URLs are otherwise considered sensitive, and enabling this feature might allow team admins to undermine the security of email validation, teams where this feature is enabled must be explicitly listed in the galley configuration file.

On the galley side, this introduces two new feature flags. exposeInvitationURLsToTeamAdmin is a team-scoped feature (with getter and setter routes in the public API) which controls whether the invitation URLs are exposed in the brig invitation endpoints. exposeInvitationURLsTeamAllowlist is a site-scoped feature (with no per-team settings) which specifies a list of teams for which the former flag may be enabled.

The logic for reading and toggling the exposeInvitationURLsToTeamAdmin flag depends on whether the team ID is named in the allowlist.

  • If a team is in the allowlist, then the feature lock and status are visible and can be toggled in the API as with other features -- i.e. team admins can view whether the feature is locked or not, and freely toggle the flag if not locked.
  • If a team is not on the allowlist, then galley will always report that the feature is locked and disabled for that team. There is a single exception here, in that if the feature is unlocked, a team is not in the allowlist, but they have the feature enabled in the database (because the team was previously in the allowlist and the team admin had explicitly enabled it at the time), then galley will report that the feature is enabled for that team up until the point where they disable it, at which point it becomes locked and disabled.

Given its sensitivity, I've intentionally written these features so that exposeInvitationURLsToTeamAdmin defaults to disabled and locked, and even when enabled requires a team allowlist to be configured, so that explicit operator effort is required to turn it on.

On the brig side, I've added a new optional field to the Invitation type for the invite URL, and I've extended the handlers for the invitation API so they first check the status of the team feature in galley, and then populate the URL field in the invitation payload if the feature is enabled for the given team. Annoyingly, this adds a round trip to galley for every call to the invitation API, and requires adding a stack of extra typeclass constraints to add all the capabilities required for making calls to galley (previously only MonadClient was needed, for the Cassandra queries) in these codepaths.

TODO:

  • Integration tests (test cases and galley configuration) (Delayed to a later PR. Manually tested instead.)
  • Regeneration of Cassandra schema
  • Add new feature endpoints to nginz (Not needed. we only add a field to an existing endpoint's body.)
  • Administrator/configuration documentation
  • Changelog

Someone else is free to pick this one up while I'm out of office later this week.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

The logic for determining whether this feature is enabled and/or locked for a
given team is more nuanced than the default implementation.
@sysvinit sysvinit temporarily deployed to cachix September 13, 2022 11:19 Inactive
@sysvinit sysvinit temporarily deployed to cachix September 13, 2022 11:19 Inactive
@fisx fisx temporarily deployed to cachix September 16, 2022 20:19 Inactive
@fisx fisx temporarily deployed to cachix September 16, 2022 20:19 Inactive
@fisx fisx temporarily deployed to cachix September 16, 2022 20:26 Inactive
@fisx fisx temporarily deployed to cachix September 16, 2022 20:26 Inactive
@fisx fisx force-pushed the sysvinit/feature-invitation-urls branch from 9fda742 to 96b35f6 Compare September 16, 2022 20:30
@fisx fisx temporarily deployed to cachix September 16, 2022 20:30 Inactive
@fisx fisx temporarily deployed to cachix September 16, 2022 20:30 Inactive
Copy link
Contributor

@fisx fisx left a comment

Choose a reason for hiding this comment

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

Please take a look at commits 1a93732..96b35f6 and decide if you like them. Plus one more nit-pick.

And you may want to test this again if you keep my changes.

Otherwise LGTM!

@fisx fisx temporarily deployed to cachix September 16, 2022 20:39 Inactive
@fisx fisx temporarily deployed to cachix September 16, 2022 20:39 Inactive
@fisx
Copy link
Contributor

fisx commented Sep 16, 2022

Add new feature endpoints to nginz (Not needed. we only add a field to an existing endpoint's body.)

actually, "not needed" is correct, but not for this reason. we need routing from nginz, but it already covers the new end-points:

    - path: /teams/([^/]*)/features/([^/])*
      envs:
      - all
    - path: /i/teams/([^/]*)/features/([^/]*)
      envs:
      - staging
      disable_zauth: true
      basic_auth: true
      versioned: false

@supersven supersven temporarily deployed to cachix September 18, 2022 17:18 Inactive
@supersven supersven temporarily deployed to cachix September 18, 2022 17:18 Inactive
@supersven supersven temporarily deployed to cachix September 19, 2022 05:51 Inactive
@supersven supersven temporarily deployed to cachix September 19, 2022 05:51 Inactive
@supersven supersven merged commit cb6e9c2 into develop Sep 19, 2022
@supersven supersven deleted the sysvinit/feature-invitation-urls branch September 19, 2022 08:32
isovector added a commit to isovector/wire-server that referenced this pull request Oct 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants