Brig: Servantify `POST /register` and `POST /i/users` endpoint #2121

akshaymankar · 2022-02-10T11:05:56Z

https://wearezeta.atlassian.net/browse/SQCORE-1168

Noteworthy things:

I servantified POST /i/users because it had pretty much the same API and was using the same types.
While registering a user into a team, if the team has legal hold, the user can only be added if the fanoutLimit has not been reached. This check happens in galley, which generates a Wai.Error, brig just throws this error and so isn't aware of internals. This kind of error cannot be transformed into and ErrorDescription. So, for now (until galley's internal API is servantiified), brig throws this error using throwM, this error gets caught and handled by the Handler monad when it executes requests. This is of course not ideal. But hopefully we get to servantify Galley's internal endpoints and we can get rid of this hack.
The parser for NewUser is fairly complex some fields depend on other fields. To help with this, I created NewUserRaw which just parses individual fields. The schema for NewUser type uses withParser to do the validation and make composite fields.
Remove ToJSON/FromJSON for UserIdentity, use Schema for User. The ToJSON for UserIdentity encoded nulls, but every other object which used it didn't encode nulls. So, it was better to remove it.

Checklist

The PR Title explains the impact of the change.
The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
changelog.d contains the following bits of information (details):
- A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

fisx

Looks good, but I haven't gone enough into the details to confidently approve yet. Have you taken a look at the swagger output?

fisx · 2022-02-16T20:22:47Z

services/galley/src/Galley/API/Teams.hs

+-- those to enrich 'Wire.API.User.RegisterError' and ensure that these errors
+-- also show up in swagger. Currently, the error returned by this endpoint is
+-- thrown in IO, we could then refactor that to be thrown in `ExceptT
+-- RegisterError`.


I'm perfectly comfortable with waiting for polysemy with this, but sure, we can also touch it again when the internal API is servantified.

fisx · 2022-02-16T20:28:26Z

services/brig/test/integration/API/User/Account.hs

-- details.
--
-- You should have received a copy of the GNU Affero General Public License along
-- with this program. If not, see <https://www.gnu.org/licenses/>.


I tripped over this and ran headroom again just to be sure, and there were changes. Small commit coming up.

fisx · 2022-02-16T20:48:46Z

libs/wire-api/src/Wire/API/User.hs

-    newUserExpires <- o A..:? "expires_in"
-    newUserExpiresIn <- case (newUserExpires, newUserIdentity) of
+-- | Raw representation of 'NewUser' to help with writing Schema instances.
+data NewUserRaw = NewUserRaw


so much nicer, thanks!

fisx · 2022-02-16T20:53:31Z

libs/wire-api/src/Wire/API/Util/Aeson.hs

+eitherToParser (Left e) = fail e
+eitherToParser (Right a) = pure a


Suggested change

eitherToParser (Left e) = fail e

eitherToParser (Right a) = pure a

eitherToParser = either fail pure

is this even worth a new name? Possibly. Please ignore me. :)

I agree, we can just use either fail pure directly. Short and clear.

fisx · 2022-02-16T21:00:28Z

services/brig/src/Brig/API/Handler.hs

+  ok <- isWhiteListed key
+  unless ok (throwError e)


Suggested change

ok <- isWhiteListed key

unless ok (throwError e)

unlessM (isWhiteListed key) (throwError e)

https://hoogle.zinfra.io/file/wire-server/.stack-work/install/x86_64-linux-tinfo6/4477baab320e495f60baecbf311b8b6399a53d09f9ec1ecba83646a2c65c551e/8.10.7/doc/imports-0.1.0/Imports.html#v:unlessM

pcapriotti

Looks good.

One thing I'm not sure of is the huge list of error responses for the register endpoint. Even with polysemy, that corresponds to a huge list of individual error effects, which is a pain to manage. It's also not so great documentation, IMHO, since it shows way too much detail.

I think it would be nicer to have groupings of errors, like "registration error", "authentication error" etc... Then we could document each group separately (this could even be somewhat automated). We have done this sort of thing with federation errors, where it was pretty obvious that showing 20 possible error responses for basically each endpoint would have been insane.

pcapriotti · 2022-02-17T08:05:10Z

libs/wire-api/src/Wire/API/User.hs

@@ -802,6 +835,24 @@ data NewTeamUser
  deriving stock (Eq, Show, Generic)
  deriving (Arbitrary) via (GenericUniform NewTeamUser)

+newTeamUserCode :: NewTeamUser -> Maybe InvitationCode


These can be autogenerated by makePrisms. For example, instead of newTeamUserCode you can use preview _NewTeamMember.

pcapriotti · 2022-02-17T08:06:59Z

libs/wire-api/src/Wire/API/User/Activation.hs

@@ -181,7 +184,7 @@ instance ToJSON ActivationResponse where
 instance FromJSON ActivationResponse where


Why doesn't this have a ToSchema instance?

pcapriotti · 2022-02-17T08:09:11Z

libs/wire-api/src/Wire/API/Util/Aeson.hs

+eitherToParser (Left e) = fail e
+eitherToParser (Right a) = pure a


I agree, we can just use either fail pure directly. Short and clear.

fisx · 2022-02-17T09:05:20Z

I think it would be nicer to have groupings of errors, like "registration error", "authentication error" etc... Then we could document each group separately (this could even be somewhat automated).

How do you guarantee that all errors in a group have the same code and label? Because that is the information we want to have for swagger code generation, no?

fisx · 2022-02-17T09:06:30Z

where it was pretty obvious that showing 20 possible error responses for basically each endpoint would have been insane.

Not obvious to me. Is it a run-time or compile-time performance issue? Or are you just unhappy with the length of the type signatures in characters?

The ToJSON for UserIdentity encoded nulls, but every other object which used it didn't encode nulls. So, it was better to remove it

This error cannot be easily encoding in the servant type as it is "dynamic" from the perspective of brig. Once we servantify galley's internal API, we can know about this error statically. This way we can catch it and throw it as a `RegisterError` and it will show up in brigs a `RegisterError` and it will show up in brig's swagger.

Noteworthy things: - I servantified `POST /i/users` because it had pretty much the same API and was using the same types. - While registering a user into a team, if the team has legal hold, the user can only be added if the fanoutLimit has not been reached. This check happens in galley, which generates a `Wai.Error`, brig just throws this error and so isn't aware of internals. This kind of error cannot be transformed into and `ErrorDescription`. So, for now (until galley's internal API is servantiified), brig throws this error using `throwM`, this error gets caught and handled by the `Handler` monad when it executes requests. This is of course not ideal. But hopefully we get to servantify Galley's internal endpoints and we can get rid of this hack. - The parser for `NewUser` is fairly complex some fields depend on other fields. To help with this, I created `NewUserRaw` which just parses individual fields. The schema for `NewUser` type uses `withParser` to do the validation and make composite fields. - Remove ToJSON/FromJSON for UserIdentity, use Schema for User. The ToJSON for UserIdentity encoded nulls, but every other object which used it didn't encode nulls. So, it was better to remove it. Co-authored-by: Matthias Fischmann <[email protected]>

akshaymankar force-pushed the akshaymankar/servantify-register branch 2 times, most recently from 0fe8259 to 5ede06a Compare February 16, 2022 14:46

akshaymankar changed the title ~~[WIP] Brig: Servantify POST /register endpoint~~ Brig: Servantify POST /register and POST /i/users endpoint Feb 16, 2022

akshaymankar marked this pull request as ready for review February 16, 2022 14:55

akshaymankar marked this pull request as draft February 16, 2022 14:58

akshaymankar marked this pull request as ready for review February 16, 2022 15:36

akshaymankar requested review from fisx and pcapriotti and removed request for fisx February 16, 2022 15:44

fisx reviewed Feb 16, 2022

View reviewed changes

pcapriotti approved these changes Feb 17, 2022

View reviewed changes

akshaymankar force-pushed the akshaymankar/servantify-register branch 2 times, most recently from 940a466 to 0bc874b Compare February 24, 2022 14:38

akshaymankar and others added 14 commits March 1, 2022 11:11

[WIP] Brig: Servantify POST /register endpoint

0e5719f

Add some comments about complicated things I did

fff043f

WIP

a1f90dd

Avoid using bind/dispatch for schema of NewUser

36fb13d

Remove ToJSON/FromJSON for UserIdentity, use Schema for User

256f50d

The ToJSON for UserIdentity encoded nulls, but every other object which used it didn't encode nulls. So, it was better to remove it

WIP

f59aad6

Things compile, with 1 thing undefined

db1871a

Broken test for user not being allowed to join a team

ed8ba10

Add futurework to cleanup throwing in IO and better swagger

68f35d4

Changelog

dd1dd99

Deduplicate errors

b4df4e8

Delete unused swagger

6ec48bd

make add-license

c3b2ed3

akshaymankar added 3 commits March 1, 2022 11:13

Remove redundant import

76e2fb4

Fix wrong merge

7e7cf06

Handler -> Handler r

17ff786

akshaymankar force-pushed the akshaymankar/servantify-register branch from 45366d6 to 17ff786 Compare March 1, 2022 10:34

Remove unhelpful helper eitherToParser

cac1e96

akshaymankar force-pushed the akshaymankar/servantify-register branch from 41fd598 to cac1e96 Compare March 2, 2022 16:32

akshaymankar merged commit dffa163 into develop Mar 3, 2022

akshaymankar deleted the akshaymankar/servantify-register branch March 3, 2022 10:44

zebot mentioned this pull request Mar 7, 2022

Release 2022-03-07 #2181

Merged

zebot mentioned this pull request Mar 9, 2022

Release 2022-03-09 #2188

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Brig: Servantify `POST /register` and `POST /i/users` endpoint #2121

Brig: Servantify `POST /register` and `POST /i/users` endpoint #2121

Uh oh!

akshaymankar commented Feb 10, 2022 •

edited

Loading

Uh oh!

fisx left a comment

Uh oh!

fisx Feb 16, 2022

Uh oh!

fisx Feb 16, 2022

Uh oh!

fisx Feb 16, 2022

Uh oh!

fisx Feb 16, 2022

Uh oh!

pcapriotti Feb 17, 2022

Uh oh!

fisx Feb 16, 2022

Uh oh!

pcapriotti left a comment

Uh oh!

pcapriotti Feb 17, 2022

Uh oh!

pcapriotti Feb 17, 2022

Uh oh!

pcapriotti Feb 17, 2022

Uh oh!

fisx commented Feb 17, 2022

Uh oh!

fisx commented Feb 17, 2022

Uh oh!

Uh oh!

		eitherToParser (Left e) = fail e
		eitherToParser (Right a) = pure a

	eitherToParser (Left e) = fail e
	eitherToParser (Right a) = pure a
	eitherToParser = either fail pure

	ok <- isWhiteListed key
	unless ok (throwError e)
	unlessM (isWhiteListed key) (throwError e)

		@@ -181,7 +184,7 @@ instance ToJSON ActivationResponse where
		instance FromJSON ActivationResponse where

Brig: Servantify POST /register and POST /i/users endpoint #2121

Brig: Servantify POST /register and POST /i/users endpoint #2121

Uh oh!

Conversation

akshaymankar commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcapriotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fisx commented Feb 17, 2022

Uh oh!

fisx commented Feb 17, 2022

Uh oh!

Uh oh!

Brig: Servantify `POST /register` and `POST /i/users` endpoint #2121

Brig: Servantify `POST /register` and `POST /i/users` endpoint #2121

akshaymankar commented Feb 10, 2022 •

edited

Loading