IdPv3: race condition causes two persistentId to be generated

[Cantor, Scott]

On 9/23/15, 2:12 PM, "users on behalf of Dominique Petitpierre" <users-bounces at shibboleth.net on behalf of Dominique.Petitpierre at unige.ch> wrote:
>
>OK. So changing getCount() to a getActiveCount() and check if the Id to 
>be stored is the same as the one stored would be a quick fix.

That just moves the race condition to the database (if locking is optimistic) or triggers errors (if pessimistic). It will take more work than that to fix, and ultimately will require work by deployers in most cases to define appropriate error messages if we try and deal with it using retry logic.

The problem is that the two selects (or a select and insert) have to occur under a table lock to be run in separate steps, and only a small number of databases correctly take a table lock when they're told to do so. A real RDBMS that works makes the code very simple to write, but unfortunately most RDBMS' these days are being broken to favor performance over correct behavior. SQL Server is about the only one I know for certain still provides correctly isolated transactions.

>- Would this be possible for IdP 3.2?

First it needs to be filed in Jira and the work scoped.

>That is unfortunate: we don't have the control on how web applications are implemented.
>I was under the impression that on the contrary IdPv3 would support or somehow tolerate iframes
>(in SSO mode).

It uses cookies. That means it can never support iframes, unless you require third party cookies be turned on. Somebody with more time than I have to work on it might be able to figure out how to move the JSESSIONID value into the URL. That's the only way it could ever work, but that's also less secure.

>If the code is rewritten it would be nice if it was integrated in the
>StorageService architecture.  For example one could then use a
>memcached implementation with persistence (and high availibility) to
>store both persistentIds and user consent.

The reason it wasn't switched is because of compatibility, but it may be a better idea if we have to really do significant work on this to do that change and then include the migration capability to read in the old table and inject it to storage.

It wasn't something we didn't want to do, but since the code was assumed correct, porting it up directly was the simpler choice.

-- Scott