Skip to content

Fix spar user provisioning corner case#2174

Merged
fisx merged 7 commits intodevelopfrom
more-spar-tests
Mar 7, 2022
Merged

Fix spar user provisioning corner case#2174
fisx merged 7 commits intodevelopfrom
more-spar-tests

Conversation

@fisx
Copy link
Contributor

@fisx fisx commented Mar 2, 2022

https://wearezeta.atlassian.net/browse/CS-742

With the tests in this PR, but without the changes, I get this error:

Failures:

  test-integration/Test/Spar/Scim/UserSpec.hs:264:7:
  1) WireIdPAPIV1.Spar.Scim.User, Create with TM invitation; then re-provision with SCIM, True
       "<?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:error:not-found</title>   <script type=\"text/javascript\">       const receiverOrigin = '*';       window.opener.postMessage({\"payload\":{\"errors\":[\"Could not find SAML credentials, and auto-provisioning is disabled.\"],\"label\":\"forbidden\"},\"type\":\"AUTH_ERROR\"}, receiverOrigin);   </script></head></html>" does not contain "<title>wire:sso:success</title>"

  To rerun use: --match "/WireIdPAPIV1/Spar.Scim.User/Create with TM invitation; then re-provision with SCIM/True/"

  test-integration/Test/Spar/Scim/UserSpec.hs:264:7:
  2) WireIdPAPIV2.Spar.Scim.User, Create with TM invitation; then re-provision with SCIM, True
  [...]

Randomized with seed 55186091

Finished in 212.9946 seconds
527 examples, 2 failures, 67 pending

The changes in the later commits fix this. The problem was that under certain conditions, spar did not create SAML credentials during SCIM provisioning.

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 fisx force-pushed the more-spar-tests branch 8 times, most recently from 1521f5e to 3bc0a98 Compare March 7, 2022 10:00
@fisx fisx force-pushed the more-spar-tests branch from 3bc0a98 to 45ed2e8 Compare March 7, 2022 10:22
@fisx fisx changed the title more spar tests [skip ci] more spar tests Mar 7, 2022
@fisx fisx changed the title more spar tests Fix spar user provisioning corner case Mar 7, 2022
@fisx fisx marked this pull request as ready for review March 7, 2022 10:36
@fisx fisx requested a review from supersven March 7, 2022 10:36
Copy link
Contributor

@supersven supersven left a comment

Choose a reason for hiding this comment

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

The comments are optional and may be solved with another PR.

lift $ ST.runValidExternalId (validateEmailIfExists buid) (\_ -> pure ()) veid

-- {suspension via scim: if we don't reach the following line, the user will be active.}
-- TODO: suspension via scim is brittle, and may leave active users behind: if we don't
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO or FUTUREWORK? If it's a TODO we should do it, don't we? Otherwise, leave it for the future us and see, if this is really an issue.

-- lookup in that table either, but compute the `externalId` from the `UserRef`.
createValidScimUserSpar ::
forall m r.
( (m ~ Scim.ScimHandler (Sem r)),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if can't just inline m as it's only used in line 504. 🤔

specImportToScimFromInvitation :: SpecWith TestEnv
specImportToScimFromInvitation =
describe "Create with TM invitation; then re-provision with SCIM" $ do
check True
Copy link
Contributor

Choose a reason for hiding this comment

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

"Boolean blindness". Probably, this would be more readable with a dedicated type.

tenant = idp ^. SAML.idpMetadata . SAML.edIssuer
void $ createViaSaml idp privCreds uref

check :: Bool -> SpecWith TestEnv
Copy link
Contributor

Choose a reason for hiding this comment

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

One could order the functions by call time (hierarchy).

@fisx fisx merged commit 23673c0 into develop Mar 7, 2022
@fisx fisx deleted the more-spar-tests branch March 7, 2022 12:08
@zebot zebot mentioned this pull request Mar 7, 2022
@zebot zebot mentioned this pull request Mar 9, 2022
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