[WPB-27953] Make scim error responses comply with RFC7644. - #5439
Conversation
zebot
added
the
ok-to-test
This is ruled out by the type signatures on the |
c198abc to
c0b8ea0
Compare
| -- RFC 7644 section 3.12 requires that the response body of a SCIM error *is* | ||
| -- the SCIM error object, not a wire-server 'Wai.Error' with the SCIM error | ||
| -- object nested (double-encoded) into its 'message' field. | ||
| it "renders a SCIM error as the bare RFC 7644 error object" $ do |
There was a problem hiding this comment.
This test is great to show that the rendering works. But, it would also be good to prove that it is actually used. Either with dedicated integration or effect tests or by adding assertions to existing tests. I would lean to the latter, because that's quicker to do.
Co-authored-by: Sven Tennie <sven.tennie@wire.com>
893df62 to
af00e70
Compare
| mkScimErrorResp :: | ||
| Maybe Text -> | ||
| Maybe Text -> | ||
| Text -> |
There was a problem hiding this comment.
| Text -> | |
| Int -> |
I would use Int here instead of handing integers around as strings. However, I'm in nitpicking mode 馃
There was a problem hiding this comment.
you're right, but i don't think it's important enough to go another CI round. (we need more ergonomic CI!)
yell at me if insist, otherwise i'll ignore you :)
Co-authored-by: Sven Tennie <sven.tennie@wire.com>
| deleteUser_ (Just tok) Nothing (env ^. teSpar) !!! do | ||
| const 405 === statusCode |
There was a problem hiding this comment.
Have you forgotten something here? Or, is deleteUser_'s error message well enough tested (I'd say so) and you just like do blocks? 馃檭
There was a problem hiding this comment.
i added the do-block mechanically to add the assertion on the body; then i realized that the body was empty; then i realized that this is a stupid smoke test on a route that isn't even defined in the standard (/Users/:id is defined, but /Users is not), and i decided it's not a very important test and testing the status is enough.
then i forgot to remove the do block. :)
| === statusCode | ||
| >>= fmap Search.searchResults . responseJsonError | ||
|
|
||
| -- | Assert the exact body of an error response from an endpoint that is /not/ scim: brig has |
There was a problem hiding this comment.
| -- | Assert the exact body of an error response from an endpoint that is /not/ scim: brig has | |
| -- | Create the exact body of an error response from an endpoint that is /not/ scim: brig has |
馃 (I would expect a call to should... or === and friends to justify the word assert here.)
There was a problem hiding this comment.
yeah, that was wrong. i thought that this would never happen so i didn't see the need to name the entire thing after it, but it turned out it did happen, and legitimately...
assert removed!
3 participants
Fixes #5434
This changes the error response body schema for most of the SCIM api. The release notes contain a warning about this with instructions how to re-align code. If anybody is impacted by this change, they are in violation of the RFC as we were and should follow the release note instructions.
https://wearezeta.atlassian.net/browse/WPB-27953
https://wearezeta.atlassian.net/browse/WPB-17623
Checklist
changelog.d