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

Changes from 4 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
18 changes: 16 additions & 2 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,24 @@ 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:
self.host = self.c.getProperty("omero.host")
Copy link
Contributor

Choose a reason for hiding this comment

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

I could imagine an scenario where a user tries to connect to a server with the wrong client object, which would then cause some ambiguity over exactly which server Blitz is connected to.

Could I suggest that we check if self.host is already set to something different here, and if there's a mismatch throw an error?

Copy link
Member Author

@jburel jburel Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We could add a check but i think people either specify the "client" or the host/user/password
Checking the host does not stop the connection to a wrong server. Using the "connect" specifying the sessionID could lead to that situation cf. one of the examples in the description and there is "false" value return

Copy link
Member Author

Choose a reason for hiding this comment

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

i think the gateway is too "flexible" leading to some very strange situations.

Copy link
Member Author

@jburel jburel Mar 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we also set the identity if a client with connection is specified?
the same will apply i.e. checking the value from client and via constructor.

Client with session specified, specify user name and password different from the client ones. Identity in gateway does not correspond to the identity in client

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that people should either specify client or host, but in the event that they don't I think it's safer to avoid silently overriding the user-supplied host parameter. Some of the autocompletion tools people now use do have a habit of trying to fill every argument. The user really should be reading the docs, but at the same time I don't think Gateway's Python docs are as helpful as they need to be.

You're probably right that Gateway's excessive flexibility here may be causing more problems than it solves. An alternative might be to validate the connection in the client object and outright reject calls with the other parameters specified - you either init with a connected client or supply credentials, no "mix and match".

Copy link
Member Author

@jburel jburel Mar 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There are other problems especially with the connect method since new clients get created (even if one was specified in the constructor).
So probably need to describe what we expect from each method moving forward

Copy link
Member Author

@jburel jburel Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The more I look into it the more I notice points that require clarification.
The clone method for example will not work when a client with session is specified.
To make it works one should do

BlitzGateway(client_obj=client)
conn.setIdentity(user, password)

We also lose info like useragent and userid in that case

self.port = port
if self.c is not None:
self.port = self.c.getProperty("omero.port")
self.secure = secure
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()
self._session = self.c.getSession()
except omero.ClientError: # no session available
pass
self._lastGroupId = None
self._anonymous = anonymous
self._defaultOmeroGroup = None
Expand Down Expand Up @@ -2082,8 +2092,12 @@ def _resetOmeroClient(self):
logger.debug(self.ice_config)

if self.c is not None:
self.c.__del__()
self.c = None
try:
if self.c.getSessionId() != self._sessionUuid:
jburel marked this conversation as resolved.
Show resolved Hide resolved
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
Loading