Bug 60202

Given the way that the LockOutRealm works (as a Realm wrapper with no insight into the inner workings of the Realm it wraps) there is no easy way to fix this as it has no way to dertermine why an authentication has failed.

Additionally, most LDAP services implement their own lock out policy for failed passwords so use of the LockOutRealm may not be necessary in this case.

Perhaps the Realm interface could be adjusted to throw a variety of exceptions:

- AuthenticationException (triggers lock-out)
- CommunicationException (does not trigger lock-out)

Honestly, if a Realm throws an exception during authentication, I would think that should always be considered a transient failure and NOT trigger lock-out.

If the LDAPRealm is returning <false> when a communications exception occurs, I think it should instead throw an exception.

The realm doesn't and shouldn't throw exceptions at all :) Look at the lock out realm code.

(In reply to Remy Maucherat from comment #3)
> The realm doesn't and shouldn't throw exceptions at all :) Look at the lock
> out realm code.

How should the Realm behave if there is a communications error? I assert that throwing an exception is a perfectly reasonable thing to do.

The current Realm code is irrelevant. It will behave however we want it to behave. I'm suggesting a change to the behavior, not asking why it doesn't behave in a way that it wasn't programmed to do.

It's not LockOutRealm that you need to read, it's LDAPRealm, which shows that it catches exceptions like CommunicationException (a transient, non-authentication-related problem) and returns null which makes it look the same as an authentication problem, which is isn't.

So I want it to behave the way it does, mostly, and especially not fail requests like that.
-1

I agree with Christopher here. Possibly this is a two-part fix, one to adjust the LDAPRealm to treat connection failures differently than authentication failures, and one to allow the LockoutRealm to know the difference.

This is not useful, we can understand all you care about is having your issue fixed, regardless of the collateral damage. Exception propagation is clearly not the right way to do it, since the request should go on without an authenticated user and then generate a response according to the security constraints (if any).

Something that could be possible: return a singleton token principal in case of some failure that is not an auth failure. The downside is that it's a hack that is risky, and I'm not convinced it is worth trying.

I'm not asking for a hack fix and I'm not asking for a fix that might break something down the line. I'm asking for an improvement that will make the application work better implemented in whatever way the development community decides is best for the project.

It's not magical and at this point the thinking is that we have to change something and it would be problematic.

Something that could work better is having a "Realm.isUp" method or something like this. That's not quite the way this realm is designed to work with its connection at the moment.

Remy, I just want to take the opportunity to point out that, in *this* bug you claim that throwing an IOException (essentially what is happening, here) would be some kind of major moral violation yet in bug #59876 you argue that "hey, sometimes IOExceptions happen and you just have to deal with them".

It sounds like your position is that code should never change after its been written.

-1 to your attitude :(

I'm reopening this bug for further discussion.

I am not the one who closed this report in the first place, hence my -1 is seconded. You don't need to reopen a bug to have further discussion as well.

Your argument on exceptions doesn't make sense. In realms, the authentication must look as if it fails to see what the request will do, regardless on what happens in the realm. Elsewhere, exception are propagated. Async, as in the other report, is something else, the user component receives the exception and can do anything with it (so it is an opportunity for processing, logging, etc).

Created attachment 34352 [details]
isUp idea from comment 9

Using LifecycleState.FAILED is also a possibility but it could have side effects so I didn't try it.

Created attachment 34396 [details]
v2

I still had the changes locally, so I added the checks to the other similar realms. It's not accurate for a transient connection failure, but it should be good enough since the only consumer of this is the lockout realm.

That looks like a pretty elegant solution. How would it interact with third-party realms?

(In reply to Ben from comment #14)
> That looks like a pretty elegant solution. How would it interact with
> third-party realms?

A third party realm would have to implement the new API method.

(In reply to Remy Maucherat from comment #15)
> (In reply to Ben from comment #14)
> > That looks like a pretty elegant solution. How would it interact with
> > third-party realms?
> 
> A third party realm would have to implement the new API method.

Last question (bear with me since I'm not a developer):
Am I correct that the default isAvailable() you added in Realm.java would return true if a 3rd-party realm did not have its own isAvailable() method?

If so, this looks like minimal fallout and a good solution to my problem. I'll defer to the community on whether to accept the change but it has my vote.

I included the change for 8.5.9 and 9.0.0.M14. I don't plan to backport to earlier versions since it's an API change.