IdPv3: race condition causes two persistentId to be generated

[Cantor, Scott]

On 9/24/15, 12:48 PM, "users on behalf of Dominique Petitpierre" <users-bounces at shibboleth.net on behalf of Dominique.Petitpierre at unige.ch> wrote:
>
>OK. We can live with duplicate entries (tolerant database + a nightly 
>batch will remove them), we are happy with computed persistentIds and we 
>don't really need persistentId deactivation and regeneration (if ever, 
>we'll do that "by hand").

You can apply a constraint any time you want to the database, that's not a code change, but if you're trying to avoid it generating errors, that wouldn't help obviously.

>So, another suggestion is to have a simple boolean property that would 
>control if one wants to disable the regeneration logic in 
>StoredPersistentIdGenerationStrategy.java.
>We would disable it and thus never store multiple different 
>persistentIds for the same keys, just duplicates.

I don't think we could fully understand the implications of making these "simple" changes. The one you originally suggested isn't correct (I looked at it and it wouldn't work at all, race condition aside), which illustrates how deceptive it is to try and fix it with a simple change. This is a hard concurrency problem. It will still be one if we move things over to the storage API, it's just that we have existing patterns for how to handle that.

If you really don't want it to generate random IDs, the best answer would be to use the computed ID connector to begin with, actually. The ability to generate new ones and change them if needed is pretty much the core feature of the database option (I guess there's the reversibility, but that's pretty minor in practice).

>Maybe Shibboleth IdP could integrate one chosen noSQL storage library or
>other key-value system simpler than a RDBMS to handle it's own rather 
>simple persistent data (like persistentId or user consent).

Using any single choice would be a dead end, people will always complain about specific choices we make.

But there's no such thing as "simple" persistence. Non-relational options simply move the complexity into the application code. They're simpler, but the application isn't. The complexity will just be coded into the storage plugin, and add to the amount of code we have to maintain. The advantage of JDBC is that defining a few error messages to it is usually enough to get it to behave adequately, and there's just one plugin to maintain.

The only reason we left this one an outlier was we thought it was working. The best thing we can do now is just code up a new generator strategy that uses the storage API and people that need to fix the race condition can migrate to that. Much more bang for the buck from that choice.

>Well, the way it works now in IdPv3 is good enough for us. Please don't 
>break it!

If it's working, you just have very tolerantly configured browsers. We're not going to do anything to break it further, it's already broken unless you dictate people run with browsers open to trackers. Point me at a URL and I guarantee my browser won't handle it.

>In IdP 2.4.x we used code developed at Humboldt-Universitaet zu Berlin 
>that moved the login context value into the URL. But we'd rather not 
>take such a route again!

Essentially the same idea, yes. What used to be the login context is in the container session now, so it's the same problem, just a different cookie.

>Thanks again for the very detailed answers!

Thank you for reporting the bug. I'm sorry it wasn't reported originally, or we wouldn't have ported broken code over.

-- Scott