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

Improve credential logging and choice #2064

Conversation

jemrobinson
Copy link
Member

@jemrobinson jemrobinson commented Jul 31, 2024

✅ Checklist

  • You have given your pull request a meaningful title (e.g. Enable foobar integration rather than 515 foobar).
  • You are targeting the appropriate branch. If you're not certain which one this is, it should be develop.
  • Your branch is up-to-date with the target branch (it probably was when you started, but it may have changed since then).

🚦 Depends on

n/a

⤴️ Summary

Currently credential confirmation is written to the logs in unnecessary places (e.g. in the Pulumi output) and the user is not given a chance to check that the credentials are correct.

This adds:

  • a confirmation prompt the first time a set of credentials are used
  • suppresses credential information
    • after approval has been given
    • inside Pulumi (which is run as a separate process, so the previous class-variable solution was not working)

🌂 Related issues

n/a

🔬 Tests

Tested on a fresh SRE deployment and on update

@jemrobinson jemrobinson force-pushed the reduce-credentials-log-messages branch from c5ea19a to f0f3b7e Compare July 31, 2024 11:16
@jemrobinson jemrobinson marked this pull request as ready for review July 31, 2024 11:16
@jemrobinson jemrobinson requested a review from a team as a code owner July 31, 2024 11:16
Copy link

github-actions bot commented Jul 31, 2024

Coverage report

Click to see where and how coverage changed

FileStatementsMissingCoverageCoverage
(new stmts)
Lines missing
  data_safe_haven/external/api
  azure_sdk.py
  credentials.py
Project Total  

This report was generated by python-coverage-comment-action

@jemrobinson jemrobinson changed the title WIP Improve credential logging and choice Improve credential logging and choice Jul 31, 2024
@jemrobinson jemrobinson requested review from craddm, JimMadge and a team July 31, 2024 21:17
@JimMadge
Copy link
Member

JimMadge commented Aug 1, 2024

We recently removed the prompt for users to confirm the credentials, was that because it was difficult to keep in restructuring or because we decided it wasn't a good idea?

@craddm
Copy link
Contributor

craddm commented Aug 1, 2024

Was maybe because it was super annoying to have to reconfirm every time? Doing it the first time makes sense

@jemrobinson
Copy link
Member Author

Because it was prompting during non-interactive sessions (i.e. during Pulumi calls) which caused lock-ups. With this PR we can set the skip_confirmation flag in the Pulumi code which should mean that the prompt (and associated messages) only happen in the interactive part of the code.

Copy link
Member

@JimMadge JimMadge left a comment

Choose a reason for hiding this comment

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

Looks good but I think we should make sure to test the confirmation behaviour.

data_safe_haven/external/api/credentials.py Show resolved Hide resolved
@jemrobinson jemrobinson requested a review from JimMadge August 1, 2024 13:26
@JimMadge JimMadge merged commit 63539c4 into alan-turing-institute:develop Aug 1, 2024
11 checks passed
@jemrobinson jemrobinson deleted the reduce-credentials-log-messages branch January 30, 2025 11:47
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.

3 participants