Various improvements and fixes around SAML/SCIM by fisx · Pull Request #1735 · wireapp/wire-server

fisx · 2021-09-03T18:03:47Z

please read commit-by-commit. two of them may be worth commenting on:

a20a32c the error message when attempting to saml-authenticate with a user that should have been provisioned by scim, but wasn't, was confusing. we have a better one now, and the function has a clearer structure.
399fa56 is bumping saml2-web-sso to the latest master, and shouldn't change any behavior: saml2-web-sso is providing CI.CI-wrapped values in a few places (mostly email and NameID), and we just unpack it using CI.original, which recovers all casing information. in the future, we'll have the option to treat emails case-insensitively as we're supposed to. (there is currently another, more hacky way in which we do this, see here and the internal issue.)

Checklist

fisx · 2021-09-03T18:07:05Z

Failures:

  test-integration/Test/Spar/Scim/UserSpec.hs:677:3: 
  1) Spar.Scim.User, POST /Users, gives created user a valid 'SAML.UserRef' for SSO
       "<?xml version=\"1.0\" encoding=\"UTF-8\"?><!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.1//EN\" \"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd\"><html xml:lang=\"en\" xmlns=\"http://www.w3.org/1999/xhtml\"><head>  <title>wire:sso:success</title>   <script type=\"text/javascript\">       const receiverOrigin = '*';       window.opener.postMessage({type: 'AUTH_SUCCESS'}, receiverOrigin);   </script></head></html>" does not contain "<title>wire:sso:error:forbidden</title>"

I think fixing this involves changing the integration tests only, but I'm not sure about what this means for client behavior. Will check later.

fisx · 2021-09-09T21:39:46Z

  test-integration/Test/Spar/Scim/UserSpec.hs:807:12: 

  1) Spar.Scim.User, GET /Users, 1 SAML IdP, finds a SCIM-provisioned user by userName or externalId

       expected: [WithMeta {meta = Meta {resourceType = UserResource, created = 2021-09-08 20:17:59.934 UTC, lastModified = 2021-09-08 20:17:59.934 UTC, version = Weak "f2d0565e0adb388fa66a947a15864a9b5a2445365e2f366ab8c44d36ed248803", location = URI {unURI = http://spar:8080/scim/v2/Users/51bd30a5-2ca0-4dc1-9b55-2932a7a2c75d}}, thing = WithId {id = 51bd30a5-2ca0-4dc1-9b55-2932a7a2c75d, value = User {schemas = [User20,CustomSchema "urn:wire:scim:schemas:profile:1.0",CustomSchema "urn:ietf:params:scim:schemas:extension:wire:1.0:User"], userName = "scimuser_3052059", externalId = Just "scimuser_extid_3052059@example.com", name = Nothing, displayName = Just "ScimUser3052059", nickName = Nothing, profileUrl = Nothing, title = Nothing, userType = Nothing, preferredLanguage = Nothing, locale = Nothing, active = Just (ScimBool {unScimBool = True}), password = Nothing, emails = [], phoneNumbers = [], ims = [], photos = [], addresses = [], entitlements = [], roles = [], x509Certificates = [], extra = ScimUserExtra {_sueRichInfo = RichInfo {unRichInfo = RichInfoAssocList {unRichInfoAssocList = [RichField {richFieldType = "FR}\1069886n\1109209Q-UO\1020294gc \9890#\"\1073334/\1078988\183755\1001982U\141827s\RS\1061960\ACK,", richFieldValue = "5l\ESC;\1102490@\FSd\1111936\1090308!*\ACK+\rh\71691\186898\12095Uws\SOHL\r"},RichField {richFieldType = "\SOH(\5064mf\1050663\SYNMNd)\ESC?\DLE\vI]\SOHIxB\1039994v!|x\1094792\13229R\1098559", richFieldValue = "O\61505C\178252f`"},RichField {richFieldType = "xan5#l\95773", richFieldValue = "I\CAN&\156762\60202D\182435\159411\&6LRS7/4+\46685KU\a"},RichField {richFieldType = "\n\1067818}C\bc\EOT\DC4@\1091142~m\991759\ETB\EM>?BAg\1033300\&1", richFieldValue = "^\SOH\1057183\NUL)c\ETX%\SI\SIJ\186499\283!\128184\65815rX?"},RichField {richFieldType = "\ACKWX\1066478+f\1025614\&3\1051673\ETX\SUB\SOHE\1105136D\NAKU\1014819my\156232\1108624", richFieldValue = "CxU9@\98421>"},RichField {richFieldType = "_B\480\1071940W\ar\SOH\SUB\EOT\78735\13091u\55013n\DC2\162653\ESC", richFieldValue = "\SO\39887\"a\DC46\STXY\43836"},RichField {richFieldType = "\27946x\a\1104021\ESC\DLEN\176982\EOTy\1024058\993352x%\66599\1003433'", richFieldValue = "?m\96452K9=\USp\aK\1050695C\184036"}]}}}}}}]

        but got: [WithMeta {meta = Meta {resourceType = UserResource, created = 2021-09-08 20:17:59.934 UTC, lastModified = 2021-09-08 20:17:59.934 UTC, version = Weak "f2d0565e0adb388fa66a947a15864a9b5a2445365e2f366ab8c44d36ed248803", location = URI {unURI = http://spar:8080/scim/v2/Users/51bd30a5-2ca0-4dc1-9b55-2932a7a2c75d}}, thing = WithId {id = 51bd30a5-2ca0-4dc1-9b55-2932a7a2c75d, value = User {schemas = [User20,CustomSchema "urn:wire:scim:schemas:profile:1.0",CustomSchema "urn:ietf:params:scim:schemas:extension:wire:1.0:User"], userName = "scimuser_3052059", externalId = Just "scimuser_extid_3052059@example.com", name = Nothing, displayName = Just "ScimUser3052059", nickName = Nothing, profileUrl = Nothing, title = Nothing, userType = Nothing, preferredLanguage = Nothing, locale = Nothing, active = Just (ScimBool {unScimBool = True}), password = Nothing, emails = [], phoneNumbers = [], ims = [], photos = [], addresses = [], entitlements = [], roles = [], x509Certificates = [], extra = ScimUserExtra {_sueRichInfo = RichInfo {unRichInfo = RichInfoAssocList {unRichInfoAssocList = [RichField {richFieldType = "fr}\1069886n\1109209q-uo\1020294gc \9890#\"\1073334/\1078988\183755\1001982u\141827s\RS\1061960\ACK,", richFieldValue = "5l\ESC;\1102490@\FSd\1111936\1090308!*\ACK+\rh\71691\186898\12095Uws\SOHL\r"},RichField {richFieldType = "\SOH(\5064mf\1050663\SYNMNd)\ESC?\DLE\vI]\SOHIxB\1039994v!|x\1094792\13229R\1098559", richFieldValue = "O\61505C\178252f`"},RichField {richFieldType = "xan5#l\95773", richFieldValue = "I\CAN&\156762\60202D\182435\159411\&6LRS7/4+\46685KU\a"},RichField {richFieldType = "\n\1067818}c\bc\EOT\DC4@\1091142~m\991759\ETB\EM>?bag\1033300\&1", richFieldValue = "^\SOH\1057183\NUL)c\ETX%\SI\SIJ\186499\283!\128184\65815rX?"},RichField {richFieldType = "\ACKwx\1066478+f\1025614\&3\1051673\ETX\SUB\SOHe\1105136d\NAKu\1014819my\156232\1108624", richFieldValue = "CxU9@\98421>"},RichField {richFieldType = "_b\481\1071940w\ar\SOH\SUB\EOT\78735\13091u\55013n\DC2\162653\ESC", richFieldValue = "\SO\39887\"a\DC46\STXY\43836"},RichField {richFieldType = "\27946x\a\1104021\ESC\DLEn\176982\EOTy\1024058\993352x%\66639\1003433'", richFieldValue = "?m\96452K9=\USp\aK\1050695C\184036"},RichField {richFieldType = "\SOH(\43928mf\1050663\SYNmnd)\ESC?\DLE\vi]\SOHixb\1039994v!|x\1094792\13229r\1098559", richFieldValue = "O\61505C\178252f`"}]}}}}}}]

I saw this locally, but then couldn't reproduce it any more and was hoping it was something about me compiling it wrong. But it looks like it's a real thing.

SAML auto-provisioning only works if scim is disabled. This commit makes the guard more straight-forward and provides a less confusing error message.

fisx · 2021-09-10T07:58:52Z

Can't reproduce. I could, a few times, but now I can't any more.

The evidence is very confusing:

the failure only happens occasionally with unclear probability (felt like <10% even when I could reproduce it).
only richinfo is affected, in the following way: the data that is fetched from wire after having been stored (a) has lower-cased keys (not values), and (b) has one extra field.
at the same time, it's clearly not replaced by a completely different value (all the data is intact except the above two points)

I could only reproduce it on da75bba, not on 46cca3f. I have no hypothesis how the changes in saml2-web-sso, or the changes to email and NameID namgling in 46cca3f could affect the way data is stored and retrieved via scim. And only non-deterministically?!

My only answer is to fall back to the theory that there was something wrong with the build, and wait for the issue to resurface to prove me wrong... :/

This introduces optionally case-insensitive emails. This patch keeps using case information, but it's now appearent where it could be ignored in the future, and how.

fisx · 2021-09-10T14:56:38Z

It looks like the problems shown above already happen on develop, and I still have no good idea what they are about. I'll move this question to another PR.

julialongtin · 2021-09-10T16:10:09Z

services/spar/src/Spar/App.hs

@@ -421,7 +427,7 @@ verdictHandlerResultCore bindCky = \case
        -- This is the first SSO authentication, so we auto-create a user. We know the user
        -- has not been created via SCIM because then we would've ended up in the
        -- "reauthentication" branch, so we pass 'ManagedByWire'.


we remove 'ManagedByWire' here, doesn't this mean this comment should be updated?

thanks, will fix in #1755!

fisx force-pushed the various-fixes branch from 9df153c to 6d0a431 Compare September 8, 2021 09:39

fisx changed the title ~~Various integration test improvements and fixes around SAML/SCIM [skip ci]~~ Various improvements and fixes around SAML/SCIM Sep 8, 2021

fisx force-pushed the various-fixes branch 4 times, most recently from f5b93cf to 3db41d0 Compare September 9, 2021 13:29

fisx added 5 commits September 10, 2021 08:40

hscim: add TODO; tweak .gitignore

bf49a48

TODO

b0a6382

Improve test coverage.

ff7b47b

Improve error response, simplify app logic.

a20a32c

SAML auto-provisioning only works if scim is disabled. This commit makes the guard more straight-forward and provides a less confusing error message.

Whitespace.

da75bba

fisx force-pushed the various-fixes branch from 33041b3 to da75bba Compare September 10, 2021 06:43

Generate printable strings in arbitrary instance.

5a763d0

fisx force-pushed the various-fixes branch from 46cca3f to 7ba734e Compare September 10, 2021 14:24

Bump saml2-web-sso dep.

399fa56

This introduces optionally case-insensitive emails. This patch keeps using case information, but it's now appearent where it could be ignored in the future, and how.

fisx force-pushed the various-fixes branch from 7ba734e to 399fa56 Compare September 10, 2021 14:55

fisx marked this pull request as ready for review September 10, 2021 15:02

fisx requested a review from julialongtin September 10, 2021 15:02

CHANGELOG

7cdcedf

This was referenced Sep 10, 2021

Scim users get distorted between construct, post, get. #1754

Merged

Introduce team-based SP entityIDs to allow IdPs with fixed entityID to serve more than one team. #1755

Merged

julialongtin reviewed Sep 10, 2021

View reviewed changes

julialongtin approved these changes Sep 10, 2021

View reviewed changes

fisx merged commit a4e049d into develop Sep 10, 2021

fisx deleted the various-fixes branch September 10, 2021 18:55

jschaul mentioned this pull request Sep 13, 2021

Release 2021-09-14 #1764

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Various improvements and fixes around SAML/SCIM#1735

Various improvements and fixes around SAML/SCIM#1735
fisx merged 8 commits intodevelopfrom
various-fixes

fisx commented Sep 3, 2021 •

edited

Loading

Uh oh!

fisx commented Sep 3, 2021

Uh oh!

fisx commented Sep 9, 2021

Uh oh!

fisx commented Sep 10, 2021

Uh oh!

fisx commented Sep 10, 2021

Uh oh!

julialongtin Sep 10, 2021

Uh oh!

fisx Sep 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Conversation

fisx commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

fisx commented Sep 3, 2021

Uh oh!

fisx commented Sep 9, 2021

Uh oh!

fisx commented Sep 10, 2021

Uh oh!

fisx commented Sep 10, 2021

Uh oh!

julialongtin Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

fisx Sep 10, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fisx commented Sep 3, 2021 •

edited

Loading