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

Session ID and constructor #400

Merged
merged 18 commits into from
Apr 24, 2024
Merged

Session ID and constructor #400

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
9ae6018
set host and port from client if available
jburel Mar 1, 2024
dbf00ab
set session from client
jburel Mar 1, 2024
72d735c
do not close session associated to client
jburel Mar 1, 2024
ee24282
remove uncessary check
jburel Mar 1, 2024
f30d62a
raise exception if hosts and ports do not match
jburel Mar 4, 2024
65f9d94
fix syntax
jburel Mar 4, 2024
150803b
set the parameters explicitly
jburel Mar 4, 2024
489c78b
set secure flag from client
jburel Mar 6, 2024
dd97de9
return correct flag using isSecure
jburel Mar 6, 2024
704e50f
set session ID only
jburel Mar 25, 2024
e2025ed
reset sessionUuid
jburel Mar 27, 2024
88d2453
return secure flag
jburel Apr 2, 2024
a91f391
check if it is the same session
jburel Apr 2, 2024
4dc40d6
remove comment
jburel Apr 8, 2024
e989c33
remove conditional
jburel Apr 11, 2024
2474872
review how secure flag is set
jburel Apr 11, 2024
01fc825
revert to original implementation
jburel Apr 11, 2024
64bf1f9
check compatibility of secure flag
jburel Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions src/omero/gateway/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1525,14 +1525,33 @@ def __init__(self, username=None, passwd=None, client_obj=None, group=None,
self.ice_config = [os.path.abspath(str(x)) for x in [_f for _f in self.ice_config if _f]]

self.host = host
if self.c is not None:
hc = self.c.getProperty("omero.host")
if self.host is None:
self.host = hc
elif hc != self.host:
raise Exception("hosts %s and %s do not match" % (hc, self.host))
self.port = port
if self.c is not None:
pc = self.c.getProperty("omero.port")
if self.port is None:
self.port = pc
elif pc != self.port:
raise Exception("ports %s and %s do not match" % (pc, self.port))
self.secure = secure
if self.c is not None:
self.secure = self.c.isSecure()
Copy link
Member

Choose a reason for hiding this comment

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

Not overly important, but should there be a mismatch test here?

Copy link
Member

Choose a reason for hiding this comment

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

I had a similar question while performing code review but this was marked as resolved. Was this addressed somewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

tested added there https://github.com/ome/openmicroscopy/pull/6381/files

Copy link
Member

Choose a reason for hiding this comment

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

My interpretation of @joshmoore's question was: what should happen if a gateway object is constructed with an initialized client object and secure={True,False} and the encryption state of the client as returned by self.c.isSecure() does not match the secure keyword?

self.useragent = useragent
self.userip = userip

self._sessionUuid = None
self._session_cb = None
self._session = None
if self.c is not None:
try:
self._sessionUuid = self.c.getSessionId()
except omero.ClientError: # no session available
pass
self._lastGroupId = None
self._anonymous = anonymous
self._defaultOmeroGroup = None
Expand Down Expand Up @@ -1936,6 +1955,7 @@ def close(self, hard=True): # pragma: no cover
oldC = None
self.c = None
self._session = None
#self._sessionUuid = None
sbesson marked this conversation as resolved.
Show resolved Hide resolved

self._proxies = NoProxies()
logger.info("closed connection (uuid=%s)" % str(self._sessionUuid))
Expand Down Expand Up @@ -2029,7 +2049,7 @@ def isSecure(self):
Returns 'True' if the underlying omero.clients.BaseClient is connected
using SSL
"""
return hasattr(self.c, 'isSecure') and self.c.isSecure() or False
Copy link
Member

@sbesson sbesson Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't that modifying the contract of the API before a connection happens e.g. in a scenario like the following

>>> conn=BlitzGateway(username="test",passwd="test",host="localhost",secure=False)
>>> conn.isSecure()
True
>>> conn.secure
False

Copy link
Member Author

Choose a reason for hiding this comment

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

This raises another question in that case
When a client is not specified, a secure client is created regardless of the secure parameter specified. But when a client is specified, it can be secured or unsecured.

Copy link
Member

@sbesson sbesson Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The initial connection will (and should) always use an encrypted client. But

omero-py/src/omero/gateway/__init__.py

Line 2074 in 4dc40d6

self.setSecure(self.secure)
in _createSession might create an unencrypted client depending on the state of secure:

>>> conn=BlitzGateway(username=user,passwd=password,host="localhost")
>>> conn.connect()
True
>>> conn.isSecure()
False
>>> conn.close()
>>> conn=BlitzGateway(username=user,passwd=password,host="localhost",secure=True)
>>> conn.connect()
True
>>> conn.isSecure()
True

Copy link
Member Author

@jburel jburel Apr 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i get that
but as I said the client specified could be insecured and in that case even if secure=True is passed
conn.isSecure() will be false.
Do we want in that case to enforce a "secure" client specified during init

Copy link
Member

Choose a reason for hiding this comment

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

So the scenario we are discussing is

conn=BlitzGateway(client_obj=insecure_client, secure=True)
conn=BlitzGateway(client_obj=secure_client, secure=False)

Interestingly, part of the changes proposed here are to throw an exception on the following use cases

conn=BlitzGateway(client_obj=client, host=different_host)
conn=BlitzGateway(client_obj=client, port=different_port)

My feeling would be to say secure should behave in the same way i.e. if the constructor receives conflicting information from its inputs, an exception should be sent back to the caller. Unlike host and port the default of secure is False. A possible (untested) solution would be to change the default to None to cater for this scenario (and internally default to False if no client object is passed).

return self.secure

def _getSessionId(self):
return self.c.getSessionId()
Expand Down Expand Up @@ -2082,8 +2102,11 @@ def _resetOmeroClient(self):
logger.debug(self.ice_config)

if self.c is not None:
self.c.__del__()
self.c = None
try:
self.c.__del__()
self.c = None
except omero.ClientError: # no session available
pass

if self.host is not None:
if self.port is not None:
Expand Down Expand Up @@ -2125,6 +2148,16 @@ def connect(self, sUuid=None):
logger.debug("Ooops. no self._c")
return False
try:
if self.c is not None:
jburel marked this conversation as resolved.
Show resolved Hide resolved
try:
sid = self.c.getSessionId()
# we have a session already from the client
if sUuid is None or sid == sUuid:
logger.debug('connected via client')
return True
except omero.ClientError: # no session available
pass

if self._sessionUuid is None and sUuid:
self._sessionUuid = sUuid
if self._sessionUuid is not None:
Expand Down
3 changes: 2 additions & 1 deletion src/omero/gateway/scripts/dbhelpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ def login(self, groupname=None):
if groupname is None:
groupname = self.groupname
client = omero.gateway.BlitzGateway(
self.name, self.passwd, group=groupname, try_super=self.admin)
username=self.name, passwd=self.passwd,
group=groupname, try_super=self.admin)
if not client.connect():
print("Can not connect")
return None
Expand Down