Skip to content

[#1107] Fall back to memory locking when counter writes fail - #1216

Merged
vharseko merged 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/1107-memory-lockout
Nov 3, 2026
Merged

vharseko merged 3 commits into
OpenIdentityPlatform:masterfrom
vharseko:fix/1107-memory-lockout

Conversation

@vharseko

@vharseko vharseko commented Nov 3, 2026 •

Copy link
Copy Markdown
Member

A failed account-lockout counter write currently stops counting subsequent failures. Preserve the current attempt and switch only the affected identity to server-local memory locking across authentication requests. Keep positive lockout durations; use 30 minutes when the configured duration is zero.

Fallback covers counter increments and resets, retains persisted history, and logs the transition once per identity. Even a transient write failure keeps that identity in fallback until server restart; successful authentication resets the counter without restoring persistence. Counters are per server and are lost on restart. Setting the directory account status to Active or clearing the persisted counter does not clear a fallback lock. Existing physical account restrictions in physical-locking mode remain effective.

Special identities follow the same policy without name-based exemptions. With the current default lockout settings, five incorrect passwords can now temporarily lock amadmin for 30 minutes. Upgrade notes describe these operational changes.

Keep identity keys consistent across aliases and realms. Serialize fallback increments and expiry resets while sending lockout mail and invoking plugins outside the shared monitor. Reuse the resolved identity for expiry resets, and avoid directory lookups in the timed-lock pre-check when lockout is disabled or physical mode has no fallback counters.

Disabling lockout, or setting its failure count or failure interval to zero, stops enforcing temporary locks in both account-lock checks, consistently across authentication index types. This preserves the counters: re-enabling lockout before expiry enforces the existing temporary lock again. Physical account and profile restrictions retain their existing checks.

Lockout mail and plugin callbacks remain covered for healthy writable stores and configured memory counters. Configured memory mode also fixes an existing skipped-callback path: pass the login realm when no identity is resolved, without adding an identity-store lookup. When an identity is available, continue using its realm. Preserve the existing four-argument API.

Two related corrections also apply to writable stores and configured memory mode: threshold 1 starts a timed lock on the first failure, and multiplier 0 preserves the duration for subsequent lock cycles.

Validation:

  • Java 17 Maven reactor build selecting all 236 standard openam-core test classes: 2,327 tests passed, with no failures, errors, or skips, including all 69 lockout cases.
  • Before the latest fixes, 12 disable/re-enable scenarios and the configured-memory callback regression failed for the expected reasons. Coverage includes persisted counters, timed and physical-mode fallback, configured memory, administrative restrictions, notification counts, and callback realm selection.
  • Six isolated mutations were detected in the latest update: skipped persisted notifications, skipped configured-memory notifications, missing disabled-mode enforcement check, bypassed physical restrictions, missing login realm, and incorrect preference for the login realm over a resolved identity's realm.
  • Earlier mutation checks also detected missing increment serialization, missing atomic expiry/reset, the old threshold-one condition, the old zero-multiplier reset, missing duration multiplication, and double multiplication on write failure.
  • Independent final code review found no actionable findings; git diff --check passed.

Fixes #1107

@vharseko vharseko added bug security Security fix or hardening (CVE, GHSA, XSS/CSRF/SSRF) tests Test suite: coverage, fixtures, or test infrastructure java Pull requests that update java code labels Nov 3, 2026
@vharseko
vharseko requested a review from maximthomas November 3, 2026 10:42

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The fallback state is published and updated carefully.

  • ISAccountLockout.useMemoryLocking sets the fallback duration before putIfAbsent publishes it, so a concurrent request never counts against a zero duration.
  • The store-catch retry recomputes from the shared state; removing that synchronized turns simultaneousFirstWriteFailuresPreserveEveryAttempt red 153 of 200 runs.
  • AMAccountLockout.isLockedOut(AMIdentity) checks the administrator's physical lock before the memory counter, pinned by fallbackStillHonorsAnAdministrativelyDisabledAccount.

issue (blocking): For an identity in fallback, the lockout mail and the plugin callback run while the shared per-identity monitor is held.

openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:224-227, :290-295, :309-333

For a fallback identity, invalidPasswd runs recordInvalidPasswd inside synchronized (memoryInfo). At fail_count == failureLockoutCount that block calls sendLockOutNotice, which is a synchronous AMSendMail.postMail, and processedAccounttLockout. AMSendMail's session sets only mail.smtp.host/port (AMSendMail.java:55-56, :144), so no connect or read timeout applies. getAcInfo (:375-380), resetLockoutAttempts (:756-760) and AMAccountLockout.checkMemoryLockout (:332-342) take the same monitor. The trigger needs three conditions: an identity in fallback (every identity on a read-only store, the #1107 case), a lockout-notification address, and an SMTP server that accepts the connection but never answers. Then every later login for that user name, including the AMLoginContext:611 pre-check, parks a container thread on the monitor. With no timeout set, nothing bounds the wait. At BASE no shared lock was held while the mail was sent, so the worst case was one stuck thread per lockout cycle. ISAccountLockout is created per AMAccountLockout (AMAccountLockout.java:99), which is created per request (AMLoginContext.java:610, :673), so an instance field is request-local. inactivateUserAccount (:310-312) can stay where it is: it runs only for identities that are not in fallback. The store-catch arm needs no change, because its synchronized block has ended by the time control returns to invalidPasswd. The invalidPasswd(AMIdentity, AccountLockoutInfo) overload at :486 takes no shared monitor.

private long lockoutEventAt;

public int invalidPasswd(String userDN, String userName, AMIdentity amIdentity, AccountLockoutInfo acInfo) {
    AccountLockoutInfo memoryInfo = memoryLockouts.get(userDN);
    int warning;
    if (memoryInfo != null) {
        synchronized (memoryInfo) {
            warning = recordInvalidPasswd(userDN, userName, amIdentity, memoryInfo, false);
        }
    } else {
        warning = recordInvalidPasswd(userDN, userName, amIdentity, acInfo, storeInvalidAttemptsInDS);
    }
    notifyLockout(userName, amIdentity); // no shared monitor is held here
    return warning;
}

// recordInvalidPasswd, in place of the try block at :313-333:
//     lockoutEventAt = now;

private void notifyLockout(String userName, AMIdentity amIdentity) {
    long at = lockoutEventAt;
    if (at <= 0) {
        return;
    }
    lockoutEventAt = 0;
    try {
        sendLockOutNotice(userName);
        callbackImpl = AMAuthCallBackImpl.getInstance(amIdentity.getRealm());
        callbackImpl.processedAccounttLockout(at, userName);
    } catch (AMAuthCallBackException e) {
        if (debug.getState() >= IDebug.ERROR) {
            debug.error("ISAccountLockout invalidPasswd : error getting callback implementation "
                    + "instance or error from callback module", e);
        }
    } catch (Exception ex) {
        debug.message("Error activating account/sending notification ", ex);
    }
}

question (non-blocking): Was one transient write error on a writable store meant to keep the identity in server-local memory until restart?

openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:340-358, :375-380, :310; openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:317-318

The code never removes an entry from memoryLockouts, and getAcInfo returns that entry before it reads the store. Take physical mode (duration 0) and one transient IdRepoException on a counter or reset write. Until restart, that identity then gets a 30-minute memory lock each time it reaches the threshold, never inetuserstatus=Inactive, so an attacker gets the threshold again every 30 minutes. Setting the user Active or clearing sunAMAuthInvalidAttemptsData no longer lifts the lock. The body names the per-server counters; it does not name either of these effects. If fallback is only for stores that keep rejecting writes, this becomes blocking: a later successful persisted write, or the reset after a successful login, should then remove the entry. Either way, the upgrade notes (chap-upgrade-servers.adoc) should say that an administrator cannot unlock a fallback identity.


question (non-blocking): Should anyone be able to lock amadmin for the lockout duration in the default configuration?

openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:290-295; openam-core/src/main/java/com/sun/identity/authentication/service/AuthD.java:1171-1172; openam-core/src/main/java/com/sun/identity/idm/plugins/internal/SpecialRepo.java:639-645

The counter is written through AuthD.getIdentity, which uses the AdminTokenAction (dsameuser) session. SpecialRepo.setAttributes rejects every token except amadmin's, so amadmin's write always throws. BASE returned -1 and never locked amadmin. HEAD moves amadmin into fallback, so anyone who posts the threshold number of wrong passwords locks it for the lockout duration (30 minutes with the amAuth.xml default). The body's "special identities follow the same fallback policy" seems to intend this, so it is non-blocking as stated. Operators only read the upgrade notes, so put it in chap-upgrade-servers.adoc, not only in the PR body. Not run against a server.


question (non-blocking): Are the threshold-1 and multiplier-0 changes meant for every identity, not only fallback ones?

openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:259, :792

The window conjunct is gone from the lock condition, and the expiry reset skips a 0 multiplier, on every path, including writable stores and configured memory mode. With threshold 1, BASE locked at the second failure and HEAD locks at the first. With multiplier 0, BASE's expiry reset persisted duration 0, so no later cycle ever locked; HEAD keeps the duration. Both look like fixes. However, docs/superpowers/specs/2026-11-03-account-lockout-fallback-design.md:25 and the plan's :23 say that writable stores and explicit memory mode "retain their behavior". If the changes are intended, say so in the body and the upgrade notes, fix those two lines, and pin them (next comment). If not, confine both changes to fallback identities.


suggestion (non-blocking): No test pins the threshold-1 and multiplier-0 changes for identities whose store accepts writes.

openam-core/src/test/java/com/sun/identity/authentication/service/AMAccountLockoutTest.java:156-174; openam-core/src/test/java/com/sun/identity/common/ISAccountLockoutTest.java:314-315

In AMAccountLockoutTest, store() always throws (:74), and ISAccountLockoutTest.lockout() hard-codes count 5 and multiplier 1. Two mutants keep the BASE arithmetic wherever memoryLockouts lacks the userDN, and both leave the suite green (427/427). Both rows below fail at BASE and under both mutants (not run):

private ISAccountLockout lockout(long minutes, int count, int multiplier) {
    return new ISAccountLockout(true, MINUTES.toMillis(5), count, null, 3, null, null,
            MINUTES.toMillis(minutes), multiplier, ATTRIBUTE, "amAuth");
}

@Test
public void writableStoreLocksOnTheFirstFailureWhenThresholdIsOne() {
    rejectWrites = false;
    ISAccountLockout lockout = lockout(10, 1, 1);
    lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
    assertTrue(lockout(10, 1, 1).isLockedOut(lockout(10, 1, 1).getAcInfo(userDN, identity)));
    assertEquals(writes, 1);
}

@Test
public void writableStoreKeepsTheDurationWhenTheMultiplierIsZero() {
    rejectWrites = false;
    for (int i = 0; i < 2; i++) {
        ISAccountLockout lockout = lockout(10, 2, 0);
        lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
    }
    FrozenTimeService.INSTANCE.fastForward(11, MINUTES);
    ISAccountLockout reset = lockout(10, 2, 0);
    reset.resetLockoutAttempts(userDN, identity, reset.getAcInfo(userDN, identity), false);
    assertEquals(lockout(10, 2, 0).getAcInfo(userDN, identity).getActualLockoutDuration(), MINUTES.toMillis(10));
}

suggestion (non-blocking): No test pins the synchronized (memoryInfo) around fallback increments.

openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:224-227; openam-core/src/test/java/com/sun/identity/common/ISAccountLockoutTest.java:200-226

staleSnapshotsDoNotLoseConcurrentFallbackAttempts publishes the fallback entry before its threads start, so it depends on four short read-modify-writes colliding. With the synchronized removed, the row survived all 200 invocations.

Pin: make the collision deterministic. ISAccountLockout.debug is already a mock; stub its messageEnabled() to park only the first worker thread on a latch (it is called between reading fail_count and writing it back). Then start a second invalidPasswd on the same identity, wait about 200 ms, release the latch, and assert that the count grew by 2. Without the monitor, the count grows by 1.


suggestion (non-blocking): No test row uses a lockout multiplier above 1.

openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:251, :259-263

The PR moves the multiplied duration into the local actualLockoutDuration, which keeps the store-catch retry from multiplying twice. Every test row uses multiplier 1 or 0, so deleting :261 leaves the suite green (427/427), and so does going back to mutating acInfo before the store. This row, which uses the helper above, fails on both mutants: it gets 10 minutes without :261 and 40 with the pre-store mutation (not run).

@Test
public void multiplierAppliesOnceWhenTheThresholdWriteFails() {
    rejectWrites = false;
    for (int i = 0; i < 5; i++) {
        ISAccountLockout lockout = lockout(10, 5, 2);
        lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
    }
    FrozenTimeService.INSTANCE.fastForward(11, MINUTES);
    for (int i = 0; i < 4; i++) {
        ISAccountLockout lockout = lockout(10, 5, 2);
        lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
    }
    rejectWrites = true;
    ISAccountLockout lockout = lockout(10, 5, 2);
    lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
    assertEquals(lockout(10, 5, 2).getAcInfo(userDN, identity).getActualLockoutDuration(), MINUTES.toMillis(20));
}

suggestion (non-blocking): checkMemoryLockout holds the shared monitor during resetPasswdLockout's directory lookup.

openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:332-342, :194

On an expired fallback lock, resetPasswdLockout calls AuthD.getAuth().getIdentity inside synchronized (acInfo), and acInfo here is the shared memoryInfo. Once per lock cycle, every other request for that identity waits on a lookup against the very store whose failures caused the fallback. Resolve amIdentity and the key before you take the monitor; isMemoryLockout and isAccountLocked(String) already have both. Then, under the monitor, call isAccountLockout.resetLockoutAttempts(userDN, amIdentity, acInfo, false) and set warnUser = 0. The key must be the one resetPasswdLockout computes: normalizeDN(token) when storeInDS is false. delayedExpiryResetCannotEraseANewLockoutCycle swallows its TimeoutException, so it passes either way.


suggestion (non-blocking): isLockedOut(String) now reads the directory on every login in realms with lockout duration 0.

openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:246

memoryLocking is true whenever the duration is above 0 (ISAccountLockout.java:151-152), so the default configuration already paid for this lookup at BASE. Realms with duration 0 and sunStoreInvalidAttemptsInDS at its default (true) now pay one AuthD.getIdentity and one attribute read per AMLoginContext:611 check and per LoginState lockout call. The result is still false unless the identity is in fallback. Skip the lookup when lockout is disabled, and when no identity is in fallback (a static check that memoryLockouts is non-empty).


suggestion (non-blocking): No AMAccountLockout test runs in configured memory mode.

openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:246, :317-318

Every AMAccountLockoutTest row sets storeInDS=true and makes store() throw. Two mutants therefore survive: one reduces :246 to getStoreInvalidAttemptsInDS(), so a locked memory-mode user with the right password gets in; the other adds && getStoreInvalidAttemptsInDS() at :317. This row kills both (not run). The fresh name keeps the static loginFailHash entry out of other rows:

@Test
public void configuredMemoryModeLocksWithoutTheStore() {
    when(state.getLoginFailureLockoutStoreInDS()).thenReturn(false);
    when(state.getLoginFailureLockoutDuration()).thenReturn(MINUTES.toMillis(10));
    String name = "memory-" + UUID.randomUUID();
    for (int i = 0; i < 5; i++) {
        new AMAccountLockout(state).invalidPasswd(name);
    }
    assertTrue(new AMAccountLockout(state).isLockedOut(name));
    assertTrue(new AMAccountLockout(state).isAccountLocked(name));
}

suggestion (non-blocking): No test pins the design doc's rule that "a read failure alone does not activate fallback".

openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:393-395

The code is right. However, the only row with a failing read also rejects the write. A mutant that publishes the fresh acInfo in recordInvalidPasswd's null branch, and still writes, stays green. This row kills it (not run):

@Test
public void readFailureAloneKeepsPersisting() throws Exception {
    rejectWrites = false;
    when(identity.getAttribute(anyString()))
            .thenThrow(new IdRepoException("Unavailable"))
            .thenAnswer(call -> stored.get(call.getArgument(0)));
    failAttempt(10);
    failAttempt(10);
    assertEquals(writes, 2);
}

suggestion (non-blocking): The physical-lock rows run only with duration 0, so no test pins the timed-mode branch at :311.

openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:311

fallbackStillHonorsACustomPhysicalLock and fallbackStillHonorsAnAdministrativelyDisabledAccount use duration 0. Every timed-mode row uses an active identity, so changing :311 to always run the physical check survives.

Pin: add a row with duration above 0 and identity.isActive() false. Fail five times, let the lock expire with FrozenTimeService.fastForward, and assert the isAccountLocked("alice") outcome you intend (false is BASE parity).


note (non-blocking): Files outside the stated scope; the body does not mention them.

  • docs/superpowers/plans/2026-11-03-account-lockout-fallback.md — agent-workflow plan
  • docs/superpowers/specs/2026-11-03-account-lockout-fallback-design.md — agent-workflow spec

@vharseko
vharseko force-pushed the fix/1107-memory-lockout branch from 7e8a342 to b5518f4 Compare November 3, 2026 12:35
@vharseko

vharseko commented Nov 3, 2026

Copy link
Copy Markdown
Member Author

Addressed the review in this update.

  • Lockout mail and the plugin callback now run after the shared counter monitor is released, including when the threshold attempt triggers the first failed-write transition. Four regression cases block either notification stage, on an existing fallback or during transition, and verify that another request can read the lock before notification completes. They also check that a subsequent attempt does not duplicate the notification.
  • Expiry reset reuses the already resolved identity and counter key, preserving the configured-memory key and the atomic expiry/reset operation. A regression rejects directory lookups under the fallback monitor. Lock checks also skip directory access when lockout is disabled, or when physical mode has no fallback identities.
  • Fallback until restart is intentional, including after a transient write failure. A successful login resets the counter without restoring persistence. Upgrade notes and the PR description now explicitly explain the physical-to-temporary-lock transition, per-server state, and why setting the directory status to Active or clearing the stored counter does not clear an existing fallback lock.
  • Special identities intentionally follow the same policy. The upgrade notes now call out that the current defaults allow five incorrect passwords to lock amadmin for 30 minutes on the affected server.
  • The threshold-one and zero-multiplier corrections intentionally apply to writable stores and explicitly configured memory mode too. Both are documented and tested, including a subsequent lock cycle. The threshold-one correction concerns timed locking; physical deactivation already ran at a failure count of one.
  • The increment regression now pauses after reading the counter and waits for actual monitor contention or completion of the competing attempt. The expiry regression pauses between its decision and reset, independently of directory lookups. Removing either corresponding monitor makes its regression fail.
  • Added coverage for a multiplier of two when the threshold write fails, a read failure followed by successful writes, configured memory mode through AMAccountLockout, and timed-mode checks for an inactive identity. The last case preserves this helper's existing timed-mode behavior; profile validation remains separate.
  • Removed the workflow plan and specification from the PR. The operational behavior is documented in the upgrade guide instead.

Validation on Java 17: all 236 standard openam-core test classes passed, with 2,302 tests and no failures, errors, or skips, including all 44 lockout cases. Before the fixes, seven new cases failed for the expected notification/lookup/mode problems. Six isolated mutations were detected: missing increment serialization, missing atomic expiry/reset, the old threshold-one condition, the old zero-multiplier reset, missing duration multiplication, and pre-store multiplication that doubles the duration on retry. git diff --check also passed.

@vharseko
vharseko requested a review from maximthomas November 3, 2026 12:36
@vharseko vharseko added documentation Documentation, README, or javadoc concurrency Thread safety, races and synchronization performance Performance, memory, or resource-leak fix labels Nov 3, 2026

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The round-1 blocker is fixed, and the new regression cases are real pins.

  • Lockout mail and the plugin callback now run in notifyLockout after the counter monitor is released (ISAccountLockout.java:335-353).
  • The seven cases the description names fail against the previous commit's main code, and the other 37 pass (measured: 44 run, 7 failed). Those seven are the four blockedNotificationDoesNotBlockCounterReaders rows plus expiryResetReusesResolvedIdentityOutsideTheCounterMonitor, physicalModeWithoutFallbackDoesNotReadTheDirectory and disabledLockoutDoesNotReadTheDirectoryEvenWithFallback.
  • The failure path checks the lock and counts the attempt under the same normalizeDN(IdentityUtils.getDN(..)) key (AMAccountLockout.java:121-124, :302-305).

question (non-blocking): When lockout is disabled, should an existing lock stop being enforced? Right now the answer depends on the auth index type.

openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:242-244, :321

The early return added in this round gates isLockedOut(String) only. isAccountLocked(String) still runs checkMemoryLockout, and no caller gates it (AMLoginContext.java:611, :674, LoginState.java:3915).

Take a lockout that is turned off (mode off, or count 0) while a timed DS lock or a fallback lock has not yet expired:

  • A correct-password login with authIndexType=user now succeeds, because AMLoginContext:611 is its only lock check. Before this PR it was rejected.
  • The same login through the default org/service chain is still rejected at LoginState.java:2554 → isAccountLocked.
  • A wrong password still reports the user as locked.

The early return implements the previous round's suggestion ("skip the lookup when lockout is disabled"). That suggestion did not say the result would change.

This is minor either way, since nothing is throttled once lockout is off. It becomes major only if a lock must outlive disabling lockout, because then the USER-index path regressed.

If locks are meant to be released, gate the memory branch of isAccountLocked(String) too. Leave the physical branch (:315-320) as it is, so an administrator's inactivation still holds:

            if (!locked && acInfo != null && isAccountLockout.isLockoutEnabled()
                    && isAccountLockout.isMemoryLocking(userDN)) {
                locked = checkMemoryLockout(userDN, amIdentity);
            }

Or, if locks must outlive disabling lockout: drop the early return at :242-244 and rely on the hasMemoryLockouts() gate at :249-250 to avoid the lookup.


suggestion (non-blocking): No test checks that lockout mail and the plugin callback still fire when the threshold write succeeds.

openam-core/src/test/java/com/sun/identity/common/ISAccountLockoutTest.java:333-367, openam-core/src/main/java/com/sun/identity/common/ISAccountLockout.java:328

Only the four rows of blockedNotificationDoesNotBlockCounterReaders verify sendLockOutNotice and processedAccounttLockout (:362-363). Each row sets rejectWrites = true before the threshold attempt (:337), so the threshold is always reached on a persist=false path.

If lockoutEventAt = now (ISAccountLockout.java:328) ran only when !persist, both lockout test classes would stay green. Yet that change stops the mail and the plugin event for every healthy writable store, which is the normal production path. This was traced, not run as a mutant.

    @Test
    public void healthyWriteStillSendsTheLockoutNotification() {
        rejectWrites = false;
        ISAccountLockout lockout = spy(lockout(10));
        doAnswer(call -> null).when(lockout).sendLockOutNotice("alice");
        AMAuthCallBackImpl plugin = AMAuthCallBackImpl.getInstance("/");
        for (int i = 0; i < 5; i++) {
            lockout.invalidPasswd(userDN, "alice", identity, lockout.getAcInfo(userDN, identity));
        }
        assertTrue(lockout.isLockedOut(lockout.getAcInfo(userDN, identity)));
        verify(lockout, times(1)).sendLockOutNotice("alice");
        verify(plugin, times(1)).processedAccounttLockout(any(Long.class), eq("alice"));
    }

Pin: this row fails if the :328 assignment is skipped on the persisted path. Configured memory mode (storeInvalidAttemptsInDS=false) has no notification row either, and that is the other half.

@vharseko

vharseko commented Nov 3, 2026

Copy link
Copy Markdown
Member Author

Addressed both follow-up points.

  • Disabling lockout now stops enforcing temporary locks in both isLockedOut and the memory branch of isAccountLocked. This applies when the mode is off, the failure count is zero, or the failure interval is zero. The physical branch remains unchanged. Tests cover all three settings with persisted counters, timed fallback, physical-mode fallback, and configured memory counters, plus administrative and custom physical restrictions. Disabling does not clear the counter or restore persistence: re-enabling before expiry enforces the existing temporary lock again. The upgrade notes document that policy.
  • Added notification coverage for healthy writable stores in both timed and physical mode, and for configured memory counters. Each case verifies no notification before the threshold, exactly one mail call and plugin callback at the threshold, and no duplicate on the next attempt. The callback timestamp is checked as well.
  • Testing the actual AMAccountLockout configured-memory path also exposed an existing issue: it passes a null identity, so dereferencing amIdentity.getRealm() skipped the plugin callback. The helper now accepts the login realm for that path without resolving an identity. The existing four-argument API is preserved, and a resolved identity still supplies its own realm. Tests cover the root realm, a subrealm, an organization DN, and the identity-realm precedence. Notifications remain outside the counter monitors.

Validation on Java 17: all 236 standard openam-core test classes passed, with 2,327 tests and no failures, errors, or skips, including all 69 lockout cases. Before the fixes, 12 disabled-lockout cases and the configured-memory callback case failed as expected. Six isolated mutations were detected: skipped persisted notifications, skipped configured-memory notifications, missing disabled-mode check, bypassed physical restrictions, missing login realm, and incorrect callback-realm precedence. Independent final review found no actionable findings, and git diff --check passed.

@vharseko
vharseko requested a review from maximthomas November 3, 2026 14:00

@maximthomas maximthomas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

praise: The follow-up settles both round-2 points, and the tests for them fail on the previous head.

  • AMAccountLockout.isAccountLocked now checks isLockoutEnabled() before its memory branch, so turning the mode off or setting the count or interval to 0 stops the temporary lock in both checks. All 12 disablingLockoutStopsBothTimedChecksWithoutClearingTheCounter rows fail when run against b5518f4's main code.
  • ISAccountLockout.notifyLockout falls back to the login realm (openam-core/src/main/java/com/sun/identity/authentication/service/AMAccountLockout.java:130 passes loginState.getOrgDN()). Configured in-memory counters now reach the callback plugins instead of failing on a null identity. The 3 configuredMemoryModeNotifiesTheLoginRealmWithoutResolvingAnIdentity rows fail on b5518f4.
  • Both policy changes are documented in chap-upgrade-servers.adoc.

@vharseko
vharseko merged commit 2a80c16 into OpenIdentityPlatform:master Nov 3, 2026
16 checks passed
@vharseko
vharseko deleted the fix/1107-memory-lockout branch November 3, 2026 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug concurrency Thread safety, races and synchronization documentation Documentation, README, or javadoc java Pull requests that update java code performance Performance, memory, or resource-leak fix security Security fix or hardening (CVE, GHSA, XSS/CSRF/SSRF) tests Test suite: coverage, fixtures, or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Account lockout stops counting instead of falling back to memory when the identity store cannot hold the counter

2 participants