IdPv3: race condition causes two persistentId to be generated

[Cantor, Scott]

On 9/23/15, 11:28 AM, "users on behalf of Dominique Petitpierre" <users-bounces at shibboleth.net on behalf of Dominique.Petitpierre at unige.ch> wrote:
>
>One question:
>
>a) Why does StoredPersistentIdGenerationStrategy generate a new
>    persistentId instead of reporting an error?  I.e. IMHO re-assigning
>    a new persistentId is a delicate matter and should be done only
>    under the control of a human by a special tool, not the IdP.

Because getCount() is ignoring the deactivation column whereas the earlier check for an existing ID doesn't. The race is caused in part by the difference in those queries. Deactivating the older ID causes it to generate a new one. That *is* the human control mechanism. The race is between checking for an active ID and then checking how to generate a new one.

>c) To avoid that two identical persistentId be stored (second case
>    above), may be one could rely on a database (or other storage)
>    constraint preventing duplicates, and for the second thread to
>    ignore the duplicate entry error message from the database.

That's not a standard error. Every deployer would have to supply the right message to look for. Without fully transactional semantics underneath (i.e. a full table lock), there will always be a race condition between the two selects and problematic handling of the result. It's very, very hard to make it work based on the experience we have had with the storage service unless we do our own locking (which means occasional cleanup by the sysadmin when stuff fails badly) or basically punt and blame the database when it doesn't. We can fix the race condition in the code, but it will be harder to actually fix it in the database because setting the isolation level in most databases doesn't actually do what it's defined to do.

Of course, we can always outright fail if the database does, it's just that the outcome will be failed identifier generation. That might be good enough given the fact that iframes in general do not work and are not supported anyway. That still leaves tabs though. We can also do what the SP does and provide a rertryable error control where the right messages can be fed in by the deployer to figure out what's a real error and what's a database bug not honoring the rules of the road.

Anyway, bottom line is that the code was skating by sloppy before because of V2's other bugs and now it will have to be rewritten to be correct, but making it harder to use.

-- Scott