ansible / django-ansible-base

Apache License 2.0
11 stars 43 forks source link

Make ansible_id match the JWT data if creating new user #512

Closed AlanCoding closed 1 month ago

AlanCoding commented 1 month ago

Deep, deep, deep into a debugging hole, I thought I fixed the issue where the "admin" user had a resource conflict. But then it was still a conflict.

It was a conflict created by an entirely new mechanism. The JWT consumer was creating users from the JWT data that had a different ansible_id.

NOTE: We seem to have another problem where the service_id is not set for absolutely anything created in this file.

AAP-26659

AlanCoding commented 1 month ago

Trying to give a skeptical self-review of this - the logic I'm building here starts to overlap significantly with get_or_create_resource, although that doesn't have the features I want to be thorough.

Outline of the abstract case

We are given an ansible_id and resource_data. We should also have a service_id, but we don't, that that's another novel issue.

We prioritize looking up the object by ansible_id, and we may or may not find it. If we find it, we're great! Move on. If we don't find it, then we want to create it. Creating the object may end in conflict, due to a pre-existing resource conflict. A conflict doesn't necessarily imply this type of conflict, because it could be due to another unknown database constraint implemented by the application. In this last case we should prefer to let it error and just log what happens, maybe catching any DatabaseError generally.

Problems with the existing method

The existing method would just error in the event of pre-existing resource conflicts. Should it?

There's no generalization of error handling in this respect, mainly because it doesn't do error handling. So we could tighten this code up in a couple of ways.

AlanCoding commented 1 month ago

I tried to look into ways to make this better, and I just can't. There's too much model-specific behavior to do a light refactor which combines the get-by-ansible_id logic for user/teams/orgs. We could log the created attribute, but that's all we would be doing, or I could change the get_or_create to just a "create". I'm happy with this as it is now.

sonarcloud[bot] commented 1 month ago

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud