feat(client): A few improvements to clients#188
Draft
samoehlert wants to merge 46 commits intomainfrom
Draft
Conversation
Collaborator
samoehlert
commented
Nov 21, 2025
samoehlert
commented
- Autocreate a UUID if one is not passed by a client during registration (recent request)
- Display UUID on the admin page for a client (improve admin life)
- Rename hostname => client_name to prevent confusion (fix from the pentest)
- Make the UUID a unique thing to prevent a collision (fix from the pentest)
- Grab the IP from the registering client and use that to further verify a client (fix from the pentest)
s/hostname/client_name because it was confusing. make UUID a unique field. add the ability to check that a client is bound to a specific IP.
There's probably a better way to do this. Definitely needs a look and think
… IP, and use it to further verify a client
samoehlert
added
bug
Collaborator
Author
|
Please look at the get_ip stuff, it feels fragile and maybe can be made more secure. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7d0386b |
…e error message we can live without that info for now
samoehlert
commented
Dec 17, 2025
samoehlert
commented
Dec 17, 2025
samoehlert
commented
Dec 17, 2025
…g and running tests It seems like we were hitting a subtle problem where it was using cached images which caused it to run the main code instead of my code.
…-security/SCRAM into topic/soehlert/client_improvements
… building and running tests" This reverts commit b42ae97.
…me instead of hostname apparently the failure was because there were changes on main that hadn't been pulled here and they were run causing failures
…endpoints for consistency's sake and explicitness
this lets people register if they add one to the registration POST. it was just set to autocreate one only. this is more flexible.
This lets us still use the autocreate UUID that we've done here
…ts with UUID, create clients without a UUID provided, do an idempotent create, and not leak client_names
…on with UUID and without (for our autocreate UUID code branch) also up our documentation game so we have much better info in DRF spectacular
…ration tests (#157) This one was a doozie. Expired/deleted entries on one SCRAM instance were never unblocked on other instances because there is no mechanism for postgres to let django know that it needs to re-run stuff, only the active django re-runs stuff. Now, we get around that by calling process_updates and re-updating anything changed recently, however, before, the sync logic only looked at creation time (`when` field), missing any entries that were removed.. To fix this in a lot less dumb way than we used to, we now we use `django-simple-history` to detect _**any**_ entry changes and then go ahead and just reprocess them to what the DB says they should be doing. There is a lot in this PR, but the main changes are: - Split up `process_updates()` into: - `get_entries_to_process(cutoff_time)` - This is a function that we could reuse that simply finds all recently modified entries from other instances and returns them so you can Do Stuff® on em. - `reprocess_entries(entries)` - This actually "Does Stuff" by sending websocket messages to translators. We could eventually move this out of here and make it more generic to be used everywhere we Do A Websocket® but it needs to be a bit more dynamic to support future action types (see note below, not dealing with this now). - `_check_for_orphaned_history()` - This is a total edge case, but since we don't ever delete entries (our delete() override) I wanted to make sure that we catch and log any orphaned history objects that don't have a corresponding entry (say someone goes into the admin panel and actually "hard" deletes an entry, but the history stuff is still there). - Made sure to be efficient with DB calls by using `select_related()`,`.exclude()`, and `values_list()` on most of our django/db queries to make sure that we're relying on SQL to do the filtering (using built in django stuff obviously) instead of iterating on the python side (in the case of prod where we have a TON of entries.) - We used to rely on `msg.msg_type` from WebSocketMessage (which is always `translator_add`), but now we set that based on `entry.is_active` when we run the sync from DB to translator. Eventually though this needs to be more dynamic to support other action types, not sure how to solve that though. There are a few caveats here to keep in mind, but for now i think it's fine: - This breaks from the existing behave patterns, and adds a pattern where we connect directly to the containers running via compose and ignores the test database. I tried to seperate this out into full integration tests. This means that test data is left behind so we blow away everything in the DB to reset it before and after test runs. - It's possible that a docker health check could run while the behave integration tests are running. This is unlikely and I made the health check run less frequently just to help avoid that but... it's gross. For now, I think this gets us further, so it's still good. It also fixes things to use postgres instead of sqlite, which is a win overall. # Conflicts: # scram/route_manager/tests/acceptance/steps/common.py # scram/route_manager/tests/acceptance/steps/ip.py
also added pre commit config that hopefully doesnt break anything because i couldnt commit otherwise
how can there be any left i keep looking
Collaborator
crankynetman
left a comment
crankynetman
left a comment
There was a problem hiding this comment.
Looks really great, I especially love the OpenAPI stuff. I have a few questions inline about the IP stuff though.
scram/route_manager/migrations/0036_rename_hostname_client_client_name.py
Show resolved
Hide resolved
…moving the relevant code
… and make sure unique client uuid points to the correct dependency
|
Pytest Celery Scheduler Coverage
Minimum allowed coverage is Generated by 🐒 cobertura-action against 875c671 |
* main: refactor(settings): move translator to pydantic settings (#221)
…sary ones, and fix up the exception logic per ruff
|
Django Pytest/Behave Coverage
Minimum allowed coverage is Generated by 🐒 cobertura-action against 875c671 |
…ptions to not take the bare exception
samoehlert
commented
Mar 11, 2026
| serializer = self.get_serializer(existing_client) | ||
| return Response(serializer.data, status=status.HTTP_200_OK) | ||
|
|
||
| # Log the conflict without leaking client_names to the user |
Collaborator
Author
There was a problem hiding this comment.
I question this
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.