external storage locks user in AD when password has changed in AD

[blizzz]

Steps to reproduce

1. Have LDAP/AD configured as well as user specific SMB share with AD credentials (saved in DB)
2. Log in at least once, close the browser
3. Change the password in AD
4. Let cron jobs do their things

Expected behaviour

• Have only one attempt that ends up with unauthenticated, or introduce a big delay

Actual behaviour

• The storage is marked unavailable (for hard 10 minutes)
• The next attempt to authenticate follows
• dependent on the policy on AD a lock will follow

Options

Let me state first state that this mechanism follows not a modern approach but sadly is still adopted in many places.

Background: on an attempt with an invalid password, SMB Storage throws a ConnectError with "Invalid login"

• The workaround: configurable RECHECK_TTL_SEC in config.php – the hard 10 minutes could be increased by admins
• The other timestamp: extended the database and setting when the storage is available again. On certain types of errors, it could be more than the 10 minutes, e.g. 8 hours for this type of exception. Still a static time, but probably sufficient. It is required to extend the database table however. Still can include the config.php option if necessary.
• The full blown: like before, with another column that counts "bad attempts" so that the time interval increases each time.
• The awkward workaround: sets the time checked timestamp to the future on ConnectError exceptions to achieve the same result as in the second option. The advantage is that no DB changes are needed, but comes with ugliness.

Better backportability comes without DB changes, espcially if they can be time costly. Therefore, as bad as it sounds, the last option has a good balance of flexibility and complexity. What do you think @icewind1991

[blizzz]

I was investigating the code to have a workaround ready. My idea was to implement stat in the SMB storage and set a delay in cause of an AuthenticationException thrown by the smb lib.

Further, I' had the Common storage pass this delay to the Caches setAvailability method, where it would add the delay to time() before writing it to the DB.

Not too difficult, and only touches private implementations. Then I tried it with php-smbclient (on Arch btw) and this behaves very different. The auth error is triggered later and not handled at all, circumventing the availability mechanics.

So, need to look for a way to catch this properly as well. Messy thing.

Since @icewind1991 is away, maybe @rullzer or @juliushaertl have an opinion/feedback on this whole matter in general? I do not like those options listed in the opening much.

[icewind1991]

We could use the status codes in StorageNotAvailableException to have the AvailabilityWrapper set different recheck intervals for authentication (and other) errors.

For backports we could set last_checked to a future time, for nc18 change last_checked into next_check

[blizzz]

We could use the status codes in StorageNotAvailableException to have the AvailabilityWrapper set different recheck intervals for authentication (and other) errors.

It already (often) carries the error code of the previous exception and they are not aligned.

Not too difficult, and only touches private implementations. Then I tried it with php-smbclient (on Arch btw) and this behaves very different. The auth error is triggered later and not handled at all, circumventing the availability mechanics.

That's the obstacle, but it seems fine to catch it by also catching Icewind\SMB\Exception\ForbiddenException, check its error code and turn it into an StorageNotAvailableException on the SMB storage.

for nc18 change last_checked into next_check

will this DB structure change be safe on big setups?

[blizzz]

for nc18 change last_checked into next_check

will this DB structure change be safe on big setups?

answering myself, col renames shift be swift, regardless of size.