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

Group-enable setting not preserved on app settings page #26638

Closed
mirekys opened this Issue Nov 15, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@mirekys
Contributor

mirekys commented Nov 15, 2016

Steps to reproduce

  1. Restrict some app (e.g Gallery) to be available only for certain groups (e.g. "gallery-users")
  2. Reload the App settings page (you can see app's allowed groups in group_select field)
  3. Reload the App settings page again (group restrictions are now gone/not visible)
  4. Check the Groups-enable checkbox (it will clear all previous group restrictions even from database)

Expected behaviour

App's group restriction visible on App Settings page should always
correspond to what is being saved in DB appconfig table.

Actual behaviour

App's group restriction disappeared from the App Settings page after the second reload.
However, it was still stored correctly in database:

owncloud=# SELECT * FROM oc_appconfig WHERE appid='gallery' AND configkey ='enabled';
  appid  |     configkey     |    configvalue    
---------+-------------------+-------------------
 gallery | enabled           | ["gallery-users"]
(3 rows)

I've found the problem is being caused by this code:
https://github.com/owncloud/core/blob/v9.0.6/settings/controller/appsettingscontroller.php#L282-L286

			// fix groups
			$groups = array();
			if (is_string($app['groups'])) {
				$groups = json_decode($app['groups']);
			}
			$app['groups'] = $groups;
  • On the first page reoad, the $app['groups'] is being decoded from string to Array, and then saved to $this->cache.

  • On the second page reload, the $app['groups'] is being pulled from cache as Array of groups. The code above then incorrectly assigns an empty array to $app['groups'] and caches it.

I would suggest a following fix:

			// fix groups
			$groups = array();
			if (is_string($app['groups'])) {
				$groups = json_decode($app['groups']);
+			} elseif (is_array($app['groups'])) {
+                              $groups = $app['groups'];
+                       }
			$app['groups'] = $groups;

Server configuration

Database:
PostgreSQL 9.2
PHP version:
5.6
ownCloud version: (see ownCloud admin page)
9.0.6
Updated from an older ownCloud or fresh install:
Updated from 8.2.9
Where did you install ownCloud from:
official tar package

@PVince81 PVince81 added this to the 9.0.7 milestone Nov 21, 2016

@PVince81 PVince81 self-assigned this Nov 21, 2016

@PVince81 PVince81 modified the milestones: 9.0.8, 9.0.7 Nov 30, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 30, 2016

Member

@mirekys mind sending your fix as a PR ? Thanks

Member

PVince81 commented Nov 30, 2016

@mirekys mind sending your fix as a PR ? Thanks

@mirekys mirekys referenced this issue Nov 30, 2016

Merged

Fix for app groups setting preservation #26748

3 of 9 tasks complete
@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2016

Member

I couldn't reproduce the issue but you mentioned a cache, so it's probably memcache ?
Trying that now

Member

PVince81 commented Dec 1, 2016

I couldn't reproduce the issue but you mentioned a cache, so it's probably memcache ?
Trying that now

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2016

Member

Indeed, reproduceable with redis as local memcache.

Member

PVince81 commented Dec 1, 2016

Indeed, reproduceable with redis as local memcache.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 1, 2016

Member

Fixed in #26748

Member

PVince81 commented Dec 1, 2016

Fixed in #26748

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment