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

[PM-12606] Move Vault Timeout and Vault Timeout Settings to Key Management #12517

Open
wants to merge 7 commits into
base: km/pm-11528/move-lock-to-km
Choose a base branch
from

Conversation

jlf0dev
Copy link
Member

@jlf0dev jlf0dev commented Dec 20, 2024

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-12606

📔 Objective

Move Vault Timeout and Vault Timeout Settings to Key Management. Tested web, extension, desktop, and CLI.

This is mostly pure moves, only real change was exporting VaultTimeoutService as DefaultVaultTimeoutService and VaultTimeoutSettingsService as DefaultVaultTimeoutSettingsService. This meant renaming in consuming files.

Upstream PR is waiting for QA and should be stable.

📸 Screenshots

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

Copy link
Contributor

github-actions bot commented Dec 20, 2024

Logo
Checkmarx One – Scan Summary & Details7b3f1922-916b-44df-af29-fa6fa6075d20

New Issues

Severity Issue Source File / Package Checkmarx Insight
HIGH CVE-2024-11395 Npm-electron-33.2.1 Vulnerable Package
MEDIUM Missing_HSTS_Header /libs/common/src/services/api.service.ts: 230 Attack Vector

@jlf0dev jlf0dev marked this pull request as ready for review December 20, 2024 21:01
@jlf0dev jlf0dev requested review from a team as code owners December 20, 2024 21:01
Copy link
Contributor

@vleague2 vleague2 left a comment

Choose a reason for hiding this comment

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

DS file looks good!

Copy link
Contributor

@quexten quexten left a comment

Choose a reason for hiding this comment

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

(I did not comment on all instances) We have a few relative imports left over, unless I'm missing the reason for having them can we clean them up?

Copy link
Contributor

@quexten quexten left a comment

Choose a reason for hiding this comment

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

Nice work, LGTM!

Copy link
Contributor

@quexten quexten left a comment

Choose a reason for hiding this comment

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

Nice work on cleaning up the relative imports, looks like there is still some build errors, so revoking the mistaken approval.

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