feat: Profile storage v2 with secure credential storage (#107)#111
feat: Profile storage v2 with secure credential storage (#107)#111joshsmithxrm merged 11 commits intomainfrom
Conversation
Summary of ChangesHello @joshsmithxrm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant overhaul to how authentication profiles and sensitive credentials are managed. The primary goal is to bolster security by moving secrets from plain JSON files into platform-native secure storage mechanisms. This change also modernizes the profile storage schema for better maintainability and ensures that credential cleanup is robustly handled when profiles are deleted or cleared. These updates provide a more secure and streamlined authentication experience across different operating systems. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
joshsmithxrm
had a problem deploying
to
test-dataverse
GitHub Actions
Failure
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a major and well-executed enhancement by migrating profile secrets to platform-native secure storage, significantly improving security. The profile storage schema is also updated to version 2, and the v1-to-v2 migration path is handled by deleting old profiles, which is clearly documented as a breaking change. The changes are comprehensive, touching everything from credential providers and profile storage to CLI commands and tests. The new SecureCredentialStore is robust, using MsalCacheHelper for cross-platform encryption and including good practices like atomic file writes. The test suite has been updated thoroughly, with new unit tests for the secure store and E2E tests for credential cleanup, which is excellent.
I have found one high-severity issue in the credential cleanup logic for the auth delete command, which could cause credentials to be deleted prematurely if shared between profiles. My review includes a detailed comment and a code suggestion to fix this.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
tests/PPDS.Auth.Tests/Credentials/CredentialProviderFactoryTests.cs
Dismissed
Show dismissed
Hide dismissed
tests/PPDS.Auth.Tests/Credentials/SecureCredentialStoreTests.cs
Dismissed
Show dismissed
Hide dismissed
tests/PPDS.Auth.Tests/Credentials/SecureCredentialStoreTests.cs
Dismissed
Show dismissed
Hide dismissed
tests/PPDS.Auth.Tests/Credentials/SecureCredentialStoreTests.cs
Dismissed
Show dismissed
Hide dismissed
tests/PPDS.Auth.Tests/Credentials/SecureCredentialStoreTests.cs
Dismissed
Show dismissed
Hide dismissed
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive overhaul of profile storage, introducing schema v2 with platform-native secure credential storage and automatic migration from v1 profiles.
Key Changes:
- Secure credential storage: Secrets (client secrets, certificate passwords, user passwords) are now stored separately using MSAL.Extensions in platform-native secure storage (DPAPI on Windows, Keychain on macOS, libsecret on Linux with cleartext fallback)
- Schema v2 migration: Profiles now use array-based storage with name-based active profile tracking instead of dictionary with numeric index; v1 profiles are automatically deleted on first load
- Enhanced credential cleanup:
auth deleteandauth clearcommands now properly remove credentials from secure storage
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
tests/PPDS.LiveTests/Cli/AuthCommandE2ETests.cs |
Added E2E tests for credential cleanup on auth delete and auth clear |
tests/PPDS.Auth.Tests/Profiles/ProfileStoreTests.cs |
Updated tests for v2 schema (array-based profiles, name-based active profile) and v1 migration logic |
tests/PPDS.Auth.Tests/Profiles/ProfileCollectionTests.cs |
Updated tests to use ActiveProfileName instead of ActiveIndex |
tests/PPDS.Auth.Tests/Profiles/EnvironmentInfoTests.cs |
Removed tests for Id property (removed field) |
tests/PPDS.Auth.Tests/Profiles/AuthProfileTests.cs |
Removed validation tests for fields now stored in secure credential store |
tests/PPDS.Auth.Tests/Credentials/SecureCredentialStoreTests.cs |
New comprehensive unit tests for SecureCredentialStore functionality |
tests/PPDS.Auth.Tests/Credentials/CredentialProviderFactoryTests.cs |
Updated tests for async factory methods and environment variable support |
src/PPDS.Cli/Commands/Auth/AuthCommandGroup.cs |
Updated auth commands to use secure credential store; added --accept-cleartext-caching option; reordered auth who output |
src/PPDS.Cli/CHANGELOG.md |
Documented breaking changes and new features |
src/PPDS.Auth/Profiles/ProfileStore.cs |
Implemented v1 schema detection and deletion; removed encryption logic (now in SecureCredentialStore) |
src/PPDS.Auth/Profiles/ProfilePaths.cs |
Added CredentialCacheFileName constant |
src/PPDS.Auth/Profiles/ProfileCollection.cs |
Changed from dictionary to array-based storage; uses ActiveProfileName instead of ActiveIndex |
src/PPDS.Auth/Profiles/EnvironmentInfo.cs |
Removed redundant Id property |
src/PPDS.Auth/Profiles/AuthProfile.cs |
Removed ClientSecret, CertificatePassword, Password properties; added Authority property |
src/PPDS.Auth/Credentials/SecureCredentialStore.cs |
New implementation of platform-native secure credential storage using MSAL.Extensions |
src/PPDS.Auth/Credentials/JwtClaimsParser.cs |
Simplified to only extract PUID claim |
src/PPDS.Auth/Credentials/ISecureCredentialStore.cs |
New interface and DTOs for secure credential storage |
src/PPDS.Auth/Credentials/CredentialProviderFactory.cs |
Added async CreateAsync method; support for PPDS_SPN_SECRET environment variable |
src/PPDS.Auth/Credentials/ClientSecretCredentialProvider.cs |
Updated factory methods to accept credentials from secure store or environment variable |
src/PPDS.Auth/Credentials/CertificateFileCredentialProvider.cs |
Updated factory methods to accept certificate password from secure store |
src/PPDS.Auth/CHANGELOG.md |
Documented breaking changes and new features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…torage (#107) ## Summary - Add SecureCredentialStore using MSAL.Extensions for platform-native credential storage - Migrate profile schema from v1 (dict) to v2 (array with name-based active profile) - Add geographic fields (userCountry, tenantCountry) from JWT claims - Move secrets (clientSecret, certificatePassword, password) to secure store ## Key Changes - **SecureCredentialStore**: Windows DPAPI, macOS Keychain, Linux libsecret - **Schema v2**: profiles as array, activeProfile by name, authority field - **v1 Migration**: Pre-release breaking change - detects and deletes v1 files - **PPDS_SPN_SECRET**: Env var bypasses secure store for CI/CD scenarios 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Reorder the fields in ppds auth who to match pac auth who: - Add TenantCountry after TenantId - Move Entra ID Object Id before PUID - Add User Country/Region after PUID - Move Token Expires before Authority - Reorder Environment section (Geo, Id, Type, Org Id, Unique Name, Friendly Name) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Remove UserCountry and TenantCountry from AuthProfile (claims not available in token without app manifest configuration) - Remove environment.id from EnvironmentInfo (redundant with OrganizationId and EnvironmentId, was never populated) - Simplify JwtClaimsParser to only extract PUID - Update EnvironmentInfo.Create() to take url and displayName only - Update all related tests 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Document breaking changes for #107: - Schema v2 with array storage and name-based active profile - Secure credential storage using MSAL.Extensions - Removed EnvironmentInfo.Id, UserCountry, TenantCountry - auth who output ordering to match PAC CLI 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add --accept-cleartext-caching flag to auth create (Linux-only) Shows warning when secure storage unavailable and cleartext is used - auth delete now removes stored credentials from SecureCredentialStore - auth clear now clears SecureCredentialStore in addition to MSAL caches Addresses missing items from #107 review. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add IsCleartextCachingEnabled property unit tests for SecureCredentialStore - Add E2E test verifying auth delete removes stored credentials - Add E2E test verifying auth clear removes credential store 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
MSAL.Extensions doesn't allow both WithLinuxKeyring() and WithLinuxUnprotectedFile() together - they're mutually exclusive. When --accept-cleartext-caching is passed, now uses ONLY WithLinuxUnprotectedFile(). Otherwise uses ONLY WithLinuxKeyring(). Found via WSL testing. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Check if other profiles share credentials before deleting them when a profile is removed (prevents breaking other profiles) - Clean up stored credentials if auth fails after storing them (prevents orphaned credentials in secure storage) - Add null check for DisplayName in auth who output (consistency) - Add test verifying shared credential preservation behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add missing fixes from bot review: - Auth: Linux cleartext storage uses exclusive MSAL config - Cli: Shared credential preservation on profile delete - Cli: Credential cleanup on auth failure - Cli: DisplayName null check in auth who 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joshsmithxrm
force-pushed
the
feat/profile-storage-v2
branch
from
57f888a to
9755394
Compare
joshsmithxrm
had a problem deploying
to
test-dataverse
GitHub Actions
Failure
…l lookup ProfileConnectionSource and ConnectionResolver were calling the sync CredentialProviderFactory.Create() which only checks PPDS_SPN_SECRET env var. This caused integration tests to fail with "Failed to create seed after 3 attempts" because credentials stored via `auth create` could not be retrieved. Changes: - ProfileConnectionSource: Accept ISecureCredentialStore parameter, use CreateAsync() for credential lookup - ConnectionResolver: Pass credential store to ProfileConnectionSource - ProfileServiceFactory: Create SecureCredentialStore and register in DI - Add E2E test for PPDS_SPN_SECRET env var bypass path 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joshsmithxrm
had a problem deploying
to
test-dataverse
GitHub Actions
Failure
…ential lookup EnvironmentResolutionService.TryDirectConnectionAsync() was calling the sync CredentialProviderFactory.Create() which doesn't support secure store lookups. This caused env select and env who commands to fail with "No client secret found" when using profiles created with auth create. Changes: - EnvironmentResolutionService: Accept ISecureCredentialStore parameter, use CreateAsync() for credential lookup - EnvCommandGroup: Pass SecureCredentialStore to resolution service - AuthCommandGroup: Same for auth update command 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
joshsmithxrm
temporarily deployed
to
test-dataverse
GitHub Actions
Inactive
Summary
auth deleteandauth clearnow properly remove stored credentials from secure storage--accept-cleartext-cachingflag for Linux systems without libsecret installedUserCountry,TenantCountryfrom profiles (JWT claims not available), removed redundantEnvironment.IdfieldChanges
PPDS.Auth
SecureCredentialStore- Platform-native credential storage using MSAL.ExtensionsAuthProfile- RemovedUserCountry,TenantCountrypropertiesEnvironmentInfo- Removed redundantIdpropertyJwtClaimsParser- Simplified to only extract PUID claimPPDS.Cli
auth create- Added--accept-cleartext-cachingflag (Linux-only visibility)auth delete- Now removes stored credentials from SecureCredentialStoreauth clear- Now clears SecureCredentialStore in addition to MSAL cachesauth who- Reordered output to match PAC CLI formatTests
IsCleartextCachingEnabledunit tests for SecureCredentialStoreauth deleteandauth clearTest plan
--warnaserror--accept-cleartext-caching(warning shown, proceeds)Closes #107
🤖 Generated with Claude Code