Skip to content

Comments

SQSERVICES-1519 IdP for SAML that was overridden by update cannot be added again#2400

Merged
battermann merged 6 commits intodevelopfrom
SQSERVICES-1519-tm-idp-for-saml-that-was-overriden-by-update-cannot-be-added-again
May 19, 2022
Merged

SQSERVICES-1519 IdP for SAML that was overridden by update cannot be added again#2400
battermann merged 6 commits intodevelopfrom
SQSERVICES-1519-tm-idp-for-saml-that-was-overriden-by-update-cannot-be-added-again

Conversation

@battermann
Copy link
Contributor

@battermann battermann commented May 17, 2022

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

Old behavior

When an IdP was updated, the entry in the table issuer_idp_v2 did not get overridden, but instead a new entry was made. Because the old entry still existed, adding the old IdP again failed.

Changes

In this PR, on IdP update, old issuers in issuer_idp_v2 will be deleted. Not sure if this is the correct fix, as those old issuers may serve some other purpose that I am not aware of?

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.

@battermann battermann temporarily deployed to cachix May 17, 2022 10:01 Inactive
@battermann battermann temporarily deployed to cachix May 17, 2022 10:03 Inactive
@battermann battermann marked this pull request as ready for review May 17, 2022 10:03
@battermann battermann requested review from elland and fisx May 17, 2022 10:04
(teamid, idp) <- validateIdPUpdate zusr idpmeta idpid
GalleyAccess.assertSSOEnabled teamid
forM_ (idp ^. SAML.idpExtraInfo . wiOldIssuers) IdPConfigStore.deleteIssuer
IdPRawMetadataStore.store (idp ^. SAML.idpId) raw
Copy link
Contributor

Choose a reason for hiding this comment

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

if spar crashes at the right moment, this will cause the old idp to be deleted and the new not to be introduced, which is not fixable over the rest api (let alone the UI).

I don't see a good way around this, though. It would be really nice to have transactions and postgres and everything. but i would like to have the comment below reflect this. (could you maybe also fix my typo there while you're at it ("epected")?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the deletion to the end which leaves a less inconsistent state, in the case of a failure. And I added a comment.

Co-authored-by: fisx <mf@zerobuzz.net>
@battermann battermann temporarily deployed to cachix May 18, 2022 12:49 Inactive
@battermann battermann temporarily deployed to cachix May 18, 2022 13:04 Inactive
@battermann battermann temporarily deployed to cachix May 19, 2022 07:00 Inactive
@battermann battermann merged commit 73d38b8 into develop May 19, 2022
@battermann battermann deleted the SQSERVICES-1519-tm-idp-for-saml-that-was-overriden-by-update-cannot-be-added-again branch May 19, 2022 10:14
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