Skip to content

Conversation

smatting
Copy link
Contributor

@smatting smatting commented Jan 8, 2021

@smatting smatting requested a review from fisx January 8, 2021 10:17
email <- randomEmail
call $
changeEmailBrig brig uid email !!! do
(fmap Wai.label . responseJsonEither @Wai.Error) === const (Right "property-managed-by-scim")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we're using property as a technical term for something else. What about managed-by-scim? Or user-managed-by-scim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to managed-by-scim

@@ -147,6 +148,11 @@ import Network.Wai.Utilities
import qualified System.Logger.Class as Log
import System.Logger.Message

data AllowSCIMUpdates
Copy link
Contributor

Choose a reason for hiding this comment

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

move to update section below line 401?

Comment on lines 445 to 448
when
( userManagedBy u == ManagedByScim
&& allowScim == ForbidSCIMUpdates
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
when
( userManagedBy u == ManagedByScim
&& allowScim == ForbidSCIMUpdates
)
unless
( userManagedBy u /= ManagedByScim
|| old handle == new handle -- pseudo-code
|| allowScim /= ForbidSCIMUpdates
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -525,6 +544,8 @@ changeEmail u email = do
-- The user already has an email address and the new one is exactly the same
Just current | current == em -> return ChangeEmailIdempotent
_ -> do
when (userManagedBy usr == ManagedByScim && allowScim == ForbidSCIMUpdates) $
Copy link
Contributor

Choose a reason for hiding this comment

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

like above, you could change this to unless ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@fisx
Copy link
Contributor

fisx commented Jan 8, 2021

Failures:

  test-integration/Test/Spar/Scim/UserSpec.hs:1594:23: 
  1) Spar.Scim.User, email validation, enabled in team, gives user email
       uncaught exception: ErrorCall
       Assertions failed:
        1: 201 =/= 500
       
       Response was:
       
       Response {responseStatus = Status {statusCode = 500, statusMessage = "Internal Server Error"}, responseVersion = HTTP/1.1, responseHeaders = [("Transfer-Encoding","chunked"),("Date","Fri, 08 Jan 2021 11:26:14 GMT"),("Server","Warp/3.3.13"),("Content-Type","application/json")], responseBody = Just "{\"code\":500,\"message\":\"{\\\"status\\\":\\\"500\\\",\\\"schemas\\\":[\\\"urn:ietf:params:scim:api:messages:2.0:Error\\\"],\\\"detail\\\":\\\"{\\\\\\\"code\\\\\\\":403,\\\\\\\"message\\\\\\\":\\\\\\\"Updating \\\\\\\\\\\\\\\"email\\\\\\\\\\\\\\\" is not allowed, because it is managed by SCIM\\\\\\\",\\\\\\\"label\\\\\\\":\\\\\\\"property-managed-by-scim\\\\\\\"}\\\"}\",\"label\":\"server-error\"}", responseCookieJar = CJ {expose = []}, responseClose' = ResponseClose}
       CallStack (from HasCallStack):
         error, called at src/Bilge/Assert.hs:88:5 in bilge-0.22.0-LWSwcj5o3J5utdi7oQriS:Bilge.Assert
         <!!, called at test-integration/Util/Scim.hs:191:5 in main:Util.Scim
         createUser, called at test-integration/Test/Spar/Scim/UserSpec.hs:1579:29 in main:Test.Spar.Scim.UserSpec
         setup, called at test-integration/Test/Spar/Scim/UserSpec.hs:1594:23 in main:Test.Spar.Scim.UserSpec

  To rerun use: --match "/Spar.Scim.User/email validation/enabled in team/gives user email/"

Randomized with seed 1959313722

I think when spar is calling an internal brig end-point here to trigger email validation, and that end-point needs to call changeEmail of changeSelfEmail with AllowSCIMUpdates, not ForbidSCIMUpdates.

You should check if there are any other calls to this end-point and make sure that it's always AllowSCIMUpdates, or else make it a query parameter (mandatory if you want to be more certain the integration tests capture anything you've missed).

@smatting
Copy link
Contributor Author

smatting commented Jan 11, 2021

Fixed the failing test by allow scim updates on the internal endpoint /i/self/email (I failed to see this before).
I think this is a safe choice, because only public endpoints change their behaviour now.

@smatting smatting merged commit ec74254 into develop Jan 12, 2021
@smatting smatting deleted the SQSERVICES-171-forbid-scim-managed-props branch January 12, 2021 13:25
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