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

Upgraded cryptography and related dependencies #35139

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

Conversation

orangejenny
Copy link
Contributor

@orangejenny orangejenny commented Sep 18, 2024

Technical Summary

Resolves https://github.com/dimagi/commcare-hq/security/dependabot/551

Safety Assurance

Safety story

This is a sensitive area - these libraries are mostly used in SSO.

Automated test coverage

There are tests for SSO. I'm not terribly familiar with the area, so I'm also requesting a QA regression of SSO.

QA Plan

https://dimagi.atlassian.net/browse/QA-7124

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

@orangejenny orangejenny added awaiting QA QA in progress. Do not merge product/invisible Change has no end-user visible impact labels Sep 18, 2024
@orangejenny orangejenny requested a review from a team as a code owner September 18, 2024 13:17
@dimagimon dimagimon added the dependencies Pull requests that update a dependency file label Sep 18, 2024
@jingcheng16
Copy link
Contributor

jingcheng16 commented Sep 18, 2024

@orangejenny As long as this passed QA, it should be fine. Feel free to ping me once it passed QA.

I looked at a past PR that upgrades pyopenssl, I saw Biyeun request QA to test both saml and oidc. I hope this will be included in QA's test plan.

msal is used by Entra remote user management, I reviewed the changelog for msal and looks good. But this should also be tested by QA, no need to go over all test in remote user management (to save QA's time and effort), just make sure user will be deactivated when removed from Entra is enough, then we know we successfully connected to the app in Entra . Our test didn't actually sending request to any idp, we patched it.

@orangejenny
Copy link
Contributor Author

Thank you @jingcheng16 for the knowledge, I added notes to the QA ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting QA QA in progress. Do not merge dependencies Pull requests that update a dependency file product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants