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.

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

KeycloakOpenIDConnection created successfully even if the credentials are wrong #595

Open
alexrohozneanu opened this issue Sep 25, 2024 · 3 comments

Comments

@alexrohozneanu
Copy link
Contributor

alexrohozneanu commented Sep 25, 2024

I have the following code

I am using python-keycloak=4.4.0

keycloak_connection = KeycloakOpenIDConnection(
            server_url="https://localhost:8443",
            username="admin",
            password="admin",
            verify=False,
        )
admin = LibKeycloakAdmin(connection=keycloak_connection)

print(admin.get_server_info())

This works perfectly and outputs the desired result

However, if I put in some wrong credentials, the keycloak_connection object is still created, and the keycloak.exceptions.KeycloakAuthenticationError: 401: b'{"error":"invalid_grant","error_description":"Invalid user credentials"}' error is raise only when I call a method from the admin object like admin.get_server_info(). Example bellow:

keycloak_connection = KeycloakOpenIDConnection(
            server_url="https://localhost:8443",
            username="dummy",
            password="dummy",
            verify=False,
        )
admin = LibKeycloakAdmin(connection=keycloak_connection)

print(admin.get_server_info())

This is the output:

/Users/alexr/PycharmProjects/AndreeaRulesPython/.venv/lib/python3.8/site-packages/urllib3/connectionpool.py:1099: InsecureRequestWarning: Unverified HTTPS request is being made to host 'localhost'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#tls-warnings
  warnings.warn(
Traceback (most recent call last):
  File "/Users/alexr/PycharmProjects/AndreeaRulesPython/python-keycloak.py", line 16, in <module>
    print(admin.get_server_info())
  File "/Users/alexr/PycharmProjects/AndreeaRulesPython/.venv/lib/python3.8/site-packages/keycloak/keycloak_admin.py", line 963, in get_server_info
    data_raw = self.connection.raw_get(urls_patterns.URL_ADMIN_SERVER_INFO)
  File "/Users/alexr/PycharmProjects/AndreeaRulesPython/.venv/lib/python3.8/site-packages/keycloak/openid_connection.py", line 369, in raw_get
    self._refresh_if_required()
  File "/Users/alexr/PycharmProjects/AndreeaRulesPython/.venv/lib/python3.8/site-packages/keycloak/openid_connection.py", line 354, in _refresh_if_required
    self.refresh_token()
  File "/Users/alexr/PycharmProjects/AndreeaRulesPython/.venv/lib/python3.8/site-packages/keycloak/openid_connection.py", line 337, in refresh_token
    self.get_token()
  File "/Users/alexr/PycharmProjects/AndreeaRulesPython/.venv/lib/python3.8/site-packages/keycloak/openid_connection.py", line 324, in get_token
    self.token = self.keycloak_openid.token(
  File "/Users/alexr/PycharmProjects/AndreeaRulesPython/.venv/lib/python3.8/site-packages/keycloak/keycloak_openid.py", line 339, in token
    return raise_error_from_response(data_raw, KeycloakPostError)
  File "/Users/alexr/PycharmProjects/AndreeaRulesPython/.venv/lib/python3.8/site-packages/keycloak/exceptions.py", line 192, in raise_error_from_response
    raise error(
keycloak.exceptions.KeycloakAuthenticationError: 401: b'{"error":"invalid_grant","error_description":"Invalid user credentials"}'

I would expect that the error should be raised during the creation of the connection and not afterwards.
Same thing for client_id and client_secret_key

@ryshoooo
Copy link
Collaborator

Hi @alexrohozneanu

This is a deliberate design of the library, network calls are only made upon executed actions, not during instantiation.

@alexrohozneanu
Copy link
Contributor Author

Hi @ryshoooo,

Thanks for your comment!

I would expect that, in case there is no network connection to the server, the code would raise the error as soon as possible, and that would be during the instantiation of the class.

Also the name of the class is suffixed by "Connection" which is making me believe the connection should already be established during instantiation.

Are there any specific reasons on why the library is designed like that?

@ryshoooo
Copy link
Collaborator

Hi @alexrohozneanu

Thanks for the feedback :)

1 big reason why is testing, the moment you have network calls being made upon instantiation, the mocking becomes quite painful and on a much higher level than needed in user applications testing. This is particularly true in case the user applications have global instances of Admin/OpenID clients in their applications, i.e. initialized upon import, which makes test structure and mocking quite a bit more painful. Postponing network calls until actual actions are happening makes testing significantly easier.

Second reason is a bit more personal I suppose, but I like to have control of when network calls are made, and I do not wish to make them unless absolutely necessary. Instantiation of an object, (again this is my world view, so feel free to disagree) should never make a network call, it should just create an instance of the class ready to be used. If some attributes of the class require network calls to be made, make the attributes lazy and only make the network call when the attribute is retrieved in the user code.

I can understand the confusion that a Connection object does not make a connection upon instantiation, but many Connection classes in many popular libraries do not make a network call upon instantiation, only when .connect() or similar method is called. I suppose it's a matter of perspective on what the class is supposed to do, and the usual way I think about the Connection classes is that they just contain all the necessary data and configuration on how to make a TCP/UDP connection, and obv some useful methods to dial and close a connection. But that only happens when the user decides to actually dial an IP+port, and that's a principle I'd like to preserve also here.

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

No branches or pull requests

2 participants