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.

Sign up for GitHub

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

Jump to bottom

trac 13018: disabling inactive share access #4258

Merged
merged 8 commits into from Oct 20, 2015
Merged

trac 13018: disabling inactive share access #4258

merged 8 commits into from Oct 20, 2015

Conversation

joshmoore
Copy link
Member

When using joinSession on an inactive share, throw PermissionDenied:

        CannotCreateSessionException Raised if the session
E       PermissionDeniedException: exception ::Glacier2::PermissionDeniedException
E       {
E           reason = Share:1489 (497c947c-8e57-4e9a-9886-f3f87e6016a6) is inactive
E       }

See: https://trac.openmicroscopy.org/ome/ticket/13018#comment:4

@joshmoore
Copy link
Member Author

@will-moore / @aleksandra-tarkowska : I'd appreciate any additions to this test to reproduce http://trac.openmicroscopy.org/ome/ticket/13018 -- so far, I always get an exception when attempting to access such a share.

@will-moore
Copy link
Member

@aleksandra-tarkowska Do you want to have a look at this, or should I try to replicate what we were seeing in web?

@atarkowska
Copy link
Member

@will-moore I will write the full test, no worry

@atarkowska
Copy link
Member

@joshmoore done joshmoore#51

atarkowska and others added 3 commits October 14, 2015 09:19
@atarkowska @joshmoore
updating test for ticket 13018
36647dc
@joshmoore
Change to 'assert not'
8cbfb19
@joshmoore
SessionManagerImpl: add share checks to find
a80b486 
Before returning Share subclasses of Session from SessionCache,
check that they are active and non-expired. Calls to joinSession
now fail with PermissionDeniedException earlier.

NB: SessionCache might have been a better location for these
    checks but it does not have access to an Executor.
@sbesson
Copy link
Member

sbesson commented Oct 19, 2015

@joshmoore @aleksandra-tarkowska: is the test_ishare failure in https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-merge-integration-Python27/ and https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-merge-integration-python related to this PR?

@will-moore
Copy link
Member

The test tries to do self.new_client(session=s.uuid) a couple of times and both times this raises a PermissionDeniedException but it would be nice to know that this does actually work (before the share is disabled and a second time before the share is expired).

will-moore
will-moore reviewed Oct 19, 2015
View reviewed changes
components/tools/OmeroPy/test/integration/test_ishare.py
o_share.setActive(sid, True)

# test expired share, if member has no access to the image
expiration = long(time.time() * 1000) + 86400
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any relevance to the 86400? It looks like this is setting an expiration date in the future? But that wouldn't then be expired, so I'm confused.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expiration is when the share will expire rather than it has expired. Server-side this is converted into the appropriate setting for timeToLive.

@atarkowska
Copy link
Member

@will-moore there is a need to create new client object and join session, although looks like test was altered loosing its main logic, see joshmoore@36647dc#diff-cc72fc55d66d83492366c49e1420fff4R1011.

atarkowska
atarkowska reviewed Oct 19, 2015
View reviewed changes
components/server/src/ome/services/sessions/SessionManagerImpl.java

if (Boolean.FALSE.equals(active)) {
throw new SecurityViolation(prefix + " is inactive");
} else if (started.getTime() + timeToLive > System.currentTimeMillis()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Share may have no expiry date, see failing test_8118

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's never a null expiry but it is set to Long.MAX_VALUE which is causing an overflow. I'll push a fix for that in just a second.

@joshmoore
SessionManager: Handle timeToLive overflow
09864d6 
"Null" expiration values are translated to Long.MAX_VALUE.
The previous comparison overflowed to a negative number
causing login to fail.
@joshmoore
test_ishare: add positive test
e42a8ce
@joshmoore
Copy link
Member Author

The test tries to do self.new_client(session=s.uuid) a couple of times and both times this raises a PermissionDeniedException but it would be nice to know that this does actually work (before the share is disabled and a second time before the share is expired).

@will-moore, see e42a8ce

@joshmoore
Copy link
Member Author

Job is greeen: https://ci.openmicroscopy.org/view/Failing/job/OMERO-DEV-merge-integration-Python27/20/

@will-moore
Copy link
Member

OK - apart from minor potential for improving readability of the tests, this is good to merge.

@jburel
Copy link
Member

jburel commented Oct 20, 2015

Thanks all merging.

jburel added a commit that referenced this pull request Oct 20, 2015
@jburel
Merge pull request #4258 from joshmoore/trac13018-inactive-share
e3c4c6a 
trac 13018: disabling inactive share access
@jburel jburel merged commit e3c4c6a into ome:develop Oct 20, 2015
@sbesson sbesson added this to the 5.2.0 milestone Oct 28, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
develop
Projects
None yet
Milestone
5.2.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@joshmoore @will-moore @atarkowska @sbesson @jburel