Skip to content
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

Safe mapping #35804

Merged
merged 2 commits into from
Feb 19, 2025
Merged

Safe mapping #35804

merged 2 commits into from
Feb 19, 2025

Conversation

kaapstorm
Copy link
Contributor

Technical Summary

I noticed that with the current way we do mapping, it is possible to expose user secrets. With this change, I realised that maybe we could drop the "source" property in mappings, and got back to using a dictionary. Curious to hear your thoughts.

Feature Flag

KYC_INTEGRATIONS

Safety Assurance

Safety story

Local testing

Automated test coverage

Tests included

QA Plan

No QA planned

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

If a field is in a user's usercase, or custom user data, then use that,
otherwise fall back to a CommCareUser property. Only allow safe
properties.
@kaapstorm kaapstorm added the product/invisible Change has no end-user visible impact label Feb 19, 2025
@dimagimon dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Feb 19, 2025
@kaapstorm kaapstorm changed the base branch from master to nh/kyc/service February 19, 2025 13:46
@kaapstorm
Copy link
Contributor Author

Please note the base branch is currently nh/kyc/service

Copy link
Contributor

@zandre-eng zandre-eng left a comment

Choose a reason for hiding this comment

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

Nice catch with this PR. I like the changes, particularly since it now enforces what properties we can pull and makes the mapping process a bit more simple to set up.
If we're no longer going to need a "source" field then I think making the mapping field a dict again sounds like a good idea.

@kaapstorm kaapstorm merged commit 945da40 into nh/kyc/service Feb 19, 2025
13 of 14 checks passed
@kaapstorm kaapstorm deleted the nh/mapping branch February 19, 2025 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants