Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dont use preferences table quota any longer #32731

Closed
wants to merge 1 commit into from

Conversation

@tomneedham
Copy link
Member

commented Sep 17, 2018

When quota is synced for an account (on login, and on user:sync), it is read from the user backend.

If the backend doesn't provide a quota, or, if it provides null - core (since 10.0.9 b62cad6) will bring in the quota from oc_preferences which is where it was stored before oc_accounts.

The problem comes now because the user_management app sets the quota directly on the accounts table - meaning we have two sources of truth for the quota value. If users have manually assigned quotas from their admin, these will have been stored in the accounts table. These will be overwritten with the oc_preferences values on user sync (login, or user:sync).

This PR removes the logic from the SyncService to take the quota from the oc_preferences table - aligning the ownCLoud built in user management to fully utilise the oc_accounts table as the source of truth.

However, we now must consider the migration path. There can still be scenarios where the value in oc_preferences is the correct source of the quota value. Vice-versa, if the quota is changed in the UI, this value will have been set in the oc_accounts table and this should be the value used in future.

Migration proposal:

  • If quota is only in oc_preferences, use this, assign to oc_accounts, delete row from oc_preference - current behaviour
  • If quota is in oc_preferences and oc_acccounts and value is the same, delete row from oc_preference - cleanup old row
  • If quota is in oc_preferences and oc_accounts and the value is different, use oc_accounts value,delete row from oc_preference - as it will have been assigned manually sign 10 upgrade

This should result in cleaned quotas from oc_preferences, and the correctly values present in oc_accounts - and future modifications will be persisted correctly.

Scenarios to consider:

  • upgrade from 9.1 to 10 with ldap
  • direct install to 10.0.10 with this patch
  • setting quota from UI whilst using ldap

@tomneedham tomneedham added this to the development milestone Sep 17, 2018

@tomneedham tomneedham self-assigned this Sep 17, 2018

@tomneedham tomneedham requested review from PVince81, butonic and patrickjahns Sep 17, 2018

@PVince81 PVince81 requested review from jvillafanez and DeepDiver1975 Sep 17, 2018

@tomneedham

This comment has been minimized.

Copy link
Member Author

commented Sep 17, 2018

The scenario in which you will lose manually assigned quotas is as follows:

  • You are running owncloud 10.0.9/10
  • The same instance was previously on <10
  • A user has no quota configured (eg: ldap quota attribute empty, or backend provides no quota)

In this scenario, the old quota will be restored on login, or user:sync.

@jvillafanez

This comment has been minimized.

Copy link
Member

commented Sep 18, 2018

Should we include additional cases?

  • If a quota is synced and then deleted in the backend, what is the expected ownCloud quota? should it be the ownCloud's default? should it keep the current quota?
  • How can we reset the account's quota to the ownCloud's default?

The migration proposal looks good 👍

I think we still can't mark the value as "modified by the admin" or something similar so the quota isn't changed in the next user sync. I mean, sync the quota, and then the admin change it to a different one. I'd expect the quota set by the admin to be kep after the next user sync

@PVince81

This comment has been minimized.

Copy link
Member

commented Sep 19, 2018

In a regular OC upgrade from OC < 10 there are already migrations that will move the quota from oc_preferences to oc_accounts then delete them from oc_preferences. So not sure in what scenario such values would still exist.

Despite this, I agree that we should remove any code that still relies on such values existing.

@tomneedham

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

I think we still can't mark the value as "modified by the admin" or something similar so the quota isn't changed in the next user sync. I mean, sync the quota, and then the admin change it to a different one. I'd expect the quota set by the admin to be kep after the next user sync

I agree. IMO if the backend supplies the quota, you should not be able to change this assignment in owncloud usermanagement - otherwise we have some mismatch of source of metadata. @pmaier1 for how we want this to work?

@PVince81

This comment has been minimized.

Copy link
Member

commented Sep 24, 2018

@tomneedham @jvillafanez wasn't it so in past major versions that one could override the LDAP quota by setting one in user management ?

@jvillafanez

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

@tomneedham @jvillafanez wasn't it so in past major versions that one could override the LDAP quota by setting one in user management ?

I think it's still possible, but resyncing the backend (on login too?) will reset the value again.

I agree. IMO if the backend supplies the quota, you should not be able to change this assignment in owncloud usermanagement - otherwise we have some mismatch of source of metadata. @pmaier1 for how we want this to work?

As long as the admin knows that the quota is different than the one provided by the backend, it should be fine. Blocking the option should be easy though.

@PVince81

This comment has been minimized.

Copy link
Member

commented Oct 9, 2018

after discussing with @cdamken and @pako81 we thought that LDAP should always have authority.

this means that if LDAP is enabled we should prevent any modification of fields email, avatar, display name or quota if an LDAP attribute is specified for such fields in the config. If no field is specified, allow the admin to set values in the users page.

if we agree on this we can raise a new ticket to make the necessary changes for blocking input for all these fields in the users page, provisioning API and occ commands.

@phil-davis phil-davis referenced this pull request Oct 9, 2018

Closed

Add occ command acceptance tests #292

4 of 4 tasks complete
@PVince81

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

any objections to my proposal ? would make a ticket and have it handled separately

@PVince81

This comment has been minimized.

Copy link
Member

commented Oct 15, 2018

raised here: #33186

@tomneedham

This comment has been minimized.

Copy link
Member Author

commented Dec 7, 2018

this means that if LDAP is enabled we should prevent any modification of fields email, avatar, display name or quota if an LDAP attribute is specified for such fields in the config. If no field is specified, allow the admin to set values in the users page.

Yes - the admins should not be using owncloud as user administration if they are using ldap - this gives two sources of truth

@butonic

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

no. not for quota. some admins use the default quote we can configure for ldap users and later increase it manually. I guess the proper solution would be to be able to configure a quota per group. furthermore keep in mind that db users still need to be editable.

@tomneedham

This comment has been minimized.

Copy link
Member Author

commented Dec 13, 2018

  • Cleanup quotas based on the migration proposal in original post
  • Block UI editing of quota if set by backend
@PVince81

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

@ownclouders rebase

@ownclouders

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

Hey! I'm GitMate.io! This pull request is being rebased automatically. Please DO NOT push while rebase is in progress or your changes would be lost permanently ⚠️

@PVince81
Copy link
Member

left a comment

👍 we should stop to use preference tables

@ownclouders ownclouders force-pushed the quota-from-backend branch from 10054c0 to 71952f0 Dec 13, 2018

@codecov

This comment has been minimized.

Copy link

commented Dec 13, 2018

Codecov Report

Merging #32731 into master will decrease coverage by 16.52%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #32731       +/-   ##
=============================================
- Coverage     64.42%   47.89%   -16.53%     
=============================================
  Files          1197      109     -1088     
  Lines         69264    10445    -58819     
  Branches       1276     1270        -6     
=============================================
- Hits          44625     5003    -39622     
+ Misses        24267     5072    -19195     
+ Partials        372      370        -2
Flag Coverage Δ Complexity Δ
#javascript 52.89% <ø> (-0.09%) 0 <ø> (ø)
#phpunit 36.93% <ø> (-28.83%) 0 <ø> (-18320)
Impacted Files Coverage Δ Complexity Δ
apps/files_external/lib/Command/Config.php 0% <0%> (-80%) 0% <0%> (ø)
lib/private/Files/Storage/DAV.php 59.45% <0%> (-21.64%) 0% <0%> (ø)
core/js/sharedialogexpirationview.js 72.05% <0%> (-4.51%) 0% <0%> (ø)
apps/files_external/lib/Command/ListCommand.php 67.72% <0%> (-3.79%) 0% <0%> (ø)
core/js/systemtags/systemtagsinputfield.js 83.23% <0%> (-0.68%) 0% <0%> (ø)
core/js/sharedialoglinkshareview.js 82.64% <0%> (-0.42%) 0% <0%> (ø)
core/js/js.js 54.89% <0%> (-0.16%) 0% <0%> (ø)
apps/files_external/lib/Lib/Storage/SFTP.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files_external/lib/Lib/Storage/Swift.php 66.16% <0%> (ø) 0% <0%> (ø) ⬇️
core/js/setupchecks.js 93.81% <0%> (ø) 0% <0%> (ø) ⬇️
... and 1091 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca5e5f9...71952f0. Read the comment docs.

1 similar comment
@codecov

This comment has been minimized.

Copy link

commented Dec 13, 2018

Codecov Report

Merging #32731 into master will decrease coverage by 16.52%.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #32731       +/-   ##
=============================================
- Coverage     64.42%   47.89%   -16.53%     
=============================================
  Files          1197      109     -1088     
  Lines         69264    10445    -58819     
  Branches       1276     1270        -6     
=============================================
- Hits          44625     5003    -39622     
+ Misses        24267     5072    -19195     
+ Partials        372      370        -2
Flag Coverage Δ Complexity Δ
#javascript 52.89% <ø> (-0.09%) 0 <ø> (ø)
#phpunit 36.93% <ø> (-28.83%) 0 <ø> (-18320)
Impacted Files Coverage Δ Complexity Δ
apps/files_external/lib/Command/Config.php 0% <0%> (-80%) 0% <0%> (ø)
lib/private/Files/Storage/DAV.php 59.45% <0%> (-21.64%) 0% <0%> (ø)
core/js/sharedialogexpirationview.js 72.05% <0%> (-4.51%) 0% <0%> (ø)
apps/files_external/lib/Command/ListCommand.php 67.72% <0%> (-3.79%) 0% <0%> (ø)
core/js/systemtags/systemtagsinputfield.js 83.23% <0%> (-0.68%) 0% <0%> (ø)
core/js/sharedialoglinkshareview.js 82.64% <0%> (-0.42%) 0% <0%> (ø)
core/js/js.js 54.89% <0%> (-0.16%) 0% <0%> (ø)
apps/files_external/lib/Lib/Storage/SFTP.php 0% <0%> (ø) 0% <0%> (ø) ⬇️
apps/files_external/lib/Lib/Storage/Swift.php 66.16% <0%> (ø) 0% <0%> (ø) ⬇️
core/js/setupchecks.js 93.81% <0%> (ø) 0% <0%> (ø) ⬇️
... and 1091 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca5e5f9...71952f0. Read the comment docs.

@ownclouders

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2018

Automated rebase with GitMate.io was successful! 🎉

@butonic

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

note that we just added the username as a preference for mounting wnd drives ... when in truth the login column in the account table is exactly that ...

@PVince81

This comment has been minimized.

Copy link
Member

commented Dec 13, 2018

@tomneedham #32731 (comment) I agree with adding a migration to cleanup.

Blocking editing in the UI I'd see this as out of scope for this PR. Let's do that separately, especially considering that it could be tricky. Currently the web UI stores its value into oc_accounts but this PR is about oc_preferences.

pending tasks left

@PVince81

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

also need to clarify as it seems some admins think this is a legitimate use case

@tomneedham tomneedham closed this Feb 12, 2019

@tomneedham

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

This is not the correct fix. The preferences quota is required for overwrites performed in the web ui.

The issue is that the web ui is not directly updating the accounts table not the perferences, which is not valid.

@tomneedham tomneedham deleted the quota-from-backend branch Feb 12, 2019

@phil-davis

This comment has been minimized.

Copy link
Contributor

commented Mar 9, 2019

Removing backport request because PR was closed.

@dpakach dpakach referenced this pull request May 7, 2019

Closed

Add quota related tests for LDAP users #368

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.