Preventing system users being removed from system groups #1733

atarkowska · 2013-11-07T10:30:20Z

Issue spotted in #1723. Requires additional changes to PR #565

To test it check if you can't remove root or guest from system groups in Webadmin.

manics · 2013-11-08T10:21:36Z

Works as expected. However I've come across a related issue: Attempting to rename user root (as any admin user) or group system causes an exception. Do you want to fix this here or leave it?

atarkowska · 2013-11-12T11:44:17Z

@manics try now, you should't be able to edit name

manics · 2013-11-13T16:13:06Z

One last thing (sorry). In the group admin page there's nothing to stop an admin user removing themself from the system group, though it's prohibited and it seems to cause an Internal Server Error.

joshmoore · 2013-11-14T07:47:46Z

/cc @mtbc for comment re: the Internal Server Error since an ApiUsage or Validation exception would be better.

mtbc · 2013-11-14T08:28:24Z

The IAdmin stuff throws ValidationException and the integration tests catch them. It is possible that @manics is accessing them through code that catches that and throws a different exception? (Maybe something in OMEROGateway?)

See https://www.openmicroscopy.org/qa2/qa/feedback/7701/

manics · 2013-11-14T09:53:21Z

re: the Internal Server Error since an ApiUsage or Validation exception would be better.

I think yesterday's error was an Internal Server Error from apache (Plain white page rather than the django error page/dialog). I've tried it again today and it throws a Server Error. (500) ValidationException: exception ::omero::ValidationException, so maybe it was a weird temporary apache thing.

joshmoore · 2013-11-14T11:38:56Z

Hmmm..... a 500 seems odd either way. But if it wasn't an Internal from the server (i.e. blitz) then perhaps we can un-invite @mtbc from the discussion.

atarkowska · 2013-11-14T11:52:45Z

Those should prevent 'root' and current user from removing from 'system' either via experimenter or group edit forms

atarkowska · 2013-11-14T11:58:33Z

Testing root:

log in as 'root'
edit 'root' and check if check-boxes are greyed out and 'system' has no X
go to Groups
edit 'system' and check if in members 'root' has not X

Testing as admin_user:

log in and admin_user
edit admin_user and check if check-boxes are greyed out and 'system' has no X
go to Groups
edit 'system' and check if in members 'root' and admin_user has not X

bpindelski · 2013-11-15T10:48:50Z

Tested both scenarios - all works as expected. Looks OK to merge.

joshmoore · 2013-11-15T15:01:37Z

any comments on the new method? @knabar @chris-allan @will-moore @cneves

knabar · 2013-11-15T15:16:44Z

components/tools/OmeroWeb/omeroweb/webadmin/templates/webadmin/experimenter_form.html

@@ -74,10 +74,13 @@
        };

        // Since we want to disable removal of 'system' group (id=0) from chosen, this hides the 'X'
+        var admin_groups = [{% for x in admin_groups %}'{{ x|escapejs }}',{% endfor %}]


Having a trailing comma may blow up in IE before 9 - don't have a browser installed to test it right now though

http://stackoverflow.com/questions/7246618/trailing-commas-in-javascript

How about

from django.utils import simplejson list = simplejson.dumps(LIST) var JS_LIST = {{ list|safe }};

jburel · 2013-11-15T15:21:06Z

General question: do we want other admin to modify the password of root?
I have turned that off in gh-1753, no crash, Just a point to consider

will-moore · 2013-11-15T15:31:27Z

components/tools/OmeroPy/src/omero/gateway/__init__.py

+        @return:    Current Experimenter
+        @return:     Generator of L{BlitzObjectWrapper} subclasses
+        """
+        return self.getObject("ExperimenterGroup", 0).getMembers()


Instead of hard-coding 0, you could use conn.getAdminService().getSecurityRoles().systemGroupId

will-moore · 2013-11-19T13:37:17Z

The behaviour is fine - can't remove myself from system or user groups. Still a bit confusing to newbies to see "system" as a group that is added / removed according to the Administrator checkbox - but I guess we can't hide it from the list of groups, since some users are only in "system" and their data is in system group too.
Slightly strange that I can add another user to "system" group by typing this name in the groups chooser, adding it to the list, but then I can't click the X to remove it from the list (have to go to Administrator checkbox). Maybe if we're going to show the X, then it should be clickable and active.

atarkowska · 2013-11-20T11:03:20Z

that should be final commit

atarkowska · 2013-11-20T11:25:31Z

@dpwrussell: it should fix ticket http://trac.openmicroscopy.org.uk/ome/ticket/11693

snoopycrimecop · 2013-11-25T05:04:03Z

Conflicting PR.Removed from build OMERO-merge-develop#497. See the console output for more details.

joshmoore · 2013-11-25T08:12:26Z

This is conflicting with the unit support PR.

pwalczysko · 2013-11-25T14:49:10Z

Bug:

Login as user-6 (=admin) on Gretzky.
Go to Admin tab
Go to Group tab
Edit System group
in the Add/Remove users box, all the admins are with clickable crosses (see screenshot)
click on the root (to remove him from System group)
the action succeeds
click Save
Error appears (see below)

Traceback (most recent call last):

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/django/core/handlers/base.py", line 114, in get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/omeroweb/decorators.py", line 386, in wrapped
    retval = f(request, *args, **kwargs)

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/omeroweb/decorators.py", line 432, in wrapper
    context = f(request, *args, **kwargs)

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/omeroweb/webadmin/views.py", line 613, in manage_group
    conn.setMembersOfGroup(group, new_members)

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/omeroweb/webclient/webclient_gateway.py", line 982, in setMembersOfGroup
    admin_serv.removeGroups(e, [group._obj])

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/omero/gateway/__init__.py", line 3526, in __call__
    return self.handle_exception(e, *args, **kwargs)

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/omeroweb/webclient/webclient_gateway.py", line 1874, in handle_exception
    e, *args, **kwargs)

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/omero/gateway/__init__.py", line 3523, in __call__
    return self.f(*args, **kwargs)

  File "/home/omero/slave/workspace/OMERO-merge-develop/src/dist/lib/python/omero_api_IAdmin_ice.py", line 280, in removeGroups
    return _M_omero.api.IAdmin._op_removeGroups.invoke(self, ((user, groups), _ctx))

ValidationException: exception ::omero::ValidationException
{
    serverStackTrace = ome.conditions.ValidationException: experimenter 'root' may not be removed from the 'system' or 'user' group
    at ome.logic.AdminImpl.removeGroups(AdminImpl.java:710)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:307)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:183)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150)
    at ome.security.basic.EventHandler.invoke(EventHandler.java:154)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at org.springframework.orm.hibernate3.HibernateInterceptor.invoke(HibernateInterceptor.java:111)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:108)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at ome.tools.hibernate.ProxyCleanupFilter$Interceptor.invoke(ProxyCleanupFilter.java:241)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at ome.services.util.ServiceHandler.invoke(ServiceHandler.java:116)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:202)
    at $Proxy78.removeGroups(Unknown Source)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:307)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:183)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150)
    at ome.security.basic.BasicSecurityWiring.invoke(BasicSecurityWiring.java:98)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at ome.services.blitz.fire.AopContextInitializer.invoke(AopContextInitializer.java:43)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:202)
    at $Proxy78.removeGroups(Unknown Source)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at ome.services.blitz.util.IceMethodInvoker.invoke(IceMethodInvoker.java:179)
    at ome.services.throttling.Callback.run(Callback.java:56)
    at ome.services.throttling.InThreadThrottlingStrategy.callInvokerOnRawArgs(InThreadThrottlingStrategy.java:56)
    at ome.services.blitz.impl.AbstractAmdServant.callInvokerOnRawArgs(AbstractAmdServant.java:149)
    at ome.services.blitz.impl.AdminI.removeGroups_async(AdminI.java:299)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:307)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:183)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150)
    at omero.cmd.CallContext.invoke(CallContext.java:65)
    at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172)
    at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:202)
    at $Proxy79.removeGroups_async(Unknown Source)
    at omero.api._IAdminTie.removeGroups_async(_IAdminTie.java:309)
    at omero.api._IAdminDisp.___removeGroups(_IAdminDisp.java:975)
    at omero.api._IAdminDisp.__dispatch(_IAdminDisp.java:1637)
    at IceInternal.Incoming.invoke(Incoming.java:159)
    at Ice.ConnectionI.invokeAll(ConnectionI.java:2037)
    at Ice.ConnectionI.message(ConnectionI.java:972)
    at IceInternal.ThreadPool.run(ThreadPool.java:577)
    at IceInternal.ThreadPool.access$100(ThreadPool.java:12)
    at IceInternal.ThreadPool$EventHandlerThread.run(ThreadPool.java:971)

    serverExceptionClass = ome.conditions.ValidationException
    message = experimenter 'root' may not be removed from the 'system' or 'user' group
}


<WSGIRequest
path:/omero/webadmin/group/save/0/,
GET:<QueryDict: {}>,
POST:<QueryDict: {u'permissions': [u'0'], u'name': [u'system'], u'members': [u'15', u'202', u'54', u'7', u'18', u'4', u'552'], u'description': [u'']}>,
COOKIES:{'sessionid': 'z2qfnnet2nm7mu192veoxdow3gn2b33n'},
META:{'CONTENT_LENGTH': '115',
 'CONTENT_TYPE': 'application/x-www-form-urlencoded',
 'DOCUMENT_ROOT': '/htdocs',
 'GATEWAY_INTERFACE': 'CGI/1.1',
 'HTTPS': 'on',
 'HTTP_ACCEPT': 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
 'HTTP_ACCEPT_ENCODING': 'gzip, deflate',
 'HTTP_ACCEPT_LANGUAGE': 'en-us',
 'HTTP_CONNECTION': 'keep-alive',
 'HTTP_COOKIE': 'sessionid=z2qfnnet2nm7mu192veoxdow3gn2b33n',
 'HTTP_HOST': 'gretzky.openmicroscopy.org.uk',
 'HTTP_ORIGIN': 'https://gretzky.openmicroscopy.org.uk',
 'HTTP_REFERER': 'https://gretzky.openmicroscopy.org.uk/omero/webadmin/group/edit/0/',
 'HTTP_USER_AGENT': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/536.30.1 (KHTML, like Gecko) Version/6.0.5 Safari/536.30.1',
 'PATH': '/usr/local/bin:/usr/bin:/bin',
 'PATH_INFO': u'/webadmin/group/save/0/',
 'PATH_TRANSLATED': '/htdocs/webadmin/group/save/0/',
 'QUERY_STRING': '',
 'REMOTE_ADDR': '10.12.1.115',
 'REMOTE_PORT': '52768',
 'REQUEST_METHOD': 'POST',
 'REQUEST_URI': '/omero/webadmin/group/save/0/',
 'SCRIPT_FILENAME': '/home/omero/OMERO-CURRENT/var/omero.fcgi',
 'SCRIPT_NAME': u'/omero',
 'SCRIPT_URI': 'https://gretzky.openmicroscopy.org.uk/omero/webadmin/group/save/0/',
 'SCRIPT_URL': '/omero/webadmin/group/save/0/',
 'SERVER_ADDR': '134.36.65.227',
 'SERVER_ADMIN': '[no address given]',
 'SERVER_NAME': 'gretzky.openmicroscopy.org.uk',
 'SERVER_PORT': '443',
 'SERVER_PROTOCOL': 'HTTP/1.1',
 'SERVER_SIGNATURE': '<address>Apache/2.2.14 (Ubuntu) Server at gretzky.openmicroscopy.org.uk Port 443</address>\n',
 'SERVER_SOFTWARE': 'Apache/2.2.14 (Ubuntu)',
 'SSL_TLS_SNI': 'gretzky.openmicroscopy.org.uk',
 'wsgi.errors': <flup.server.fcgi_base.TeeOutputStream object at 0x3910b50>,
 'wsgi.input': <flup.server.fcgi_base.InputStream object at 0x3910990>,
 'wsgi.multiprocess': True,
 'wsgi.multithread': False,
 'wsgi.run_once': False,
 'wsgi.url_scheme': 'https',
 'wsgi.version': (1, 0)}>

atarkowska · 2013-11-25T15:02:31Z

That is not related as the PR hasn't been merged because of the conflict

snoopycrimecop · 2013-11-26T05:05:25Z

Conflicting PR.Removed from build OMERO-merge-develop#498. See the console output for more details.

snoopycrimecop · 2013-11-27T05:06:58Z

Conflicting PR.Removed from build OMERO-merge-develop#499. See the console output for more details.

additional changes to PR #565

….systemGroupId

…dmin::getSecurityRoles

…t, close #11693

manics · 2013-11-28T15:27:59Z

Finally, good to merge :)

joshmoore · 2013-12-02T09:23:42Z

Re-running travis job.

knabar · 2013-12-03T09:13:39Z

Looks good to merge

…oup-change-restrictions Preventing system users being removed from system groups

atarkowska · 2014-01-05T23:03:27Z

--no-rebase Following #1723

will-moore · 2014-01-15T12:05:25Z

components/tools/OmeroWeb/omeroweb/webadmin/views.py

+                                     'owners': ownerIds, 'members':memberIds, 'experimenters':experimenters},
+                                     group_is_current_or_system=group_is_current_or_system)
+        admins = [conn.getAdminService().getSecurityRoles().systemGroupId]
+        if long(gid) in system_groups and conn.isAdmin:


Was just reviewing this code as I had to fix a merge conflict...
I think you should use rootId instead of systemGroupId here, since you want the ID of the root user
admins = [conn.getAdminService().getSecurityRoles().rootId]

Also, it looks like you didn't call the conn.isAdmin: method (missing brackets) but this shouldn't be needed anyway as the login_required decorator already checks that only Admins can view this page.

I'll make these changes in my conflict resolution, unless I'm missing something?

fixed in #1983

atarkowska mentioned this pull request Nov 7, 2013

fix #10209: restrict one's changes to user, group #1723

Merged

knabar reviewed Nov 15, 2013
View reviewed changes

will-moore reviewed Nov 15, 2013
View reviewed changes

atarkowska added 5 commits November 27, 2013 12:17

Preventing system users being removed from system groups

54b49b8

additional changes to PR #565

preventing from changing user and group name of system entities

5056d09

adding getAdministrators helper to the gateway

48160f5

fixing prohibited action of user/group removal

e5cf050

replacing hardcoded value '0' by getAdminService().getSecurityRoles()…

bc6453c

….systemGroupId

atarkowska added 5 commits November 27, 2013 12:17

replacing hardcoded group and user ids by values provided via api::IA…

2da9a7f

…dmin::getSecurityRoles

removing trailing comma

8297de2

uncheking admin checkbox when removed from system group

4694ca3

preventing from changing the name of current group in the EventContex…

da349f5

…t, close #11693

fixing issue with passing inexistent object to the template

4bd0c0b

joshmoore added a commit that referenced this pull request Dec 3, 2013

Merge pull request #1733 from aleksandra-tarkowska/trac-11465-user-gr…

ef0a902

…oup-change-restrictions Preventing system users being removed from system groups

joshmoore merged commit ef0a902 into ome:develop Dec 3, 2013

atarkowska deleted the trac-11465-user-group-change-restrictions branch December 5, 2013 19:50

will-moore reviewed Jan 15, 2014
View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preventing system users being removed from system groups #1733

Preventing system users being removed from system groups #1733

atarkowska commented Nov 7, 2013

manics commented Nov 8, 2013

atarkowska commented Nov 12, 2013

manics commented Nov 13, 2013

joshmoore commented Nov 14, 2013

mtbc commented Nov 14, 2013

manics commented Nov 14, 2013

joshmoore commented Nov 14, 2013

atarkowska commented Nov 14, 2013

atarkowska commented Nov 14, 2013

bpindelski commented Nov 15, 2013

joshmoore commented Nov 15, 2013

knabar Nov 15, 2013

atarkowska Nov 19, 2013

jburel commented Nov 15, 2013

will-moore Nov 15, 2013

will-moore commented Nov 19, 2013

atarkowska commented Nov 20, 2013

atarkowska commented Nov 20, 2013

snoopycrimecop commented Nov 25, 2013

joshmoore commented Nov 25, 2013

pwalczysko commented Nov 25, 2013

atarkowska commented Nov 25, 2013

snoopycrimecop commented Nov 26, 2013

snoopycrimecop commented Nov 27, 2013

manics commented Nov 28, 2013

joshmoore commented Dec 2, 2013

knabar commented Dec 3, 2013

atarkowska commented Jan 5, 2014

will-moore Jan 15, 2014

atarkowska Jan 15, 2014

Preventing system users being removed from system groups #1733

Preventing system users being removed from system groups #1733

Conversation

atarkowska commented Nov 7, 2013

manics commented Nov 8, 2013

atarkowska commented Nov 12, 2013

manics commented Nov 13, 2013

joshmoore commented Nov 14, 2013

mtbc commented Nov 14, 2013

manics commented Nov 14, 2013

joshmoore commented Nov 14, 2013

atarkowska commented Nov 14, 2013

atarkowska commented Nov 14, 2013

bpindelski commented Nov 15, 2013

joshmoore commented Nov 15, 2013

knabar Nov 15, 2013

Choose a reason for hiding this comment

atarkowska Nov 19, 2013

Choose a reason for hiding this comment

jburel commented Nov 15, 2013

will-moore Nov 15, 2013

Choose a reason for hiding this comment

will-moore commented Nov 19, 2013

atarkowska commented Nov 20, 2013

atarkowska commented Nov 20, 2013

snoopycrimecop commented Nov 25, 2013

joshmoore commented Nov 25, 2013

pwalczysko commented Nov 25, 2013

atarkowska commented Nov 25, 2013

snoopycrimecop commented Nov 26, 2013

snoopycrimecop commented Nov 27, 2013

manics commented Nov 28, 2013

joshmoore commented Dec 2, 2013

knabar commented Dec 3, 2013

atarkowska commented Jan 5, 2014

will-moore Jan 15, 2014

Choose a reason for hiding this comment

atarkowska Jan 15, 2014

Choose a reason for hiding this comment