Skip to content

Comments

SQSERVICES-377 fix email verification when external id is updated via SCIM#2374

Merged
battermann merged 3 commits intodevelopfrom
SQSERVICES-377-changing-external-id-via-scim-doesnt-update-email
May 18, 2022
Merged

SQSERVICES-377 fix email verification when external id is updated via SCIM#2374
battermann merged 3 commits intodevelopfrom
SQSERVICES-377-changing-external-id-via-scim-doesnt-update-email

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented May 9, 2022

https://wearezeta.atlassian.net/browse/SQSERVICES-377

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.schema documentation.
  • 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.

@battermann battermann temporarily deployed to cachix May 9, 2022 14:36 Inactive
@battermann battermann changed the title fix email verification when external id is updated via SCIM SQSERVICES-377 fix email verification when external id is updated via SCIM May 9, 2022
@battermann battermann force-pushed the SQSERVICES-377-changing-external-id-via-scim-doesnt-update-email branch from 0658702 to 2d2b7c7 Compare May 9, 2022 14:37
@battermann battermann temporarily deployed to cachix May 9, 2022 14:37 Inactive
@battermann battermann temporarily deployed to cachix May 9, 2022 14:41 Inactive
@battermann battermann force-pushed the SQSERVICES-377-changing-external-id-via-scim-doesnt-update-email branch from c5f75de to 29f70c8 Compare May 9, 2022 14:47
@battermann battermann temporarily deployed to cachix May 9, 2022 14:47 Inactive
@battermann battermann requested review from fisx and smatting May 9, 2022 15:01
deriving (Eq, Show)

-- | Note that a 'SAML.UserRef' may contain an email. Even though it is possible to construct a 'ValidExternalId' from such a 'UserRef' with 'UrefOnly',
-- this does not represent a valid 'ValidExternalId'. So in case of a 'UrefOnly', we can assume that the 'UserRef' does not contain an email.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there as smart constructor that enforces this somewhere? It doesn't seem like an assumption that is likely to "bite" us in the future, but it also shouldn't be expensive to have the code check to ensure we aren't recording a UNameIDEmail in a URefOnly (losslessly up-converting to using EmailAndUref instead).

Copy link
Contributor

Choose a reason for hiding this comment

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

I would aim for a tag at the NameID type instead that guarantees the existence of an email address at the type level. A smart constructor for UserRef would work, too, but my hunch is that it'll leave more room for error.

Not sure we'll have the time for either, though. If you want to try touching saml2-web-sso knock yourself out! :)

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.

LGTM 👍

@battermann battermann temporarily deployed to cachix May 16, 2022 09:06 Inactive
@battermann battermann merged commit 440c43b into develop May 18, 2022
@battermann battermann deleted the SQSERVICES-377-changing-external-id-via-scim-doesnt-update-email branch May 18, 2022 06:39
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.

4 participants