[QA] admin user gets disabled, when user limit is reached

jnweiger commented 3 years ago

Seen with ownCloud for QNAP 10.8.0 from ownCloud_10.8.0_arm-x41.qpkg (dated 2021-07-27)

Intentionally enabled 7 users while only a 5 user license was active

A minute later the admin user got logged out :-(

root@e642b8e5209a: /var/www/owncloud # occ user:list -a enabled -a lastLogin -a email
  - admin:
    - enabled: false
    - lastLogin: 1627400695
    - email: qnap-admin@example.com
  - jw@owncloud.com:
    - enabled: true
    - lastLogin: 1627392401
    - email: jw@owncloud.com
  - user1:
    - enabled: true
    - lastLogin: 0
    - email: user1@ex.hop
  - user2:
    - enabled: false
    - lastLogin: 0
    - email: user2@ex.mail
  - user3:
    - enabled: true
    - lastLogin: 1627342138
    - email: user3@mail.org
  - user4:
    - enabled: true
    - lastLogin: 1627342197
    - email: user4@mail.org
  - user5:
    - enabled: true
    - lastLogin: 0
    - email: user5@mail.com
  - user6:
    - enabled: true
    - lastLogin: 1627342285
    - email: u6@ex

jnweiger commented 3 years ago

This only happend once so far. Earlier tests disabled only normal users...

jnweiger commented 3 years ago

Not reproducable. We invested some time and brainstorming to recreate the exact conditions when that happend. It does not happen now. Sorry.

wkloucek commented 3 years ago

I will have look. Normally the used function should give the users in a defined order, but I will inspect the query and read about array iterations in php (these are the two points which could behave non-deterministic).

jnweiger commented 3 years ago

The situation was roughly like this: The last user in the list (sorted alphabetcally) got manually re-enabled after two others got manually enabled (where only one would fit). This user should have been disabled, but instead the first user (admin) got disabled. Could also be an off-by-one with wraparound?

As a safety-net: Can we give admin users a lower prio for disabling?

jnweiger commented 3 years ago

Reproduced once more:

login as admin with the web ui
log in as user5 with the web ui (using a different browser session)
connect a desktop client as user5
wait until sync finishes
admin disables user5, then disables users4 and user3; then he re-enables user5
desktop client immediately pops up a password requester (apparently due to the brief disable)
immediatly log in the desktop client again. It succeeds a sync run.
a few seconds later admin gets logged out.
the user table is

# occ user:list -a enabled
  - admin: false
  - jw@owncloud.com: true
  - me@myself.home: true
  - user1: true
  - user2: true
  - user3: false
  - user4: false
  - user5: true
  - user6: true
  - user7: true

Nothing in the logs.

wkloucek commented 3 years ago

During developing I saw an order by statement in the source code and was satisfied... Must have overlooked that conditional....

https://github.com/owncloud/core/blob/32b9b60c3e7c80d88273795ff93a31794ca5942f/lib/private/User/AccountMapper.php#L236-L238

jnweiger commented 3 years ago

Retested with https://github.com/owncloud/qnap-packaging/releases/tag/v10.8.0.0-rc3

I now have a reliable reproducer to disable admin:

have a 5 user license
log in as admin through the web ui
create users 007-alice, 007-bob, 007-carol, 007-dave, 007-james
wait 1 min,
admin gets logged out, his account is disabled.

Disabling users that sort alphabetically last is not good enough. LDAP users often have identifiers that start with a number.

Expected behaviour: the last member of admin group should never be disabled.

jnweiger commented 3 years ago

Unrelated to this issue, but nicely confirmed while testing this situation: Guest users (invited via email and public link) are never disabled due to license limitations. OK.

wkloucek commented 3 years ago

Retested with https://github.com/owncloud/qnap-packaging/releases/tag/v10.8.0.0-rc3

I now have a reliable reproducer to disable admin:

have a 5 user license

log in as admin through the web ui

create users 007-alice, 007-bob, 007-carol, 007-dave, 007-james

wait 1 min,

admin gets logged out, his account is disabled.

Disabling users that sort alphabetically last is not good enough. LDAP users often have identifiers that start with a number.

Expected behaviour: the last member of admin group should never be disabled.

The sort statement in https://github.com/owncloud/core/blob/32b9b60c3e7c80d88273795ff93a31794ca5942f/lib/private/User/AccountMapper.php#L236-L238 uses the userid = username.... I thought it was a numeric id...

https://github.com/owncloud/qnap/pull/42 now disables admins only if needed (eg. if only admins are there and they exceed the license limit...). That means admin users will win always over non admin users. Besides that an 007-admin will win over an 001-admin and stay active.

wkloucek commented 3 years ago

@jnweiger I updated the ownCloud QPKG on the AMD64 device to https://github.com/owncloud/qnap-packaging/releases/tag/v10.8.0.0-rc4 which includes the fix... Could you try again? :-)

jnweiger commented 3 years ago

Seen with this RC4:

That is still to be expected, right?

wkloucek commented 3 years ago

Seen with this RC4:

That is still to be expected, right?

Yes, the QPKG is still not signed. Will be hopefully done until the end of the week :-)

jnweiger commented 3 years ago

Confirmed fixed with ownCloud_10.8.0.0-r_x86_64.qpkg from https://github.com/owncloud/qnap-packaging/releases/tag/v10.8.0.0-rc4

owncloud / qnap

[QA] admin user gets disabled, when user limit is reached #33