Add basic auth client setup #1

MrCreosote · 2025-09-22T00:16:24Z

Adds

A mininal async auth client
an unasync script to build a sync client from the above
docker compose set up for an auth server to test against
a test rig that sets up the auth server with users and roles (not used yet)
tests for the current functionality other than error handling code that can't yet be reached
eventual publishing on pypi with uv, need to look into this further
Github actions

Adds * A mininal async auth client * an unasync script to build a sync client from the above * docker compose set up for an auth server to test against * a test rig that sets up the auth server with users and roles (not used yet) * tests for the current functionality other than error handling code that can't get be reached * eventual publishing on pypi with uv, need to look into this further * Github actions

github-advanced-security · 2025-09-22T00:17:23Z

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

codecov · 2025-09-22T00:17:31Z

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

ialarmedalien · 2025-09-22T14:34:55Z

.github/dependabot.yml

+    open-pull-requests-limit: 25
+
+  # Python
+  - package-ecosystem: "pip" # See documentation for possible values


you can now use "uv" as the value for python repos with uv dependency management

https://docs.astral.sh/uv/guides/integration/dependency-bots/#dependabot

Hmm, the issue linked in those docs says it doesn't update the pyproject.toml file yet, sounds like it's not quite ready for prime time

ialarmedalien · 2025-09-22T14:36:18Z

.github/workflows/test.yml

+        export UV_PROJECT_ENVIRONMENT="${pythonLocation}"
+        uv sync --locked


uv can also manage your python installation if you have committed the .python-version file, so you don't need the python install step or the UV_PROJECT_ENVIRONMENT env var

Right, but the current way means we can matrix test multiple versions of python if we want to, which I'm guessing we'll probably want to

May want to remove the .python-version file in that case.

I'm not sure how that file interacts with the UV_PROJECT_ENVIRONMENT variable - it might cause the file to be ignored. I'd like to keep the file for when people are installing deps locally. It it becomes an issue we can delete it in the test.yml file

ialarmedalien · 2025-09-22T14:38:23Z

README.md

+
+## Installation
+
+TODO INSTALL setup a KBase pypi org and publish there


please announce when you do this as there are a few other packages that it'd be great to put up there

That's the plan

ialarmedalien · 2025-09-22T14:39:53Z

.gitignore

+/.pytest_cache/
+__pycache__
+/.venv/


do you need to add in the directories that get created when you run uv build so you don't get the build gubbins committed to the repo?

I haven't built a package with uv yet so I didn't know about the gubbins, but when I do I'll update the ignore

ialarmedalien · 2025-09-22T14:47:25Z

src/kbase/auth/client.py

+from kbase.auth._async.client import AsyncClient as AsyncKBaseAuthClient  # @UnusedImport
+from kbase.auth._sync.client import Client as KBaseAuthClient  # @UnusedImport


what tool/IDE is the @UnusedImport annotation for? ruff uses unused-import or F401.

I don't know why these IDEs and linters can't get their acts together and decide on a shared syntax for disabling linting for a rule or a line.

yeah, that'd be nice

ialarmedalien · 2025-09-22T14:49:26Z

test/test_client.py

+    with pytest.raises(type(expected)) as e:
+        KBaseAuthClient.create(url)
+    assert str(e.value) == expected.args[0]


with pytest.raises(type(expected), match="some string to match"): KBaseAuthClient.create(url)

ialarmedalien · 2025-09-22T14:50:50Z

test/test_client.py

@@ -0,0 +1,63 @@
+import pytest
+
+from conftest import AUTH_URL, AUTH_VERSION, auth_users  # @UnusedImport


why import something if it is not used?

It's used in the last 2 tests. PyDev doesn't realize it's a fixture since it just looks like an argument to a method

PyDev is not making a strong case for its adoption ATM...

I'm actually curious at to how many tools would catch this. They'd have to know the specifics of pytest fixtures, and if the method isn't marked as a test they don't really have enough info to know that the function / method argument is a fixture

Any fixture in conftest is already discoverable in test files by pytest, so adding it to direct imports can cause weird behavior for any dev thing looking at it.

Does PyDev whine if it's not included as an import?

Huh, it does not. Well that's neat, I've been doing that for years now. I wonder how I got that in my head.

Anyway, removed

maybe I started putting the import there because otherwise it's not clear where the fixture comes from to the uninitiated

ialarmedalien · 2025-09-22T14:52:14Z

test/conftest.py

+import time
+
+
+# settings are coming from the docker-compose file


"settings are coming from" ==> either "settings come from" or "settings are from"

I mean all 3 are grammatically correct I think, but in the interests of brevity I've updated it

ialarmedalien · 2025-09-22T14:54:51Z

test/conftest.py

+                        status = j["Health"]
+                        if status == "healthy":


could just check the content of j["Health"] without saving it to a variable -- if j.get("Health") ...

status is used a few lines down in the method

ialarmedalien · 2025-09-22T14:57:10Z

pyproject.toml

+    "pytest-cov==7.0.0",
+    "requests==2.32.5",
+    "unasync==0.6.0",
+]


I would encourage good practices by adding in a code formatting/linting section with a ruff config here. Just makes life easier not to have to worry about code format and dumb errors that can be caught by tools.

I'm not a big fan linters at this point, they're too opinionated and catch mostlyi trivial stuff

I much prefer using a code formatter wherever possible -- if everyone's code is laid out the same, it's much quicker to read, review, and understand.

Linters catch all kinds of useful errors, especially if you have devs with different levels of experience working on a project. You or I might have the ability to produce perfect code every time, but that isn't true for everyone.

As for being opinionated, what's that expression about the pot and the kettle...? 😉

I'm definitely opinionated, but when it comes to code style I think I keep my opinions in check these days. As long as it's readable I don't care too much.

If we start having junior devs work on this repo I'm willing to revisit adding a code formatter, but right now the ROI just isn't there. I've spent too much time setting up, configuring, and integrating code formatters only to have them waste my time catching trailing whitespace (cough codacy cough). Right now there's little to no benefit IMO

It is extremely quick and easy to set up ruff to format your code -- it even looks as though PyDev has ruff as one of the formatters. I believe ruff catches trailing whitespace, and any decent IDE should also have a setting for it. You can also add a standard .editorconfig file to do that -- e.g. https://github.com/linkml/linkml/blob/main/.editorconfig

We have already spent more time debating this than it would have taken to add a section to the pyproject.toml file with the setup for formatting and linting.

But not more time than it will take in the future having test runs fail due to trivial issues, fixing said trivial issues that don't need to be fixed, and tweaking the config until everyone's equally annoyed with it. I've been through this cycle several times before with many different linters; it's not necessary and not worth the effort

I have had the opposite experience.

Well, it seems we're at an impasse - can you agree to leave things as they are for now and add a linter when / if it becomes an issue?

ialarmedalien · 2025-09-22T14:58:02Z

src/kbase/auth/_async/client.py

+        j = r.json()
+    except Exception:
+        err = "Non-JSON response from KBase auth server, status code: " + str(r.status_code)
+        # TDOO TEST LOgging in the future


two typos in one line. The horror!

I may not survive the spiral of shame I'm in

Anyway, fixed

ialarmedalien · 2025-09-22T15:05:35Z

src/kbase/auth/_async/client.py

+"""
+A client for the KBase Authentication service.
+"""


Could you add a line here that indicates that the sync client is derived from the asynchronous client, so that if people don't read the README, they won't go and waste a load of effort editing two files when they could just edit one?

Something neutral that could appear in either file, e.g. "The sync auth client is derived from the async client, and any edits should be made to src/kbase/auth/_async/client.py and propagated using the script process_unasync.py".

Fixed, although I'm referencing the readme for instructions so there's a single point of truth

You would (or wouldn't) be surprised how many people don't bother with the README but launch straight into the code. Not that I have ever done that, of course.

I wouldn't be surprised in the least

There's a README here?

Yes? Not sure if this is a joke I'm missing or a serious question

ialarmedalien · 2025-09-22T15:07:53Z

src/kbase/auth/_async/client.py

+            raise
+        # TODO CLIENT look through the myriad of auth clients to see what functionality we need
+        # TODO CLIENT cache token & user using cachefor value from token
+        # TODO RELIABILIY could add retries for these methods, tenacity looks useful


TODO spelling of RELIABILITY
TODO add a space in "token & user using cachefor value"

Fixed reliability

cachefor is actually the field in the token data structure, so I'm leaving that

ialarmedalien · 2025-09-22T15:08:24Z

src/kbase/auth/_async/client.py

+
+import httpx
+import logging
+from typing import Self


where is this used?

It's the return type of the create() classmethod

Ah, seen it - thanks!

ialarmedalien · 2025-09-22T15:10:08Z

src/kbase/auth/_async/client.py

+
+def _check_request(r: httpx.Request):
+    try:
+        j = r.json()


I know that characters are at a premium right now, but could you spare a few more for these variable names? j is rather terse and uninformative.

Thanks to you my keyboard is now running out of charact

ialarmedalien · 2025-09-22T15:18:46Z

test/conftest.py

+        # TODO TEST add a way to keep things running and be able to rerun tests
+        _run_dc(env, "down")


Alternatively, have a wrapper script that does all the docker set up stuff, executes pytest, and then shuts everything down. That would enable you to keep your other docker containers running whilst working on the python container.

That's basically what I would make this do - just leave it up after running tests the first time, and then shut it down with docker compose down

ialarmedalien · 2025-09-23T14:22:58Z

FYI - for some reason GitHub will not let me resolve conversations, so I have not been able to achieve closure on some of these points.

... which is what it's actually doing

MrCreosote · 2025-09-23T16:12:55Z

Changed the _checK_request function to _check_response for accuracy

briehl

Just a minor change along with @ialarmedalien 's comments..

briehl · 2025-09-23T17:12:19Z

src/kbase/auth/_async/client.py

+"""
+A client for the KBase Authentication service.
+"""


There's a README here?

briehl · 2025-09-23T17:27:39Z

test/test_client.py

@@ -0,0 +1,63 @@
+import pytest
+
+from conftest import AUTH_URL, AUTH_VERSION, auth_users  # @UnusedImport


Any fixture in conftest is already discoverable in test files by pytest, so adding it to direct imports can cause weird behavior for any dev thing looking at it.

Does PyDev whine if it's not included as an import?

briehl · 2025-09-23T17:30:20Z

test/test_client.py

+        await AsyncKBaseAuthClient.create(url)
+
+
+@pytest.mark.asyncio


You can add a block like:

[tool.pytest.ini_options] asyncio_mode = "auto"

to pyproject.toml and not have to put the annotation on async tests.

I actually prefer the explicit marking of tests

MrCreosote · 2025-09-30T16:38:18Z

@ialarmedalien approved out of band

MrCreosote requested review from briehl and ialarmedalien September 22, 2025 00:16

ialarmedalien reviewed Sep 22, 2025

View reviewed changes

Fix assorted review comments

b981a87

_check_request -> _check_response

b62aa48

... which is what it's actually doing

briehl suggested changes Sep 29, 2025

View reviewed changes

Remove unnecessary import

9e4b74b

briehl approved these changes Sep 30, 2025

View reviewed changes

MrCreosote merged commit 28ed216 into main Sep 30, 2025
5 checks passed

		export UV_PROJECT_ENVIRONMENT="${pythonLocation}"
		uv sync --locked


		## Installation

		TODO INSTALL setup a KBase pypi org and publish there

		from kbase.auth._async.client import AsyncClient as AsyncKBaseAuthClient # @UnusedImport
		from kbase.auth._sync.client import Client as KBaseAuthClient # @UnusedImport No newline at end of file

		@@ -0,0 +1,63 @@
		import pytest

		from conftest import AUTH_URL, AUTH_VERSION, auth_users # @UnusedImport

		import time


		# settings are coming from the docker-compose file

		# TODO TEST add a way to keep things running and be able to rerun tests
		_run_dc(env, "down")

Uh oh!

Add basic auth client setup #1

Add basic auth client setup #1

Uh oh!

Conversation

MrCreosote commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-advanced-security bot commented Sep 22, 2025

Uh oh!

codecov bot commented Sep 22, 2025

Welcome to Codecov 🎉

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ialarmedalien Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrCreosote Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrCreosote Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MrCreosote commented Sep 22, 2025 •

edited

Loading

ialarmedalien Sep 22, 2025 •

edited

Loading

MrCreosote Sep 23, 2025 •

edited

Loading

MrCreosote Sep 22, 2025 •

edited

Loading