Skip to content

Add create-by audit for scenario creation (again) #13

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kanakanajm
Copy link
Collaborator

Record user IDs and realm/LHA/Org IDs when creating new scenario. User IDs and Org IDs are extracted from the authentication middleware. I only changed the db model for Scenario but not any DTOs in between (I just changed the signature of those functions in controller and tasks). So far the User IDs are UUIDs generated by Keycloak, not username but we can change that upon request and the Org IDs are just the realm IDs on Keycloak as well. I think we might need to map those onto county code IDs?

Sorry for the weird commit and revert I made on main branch. I accidentally committed there.

@Kanakanajm Kanakanajm requested a review from NXXR July 16, 2025 18:27
@Kanakanajm Kanakanajm self-assigned this Jul 16, 2025
return await controller.create_scenario(
scenario,
request.state.user.userId if request.state.user else None,
request.state.realm if request.state.realm else None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the LHA/Org IDs need to be mapped to the county codes.

IIRC, Mariama/Jonas mentioned that this mapping already exists. Maybe @JonasGilg can point us to it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NXXR
Copy link
Collaborator

NXXR commented Jul 17, 2025

Mapping LHA IDs to county codes (or list of nuts) would be useful sooner or later.
Although atm I don't see a reason why we need this immediately 🤔
For access control we would add the LHA/realm IDs to the whitelist anyway no need to go with a mapping detour since access is based on LHAs not country code or similar.
Same when creating scenarios with private LHA data, this would be shared between LHA <-> LHA not for a specific county code, right? ._.

Copy link
Collaborator

@NXXR NXXR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Jackie,
you need to add the UserId and OrgId to the App's models (at least in /ESID-Backend/api/src/api/app/models/scenario.py) otherwise it will not get returned in the API request but only stored in the DB.
You can still pass it along until the db task and put it in there, since the request only returns the id anyways, or write both into the scenario object as soon as you have the info, both is fine.
Finally, you then also need to fix the scenario_get_by_id task to write the additional ids from the db into the app's Scenario DTO so they get returned properly.

Comment on lines +31 to +32
userId: Optional[str] = Field(default=None, nullable=True) # Created by user
orgId: Optional[str] = Field(default=None, nullable=True) # Created by user's LHA/Organization
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is the DB model this should probably be a UUID.

Suggested change
userId: Optional[str] = Field(default=None, nullable=True) # Created by user
orgId: Optional[str] = Field(default=None, nullable=True) # Created by user's LHA/Organization
CreatorUserId: Optional[uuid.UUID] = Field(default=None, nullable=True)
CreatorOrgId: Optional[uuid.UUID] = Field(default=None, nullable=True)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User IDs from Keycloak are UUIDs but Org IDs are just strings.

@NXXR
Copy link
Collaborator

NXXR commented Jul 17, 2025

⚠️ Important note for us:
When merging and deploying this we also need to create an alembic migration (see last 2 steps of the setup.md) in order to create the missing columns in the databases. previous entries int he DB will just have null in these fields so it's fine to migrate even with existing data in the database.

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

Successfully merging this pull request may close these issues.

4 participants