Skip to content

Conversation

@VladIftime
Copy link
Collaborator

@VladIftime VladIftime commented Apr 15, 2025

This should represent an 'interface' for devs to use as a client for the WebSocket connection.

  • Add a test using an FRBC RM

…hat are expected to be overwritten

Signed-off-by: Vlad Iftime <[email protected]>
…hat are expected to be overwritten

Signed-off-by: Vlad Iftime <[email protected]>
Signed-off-by: Vlad Iftime <[email protected]>
Copy link
Contributor

@MauriceHendrix MauriceHendrix left a comment

Choose a reason for hiding this comment

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

Just a few comments to discuss, will look more closely later if I get some time


REQTEST_TIMEOUT = 10
PAIRING_TIMEOUT = datetime.timedelta(minutes=5)
KEY_ALGORITHM = "RSA-OAEP-256"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we need a discussion on what's the best default and how to communicate this


# pylint: disable=too-many-instance-attributes
# pylint: disable=too-many-arguments
def __init__(
Copy link
Contributor

Choose a reason for hiding this comment

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

hhm I wonder if we want to couple the JWK and encryption methods inside the abstract class or not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

from typing import TYPE_CHECKING
if TYPE_CHECKING:
    from jwskate import Jwk

@MauriceHendrix
Copy link
Contributor

MauriceHendrix commented Apr 23, 2025

A few general comments:

  • your ci fails because it doesn't seem to install websockets, correctly, I think you need to add it to dev-requirements.txt

  • can we use the logging module instead of the many print statements?

  • I'm a bit concerned about backwards compatibility, for example with moving s2_connection terms of moving I think it should be possible with some trickery with various imports in init.py files to make it importable from it's original location, as long as the old signatures will work.

  • the abstract client seems to contain a few specifics around jwk's and tokens, that we may prefer to leave up to the implementation / default client?

@MauriceHendrix
Copy link
Contributor

I had a quick look.
In terms of moving the imports you could probably solve it by adding something like the following to __init__.py (in the `src folder):

import sys
from s2python.communication.s2_connection import S2Connection
sys.modules['s2python.s2_connection'] = sys.modules.get('s2python.communication.s2_connection', None)

VladIftime and others added 4 commits May 6, 2025 15:32
@MauriceHendrix
Copy link
Contributor

I had a quick look. In terms of moving the imports you could probably solve it by adding something like the following to __init__.py (in the `src folder):

import sys
from s2python.communication.s2_connection import S2Connection
sys.modules['s2python.s2_connection'] = sys.modules.get('s2python.communication.s2_connection', None)

actually sorry @VladIftime looks like it needs to be one level up, I changed that myself

@MauriceHendrix
Copy link
Contributor

I moved the backward compatibility fix as I suggested the wrong place, sorry. I Think you don't need the pass statements, but do need to add jwskate to dev-requirements.txt @VladIftime

@MauriceHendrix
Copy link
Contributor

@VladIftime see #119 for fixes to tests and linting errors

* fixed linting errors

* typing error in backwards compatibility fix

* fixed missing requests typing

* fixed missing import
using the connection details and solved challenge.
Note: This is a placeholder implementation. In a real implementation,
this would use a WebSocket library like websocket-client or websockets.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this would use a WebSocket library like websocket-client or websockets.
this uses a WebSocket library like websocket-client or websockets.

"""
if self._ws_connection:

logger.info("Would close WebSocket connection")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
logger.info("Would close WebSocket connection")
logger.info("WebSocket connection closed")

self.send_header("Content-Type", "application/json")
self.end_headers()

# Create challenge (normally would be a JWE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Create challenge (normally would be a JWE)
# Create challenge (normally a JWE)

Copy link
Collaborator Author

@VladIftime VladIftime left a comment

Choose a reason for hiding this comment

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

move the examples into the source.

@jorritn
Copy link
Contributor

jorritn commented May 20, 2025

The example would also serve to run an integration test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants