NonUniqueResultException during RevokeOwnSecondFactorCommand

[phavekes]

This issue is imported from pivotal - Originaly created at Jul 24, 2019 by Pieter van der Meulen

When a user revokes their own activated tiqr token in stepup-selfservice, this fails with a "Token revocation failed" error in stepup-selfservice. Works for SMS and YK, so GSSP related?

The revocation mail is sent to the user. The logs show that the RevokeOwnSecondFactorCommand fails with a NonUniqueResultException from Doctrine. The token remains visible in stepup-selfservice and can be used for authentication at the stepup-gateway.

Tested on the dev environment with institution-b. Versions:

• Stepup-SelfService-develop-20190514102707Z-8133a06d38e2c3806a69c94fc99467ac3ea7bd99
• Stepup-Middleware-develop-20190603111411Z-c19187f327516f62415121509faf895cfd72150f

Jul 24 10:41:41 stepup-dev-app.test2.surfconext.nl stepup-middleware[23283]: {"channel":"app","level":"NOTICE","message":"Received request to process Command \"Surfnet\\StepupMiddleware\\CommandHandlingBundle\\Identity\\Command\\RevokeOwnSecondFactorCommand[31a4417a-474d-47ab-a628-09bed441fb50]\"","context":[],"extra":{"server":"middleware.dev.surfconext.nl","application":"middleware","request_id":"084c2d8153fc1ee7d9f1c80ebb070882"}}
Jul 24 10:41:41 stepup-dev-app.test2.surfconext.nl stepup-middleware[23283]: {"channel":"app","level":"NOTICE","message":"Ensuring that the actor institution is on the whitelist, or the actor is SRAA","context":[],"extra":{"server":"middleware.dev.surfconext.nl","application":"middleware","request_id":"084c2d8153fc1ee7d9f1c80ebb070882"}}
Jul 24 10:41:41 stepup-dev-app.test2.surfconext.nl stepup-middleware[23283]: {"channel":"app","level":"ERROR","message":"Exception occurred while processing command \"Surfnet\\StepupMiddleware\\CommandHandlingBundle\\Identity\\Command\\RevokeOwnSecondFactorCommand[31a4417a-474d-47ab-a628-09bed441fb50]\": \"\", rolling back transaction","context":{"exception":{"class":"Doctrine\\ORM\\NonUniqueResultException","message":"","code":0,"file":"/opt/stepup/Stepup-Middleware-develop-20190603111411Z-c19187f327516f62415121509faf895cfd72150f/vendor/doctrine/orm/lib/Doctrine/ORM/AbstractQuery.php:774"}},"extra":{"art":"36586","server":"middleware.dev.surfconext.nl","application":"middleware","request_id":"084c2d8153fc1ee7d9f1c80ebb070882"}}
Jul 24 10:41:41 stepup-dev-app.test2.surfconext.nl stepup-middleware[23283]: {"channel":"app","level":"CRITICAL","message":"","context":{"exception":{"class":"Doctrine\\ORM\\NonUniqueResultException","message":"","code":0,"file":"/opt/stepup/Stepup-Middleware-develop-20190603111411Z-c19187f327516f62415121509faf895cfd72150f/vendor/doctrine/orm/lib/Doctrine/ORM/AbstractQuery.php:774"}},"extra":{"art":"36586","server":"middleware.dev.surfconext.nl","application":"middleware","request_id":"084c2d8153fc1ee7d9f1c80ebb070882"}}
JJul 24 10:41:41 stepup-dev-app.test2.surfconext.nl stepup-selfservice[36326]: {"channel":"app","level":"NOTICE","message":"Logged in user with a session within time limits detected, updating session state","context":[],"extra":{"server":"sa.dev.surfconext.nl","application":"self-service","request_id":"084c2d8153fc1ee7d9f1c80ebb070882"}}
Jul 24 10:41:41 stepup-dev-app.test2.surfconext.nl stepup-selfservice[36326]: {"channel":"app","level":"WARNING","message":"Command \'Identity:RevokeOwnSecondFactor\' with UUID \'31a4417a-474d-47ab-a628-09bed441fb50\' could not be executed (Doctrine\\ORM\\NonUniqueResultException: )","context":[],"extra":{"server":"sa.dev.surfconext.nl","application":"self-service","request_id":"084c2d8153fc1ee7d9f1c80ebb070882"}}

Estimation: Very hard to estimate, depends on findings.

[phavekes]

The problem is that when a vetted token is removed in the RaCandidate projection all the candidates are deleted with the specified IdentityId. When a identity could become a RA for multiple institutions this would result in an NonUniqueResultException because only a single candidate is expected to be deleted.

There is however a broader problem and that is that if a identity has multiple tokens you don\'t want to delete the candidate if only a single token is deleted but the identity still has another vetted token. This didn\'t seem to be implemented when allowing multiple tokens per identity and therefore this wasn\'t encountered when implementing FGA. (bstrooband - Jul 26, 2019)

[phavekes]

How I think it should work is that a user is an RA candidate for an institution if and only if the user:

• Currently has at least one vetted LoA 3 token
• The user does not currently have an explicit role in the institution (i.e. use_ra and use_raa do not count)

Note that currently in Stepup there is only one LoA 3 token (Yubikey), but we\'ll have more soon when we take FIDO2 to production.

Support for multiple tokens was introduced in Release 15. What is the impact on Release 15 and 16? (Pieter van der Meulen - Jul 26, 2019)

[phavekes]

I\'ve added a Behat test scenario to validate the behavior and could verify that a RA candidate would be removed when a vetted second factor was removed even when there where valid (vetted) tokens left. Therefore I\'ve introduced an event which will fire if an identity doesn\'t have any valid vetted token left so the candidate will only get removed after that event. And if that candidate has multiple entries in the ra candidate projection they are all getting removed, so the NonUniqueResultException shouldn\'t be thrown at that moment. (bstrooband - Jul 29, 2019)

[phavekes]

The problem is that if an identity has multiple vetted tokens and one of them token gets revoked then the identity is removed from the ra candidates projection so the identity won\'t be able to get selected to become RA(A) because the user is not in the projection. If it is currently not possible to have more then one vetted token per identity then it would have affected the production data. (bstrooband - Jul 29, 2019)

[phavekes]

@bstrooband Since R15 it is possible to have more than one token per identity, and this is used in production. (Pieter van der Meulen - Jul 29, 2019)

[phavekes]

As discussed. There are two bugs:

Bug introdiced in Release 17

In R17 the revocation event assumes that an identity never has more then one entry in the ra_candidate table. This assumption is incorrect as an identity can now have more than one entry in the ra_candidate, one for each institution where the identity\'s institution is included in select_raa. This is was caused the NonUniqueResultException

Bug introduced in Release 15

Identities are always removed from the ra_candidate table when a token associated by the identity is revoked, this is incorrect. Since Release 15 a user may have more than one vetted token. An identity must only be removed from the ra_candidate table when the user has no active tokens. The current behaviour (R15 + R16) leads to identities being removed from the ra_candidate table when they should not. The effect is that these users cannot be granted the RA(A) role. This can only happen when the user has more than one token. A workaround is to vet a new token for the user as this causes the identity to be added to the ra_candidate projection.

You can use the following query to find the affected identities:

SELECT DISTINCT vetted.identity_id FROM `middleware`.`vetted_second_factor` as vetted 
    WHERE vetted.identity_id NOT IN (SELECT candidate.identity_id FROM `middleware`.`ra_candidate` as candidate)
      AND vetted.identity_id NOT IN (SELECT listing.identity_id FROM `middleware`.`ra_listing` as listing)
    ORDER BY vetted.identity_id
``` (Pieter van der Meulen - Jul 29, 2019)

[phavekes]

Thanks for the recap.

Be aware that the query will work in R15 but not from R17 (FGA) on wards because a user could then become a RA for multiple different institutions and not only for their own.

I\'ve also looked at the migrations for the FGA but only an extra column is added ra_institution and will use the already available institution column to fill that column:

UPDATE ra_listing SET ra_institution=institution
UPDATE ra_candidate SET ra_institution=institution

I was hoping that pushing the institution configuration again would work but it would only work if the configuration is changed and then only the changed select_raaoptions are updated so unfortunately this won\'t work either out of the box.

So currently indeed the only resort is to vet a new token. (bstrooband - Jul 29, 2019)

[phavekes]

The problem was fixed in https://github.com/OpenConext/Stepup-Middleware/pull/284

What still needs to be done is to perform more indepth verification. We will discuss this during stand-up or on the slack channel.

What certainly remains to be done is the verification that the event stream is replayable. (Michiel Kodde - Jan 6, 2020)