Warning: Can't synchronize with repository "(default)" (/home/git/ome.git does not appear to be a Git repository.). Look in the Trac log for more information.
Notice: In order to edit this ticket you need to be either: a Product Owner, The owner or the reporter of the ticket, or, in case of a Task not yet assigned, a team_member"

Task #8685 (closed)

Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

Making {omero.share:<sid>} enough to activate share context

Reported by: atarkowska Owned by: atarkowska
Priority: blocker Milestone: OMERO-4.4
Component: Web Version: n.a.
Keywords: n.a. Cc: web-team@…
Resources: n.a. Referenced By: n.a.
References: n.a. Remaining Time: 0.0d
Sprint: 2012-05-22 (15)

Description (last modified by atarkowska)

Now, in order to switch to share context, connection needs to call activate/deactivate on the security context. That will cause concurrency issues in multitudinous environment.

As discussed previously setting {"omero.share": sid} in query should be enough to get "shared" data by members of the share.

Action to be taken:

  1. utest adjustment (Ola)
  2. security context implementation (Josh)

See #8687 as well

Change History (14)

comment:1 Changed 12 years ago by atarkowska

  • Description modified (diff)

comment:2 Changed 12 years ago by atarkowska

It looks like SecurityViolation? is raised because omero.gateway.ImageWrapper?._prepareTB sets 'SERVICE_OPTS' based on the image details (group id of the image owner) rather then group id from the event context (group id of the member of the share accessing image. In that case security system does not know what to do. Either we define the hierarchy of parameters or we change the implementation. Please refer to the #8687 because more issues like that appeared in the log.

sopts = dict(self._conn.CONFIG['SERVICE_OPTS'] or {})

2012-05-04 12:43:03,646 WARNI [                           omero.gateway] (proc.09016) debug:3188 SecurityViolation on <class 'webclient.webclient_gateway.OmeroWebSafeCallWrapper'> to <a:80:38:b3:68b257ee:137119db057:-7d32omero.api.ThumbnailStore> setPixelsId((1707L, {'omero.share': '14945', 'omero.group': '4'}), {})
Traceback (most recent call last):
  File "/Users/ola/Dev/omero/dist/lib/python/omero/gateway/__init__.py", line 3206, in __call__
    return self.f(*args, **kwargs)
  File "/Users/ola/Dev/omero/dist/lib/python/omero_api_ThumbnailStore_ice.py", line 95, in setPixelsId
    return _M_omero.api.ThumbnailStore._op_setPixelsId.invoke(self, ((pixelsId, ), _ctx))
SecurityViolation: exception ::omero::SecurityViolation
{
    serverStackTrace = ome.conditions.SecurityViolation: User 52 is not a member of group 4 and cannot login

comment:3 Changed 12 years ago by atarkowska

  • Cc web-team@… added; cxallan wmoore jmoore removed
  • Owner changed from atarkowska to jmoore

How do you think Josh?

comment:5 Changed 12 years ago by jmoore

The intent is certainly not that "omero.group" and "omero.share" be set at the same time. It would probably be good to specify what that even means. What I was expecting clients to do was to set omero.share if the share is active, and otherwise use whatever omero.group the implementation needs (like that from the group, etc). In other words, for a share invocation, omero.share should suffice, and if you want to try to set omero.group too we can make that work but you should definitely not use just the group for an object in the share, since that doesn't tell the server to switch to the "share" implementation.

comment:6 Changed 12 years ago by jmoore

Ola, this test is passing for me on my chmod branch. Any thoughts on how we could go about testing?

comment:7 Changed 12 years ago by atarkowska

The test was written against 8610_mergin it means we have to merge both branches together then

comment:8 Changed 12 years ago by wmoore

These branches are already merged in my chmod_web_8434 branch.

comment:9 Changed 12 years ago by jburel

  • Sprint changed from 2012-05-08 (14) to 2012-05-22 (15)

Moved from sprint 2012-05-08 (14)

comment:10 Changed 12 years ago by Aleksandra Tarkowska <A.Tarkowska@…>

(In [2e08771d8967edc3f9758e39c828bda3ea052b8b/ome.git] on branch develop) adjusting integration test ishare.test8513, see #8685 and #8687 for comments

comment:11 Changed 12 years ago by jmoore

  • Owner changed from jmoore to atarkowska
  • Remaining Time changed from 0.5 to 0.1

Ola, I've committed what I needed to as part of #8608:

commit 9235f74342b150a13bcd93a920c62ab4306f055a
Author: jmoore <josh@glencoesoftware.com>
Date:   Thu May 10 11:55:07 2012 +0200

    Ignoring omero.group/user when omero.share set (Fix #8608)

diff --git a/components/server/src/ome/security/basic/BasicEventContext.java b/components/server/src/ome/security/basic/BasicEventContext.java
index 2b3009b..a17933e 100644
--- a/components/server/src/ome/security/basic/BasicEventContext.java
+++ b/components/server/src/ome/security/basic/BasicEventContext.java
@@ -96,7 +96,23 @@ class BasicEventContext extends SimpleEventContext {
 
         // Now re-apply values.
         final List<String> toPrint = new ArrayList<String>();
-        
+
+        final Long sid = parseId(callContext, "omero.share");
+        if (sid != null) {
+            if (!isAdmin) {
+                // If the user is not an admin then we need to verify that
+                // s/he is a valid member of the share.
+                ShareData data = store.getShareIfAccessible(sid, isAdmin, this.cuId);
+                if (data == null) {
+                    throw new SecurityViolation(String.format(
+                            "User %s cannot access share %s", this.cuId, sid));
+                }
+            }
+            setShareId(sid);
+            toPrint.add("share="+sid);
+            return; // IGNORE all other settings for share (#8608)
+        }

Passing off to you. Close if you think it's all done.

comment:12 Changed 12 years ago by atarkowska

  • Status changed from new to accepted

comment:13 Changed 12 years ago by atarkowska

  • Remaining Time changed from 0.1 to 0
  • Resolution set to fixed
  • Status changed from accepted to closed

Great works now.

comment:14 Changed 12 years ago by Aleksandra Tarkowska <A.Tarkowska@…>

(In [23de52ab460415ccaea71c5494a7135cbdbe8da9/ome.git] on branch develop) final clean up of sharing context and activation, see #8685

Note: See TracTickets for help on using tickets. You may also have a look at Agilo extensions to the ticket.

1.3.13-PRO © 2008-2011 Agilo Software all rights reserved (this page was served in: 0.82623 sec.)

We're Hiring!