Add typing and async support for SSO #291

mattgd · 2024-07-25T16:23:50Z

Description

Add typing and async support for SSO.

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

linear · 2024-07-25T16:23:53Z

DSYNC-1735 Add types to Connection API methods

tribble

See comments inline. Creating an HTTP client to generate a URL seems like such a smell. Is there a better way?

tribble · 2024-07-25T17:54:30Z

tests/test_sso.py

    @pytest.fixture
    def mock_profile(self):
        return {
+            "object": "profile",


Kind of amazing how invalid our test fixtures were.

tribble · 2024-07-25T17:55:34Z

tests/test_sso.py

        connection_list = [MockConnection(id=str(i)).to_dict() for i in range(10)]

        return {
+            "object": "list",


I did create a little list_response_of helper to help with this repetition

tribble · 2024-07-25T17:57:24Z

tests/test_sso.py

+        self.customer_domain = "workos.com"
+        self.login_hint = "[email protected]"
+        self.redirect_uri = "https://localhost/auth/callback"
+        self.state = json.dumps({"things": "with_stuff"})


Should this correspond to a Connection state?

Given tests are passing, it makes me wonder how (or if) this is used.

This is actually the authorization state (values a dev would stuff into a authorization URL. I'll rename for clarity.

Ah. Damn you state and how general you are.

tribble · 2024-07-25T18:01:11Z

tests/test_sso.py

+        connections = self.sso.list_connections()
+        all_connections = []

-    def test_delete_connection(self, setup_with_client_id, mock_raw_request_method):
-        mock_raw_request_method(
-            "delete",
-            "No Content",
-            204,
-            headers={"content-type": "text/plain; charset=utf-8"},
+        for connection in connections.auto_paging_iter():
+            all_connections.append(connection)


We repeat something like this for every auto paging test. I wonder if it's worth having some helper. (Not in this PR)

Yeah, worthy of a helper for sure.

tribble · 2024-07-25T18:03:48Z

tests/test_sso.py

+        request_params = request_kwargs["params"]
+        assert request_params["connection_type"] == "GenericSAML"


For tests like this I wonder if we should explicitly assert all the params, like assert {"connection_type": "GenericSaml", "limit": 10} == request_kwargs["params"] or something similar. We know exactly how we're calling list_connections. it seems like we should know exactly how that translates into the HTTP request.

That's a great idea. Updating.

tribble · 2024-07-25T18:37:28Z

workos/sso.py


        params = {
-            "connection_type": connection_type.value if connection_type else None,
+            "connection_type": connection_type,


We are not consistent about when we always add a key (even if it's None) or only add the key to the params if it's specified. I'm pretty sure I'm guilty of this as well. We should pick a style and go with it.

... and I realized, if we do want to omit missing keys, I wonder if it's easier to come full circle and define our params as BaseModels.

Then we could just....

params = ConnectionParams(connection_type=connection_type) # when it's time to use the params Request(params=ConnectionParams.model_dump(exclude_unset=True))

Utilizing BaseModel could work. Also worth noting the HTTP client has a helper that removes all None values from query params before sending the request.

Oh. In that case I guess we don't need to care.

tribble · 2024-07-25T18:39:42Z

workos/sso.py

+        return WorkOsListResource(
+            list_method=self.list_connections,
+            list_args=params,
+            **ListPage[Connection](**response).model_dump(exclude_unset=True),


I explicitly added exclude_unset to the events endpoint because I didn't want to serialize the before key, which does not exist in the event list metadata.

Do you think we should extend this to all list endpoints? It might be moot, the API may not omit any fields.

Oops, nope. Just a copy-pasta. Removing!

tribble · 2024-07-25T18:40:32Z

workos/sso.py

            "domain": domain,
            "organization_id": organization_id,
-            "limit": limit,
+            "limit": limit or DEFAULT_RESPONSE_LIMIT,


Same here, didn't this get a value above?

Ah yep, now that I have Pylance working I can see this is unnecessary.

tribble · 2024-07-25T18:44:31Z

workos/utils/connection_types.py

+ConnectionType = Literal[
+    "ADFSSAML",
+    "AdpOidc",
+    "AppleOAuth",
+    "Auth0SAML",
+    "AzureSAML",
+    "CasSAML",
+    "CloudflareSAML",
+    "ClassLinkSAML",
+    "CyberArkSAML",
+    "DuoSAML",
+    "GenericOIDC",
+    "GenericSAML",
+    "GitHubOAuth",
+    "GoogleOAuth",
+    "GoogleSAML",
+    "JumpCloudSAML",
+    "KeycloakSAML",
+    "LastPassSAML",
+    "LoginGovOidc",
+    "MagicLink",
+    "MicrosoftOAuth",
+    "MiniOrangeSAML",
+    "NetIqSAML",
+    "OktaSAML",
+    "OneLoginSAML",
+    "OracleSAML",
+    "PingFederateSAML",
+    "PingOneSAML",
+    "RipplingSAML",
+    "SalesforceSAML",
+    "ShibbolethGenericSAML",
+    "ShibbolethSAML",
+    "SimpleSamlPhpSAML",
+    "VMwareSAML",
+]


The API types also include pending and Auth0Migration types.

I think whoever owns SSO (is that us? LOL) could give us guidance around whether it's safe to omit these.

Noticed that also. Let me ask.

tribble · 2024-07-25T18:44:44Z

workos/utils/http_client.py

@@ -172,3 +172,26 @@ async def request(
        )
        response = await self._client.request(**prepared_request_parameters)
        return self._handle_response(response)
+
+
+class FakeHTTPClient(SyncHTTPClient):


tribble

Sheep it

mattgd self-assigned this Jul 25, 2024

mattgd added 2 commits July 25, 2024 12:29

Add typing and async support for SSO.

9cbcb89

Compatibility fix.

d0130fe

mattgd force-pushed the feature/dsync-1735-add-types-to-connection-api-methods branch from 55dbdaf to d0130fe Compare July 25, 2024 16:30

mattgd marked this pull request as ready for review July 25, 2024 16:31

mattgd requested a review from a team as a code owner July 25, 2024 16:31

mattgd requested review from amygdalama and tribble and removed request for a team July 25, 2024 16:31

tribble reviewed Jul 25, 2024

View reviewed changes

Fixes based on feedback.

97fc7da

mattgd requested a review from tribble July 25, 2024 19:55

Remove print.

cb62cc6

tribble approved these changes Jul 25, 2024

View reviewed changes

mattgd merged commit 2894241 into beta-5.0 Jul 25, 2024
5 checks passed

mattgd deleted the feature/dsync-1735-add-types-to-connection-api-methods branch July 25, 2024 20:04

tribble pushed a commit that referenced this pull request Jul 30, 2024

Add typing and async support for SSO (#291)

67c262b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typing and async support for SSO #291

Add typing and async support for SSO #291

mattgd commented Jul 25, 2024

linear bot commented Jul 25, 2024

tribble left a comment

tribble Jul 25, 2024

tribble Jul 25, 2024

tribble Jul 25, 2024

tribble Jul 25, 2024

mattgd Jul 25, 2024

tribble Jul 25, 2024

tribble Jul 25, 2024

mattgd Jul 25, 2024

tribble Jul 25, 2024

mattgd Jul 25, 2024

tribble Jul 25, 2024

tribble Jul 25, 2024

mattgd Jul 25, 2024

tribble Jul 25, 2024

tribble Jul 25, 2024

mattgd Jul 25, 2024

tribble Jul 25, 2024

mattgd Jul 25, 2024

tribble Jul 25, 2024

mattgd Jul 25, 2024

tribble Jul 25, 2024

tribble left a comment

		request_params = request_kwargs["params"]
		assert request_params["connection_type"] == "GenericSAML"

Add typing and async support for SSO #291

Add typing and async support for SSO #291

Conversation

mattgd commented Jul 25, 2024

Description

Documentation

linear bot commented Jul 25, 2024

tribble left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tribble left a comment

Choose a reason for hiding this comment