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

Fix Environment.getUserRootDirectory() on Non-Windows Platforms #6581

Conversation

rzyns
Copy link
Contributor

@rzyns rzyns commented Oct 20, 2023

fix: Environment.getUserRootDirectory() only ever executes Windows code path

From the Azure msal-node docs:

// You can use the helper functions provided through the Environment class to construct your cache path
// The helper functions provide consistent implementations across Windows, Mac and Linux.
const cachePath = path.join(Environment.getUserRootDirectory(), "./cache.json");

The existing (broken) code:

static getUserRootDirectory(): string | null {
return !this.isWindowsPlatform
? this.getUserHomeDirOnUnix()
: this.getUserHomeDirOnWindows();
}

msal-node-extensions: Environment.getUserHomeDirOnUnix() was never called, because !this.isWindowsPlatform would always be false, since isWindowsPlatform() is a static method and not a property getter (or property)

@github-actions github-actions bot added the extensions Related to extensions for the base libraries label Oct 20, 2023
Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Can you please generate changefiles (npm run beachball:change from the root of the repo) and add a test?

@rzyns rzyns force-pushed the feature/fix-environment-getUserRootDirectory branch from 0000eab to a65b8cb Compare October 20, 2023 22:06
@rzyns
Copy link
Contributor Author

rzyns commented Oct 20, 2023

@tnorling: I amended my original commit with a more informative commit message and a test (and it even fails on the previous commit, but passes with this change 😄). Also ran beachball, so I think everything should be in order now

@rzyns rzyns changed the title Fix constant expression in ternary condition Fix Environment.getUserRootDirectory() on Non-Windows Platforms Oct 20, 2023
@rzyns rzyns force-pushed the feature/fix-environment-getUserRootDirectory branch from 054b68c to 7e5ce20 Compare October 26, 2023 15:22
@tnorling
Copy link
Collaborator

@rzyns Please allow edits from maintainers on this pull request so that we can update from the base branch and get this merged.

@rzyns rzyns force-pushed the feature/fix-environment-getUserRootDirectory branch from 7e5ce20 to 0b8a997 Compare November 1, 2023 13:56
@rzyns
Copy link
Contributor Author

rzyns commented Nov 1, 2023

@rzyns Please allow edits from maintainers on this pull request so that we can update from the base branch and get this merged.

@tnorling, I updated with rebase as I did in #6600, but AFAIK, I can't allow edits from maintainers because the fork is owned by an organization, and not a user-owned repository. If you know a solution to that, please do let me know :)

@tnorling
Copy link
Collaborator

tnorling commented Nov 1, 2023

Please see CI failures related to formatting (you can run npm run format:fix to address). In the future I'd suggest forking from a repository that can allow maintainer edits. It's challenging to coordinate rebase and merge asynchronously before the branch becomes out of date again.

rzyns added 2 commits November 2, 2023 10:51
…de path

msal-node-extensions: `Environment.getUserHomeDirOnUnix()` was never called,
because `!this.isWindowsPlatform` would always be false, since
`isWindowsPlatform()` is a static method and not a property getter (or property)

Signed-off-by: Janusz Dziurzynski <[email protected]>
@rzyns rzyns force-pushed the feature/fix-environment-getUserRootDirectory branch from 9c5db2a to 9fbad50 Compare November 2, 2023 14:52
@rzyns
Copy link
Contributor Author

rzyns commented Nov 2, 2023

@tnorling: I just amended the original commit since the formatting issue was just a missing semicolon, and I've rebased on top of the dev branch

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2023

Codecov Report

Merging #6581 (9fbad50) into dev (81d34b4) will decrease coverage by 15.50%.
Report is 374 commits behind head on dev.
The diff coverage is 62.59%.

Flag Coverage Δ
msal-angular ?
msal-browser ?
msal-common ?
msal-core ?
msal-node ?
msal-node-extensions 69.16% <62.08%> (-6.48%) ⬇️
msal-react ?
node-token-validation ?
Files Coverage Δ
extensions/msal-node-extensions/src/Dpapi.ts 100.00% <100.00%> (ø)
.../msal-node-extensions/src/error/NativeAuthError.ts 100.00% <100.00%> (ø)
extensions/msal-node-extensions/src/index.ts 100.00% <100.00%> (ø)
...nsions/msal-node-extensions/src/packageMetadata.ts 100.00% <100.00%> (+100.00%) ⬆️
...-extensions/src/persistence/DataProtectionScope.ts 100.00% <100.00%> (ø)
...nsions/msal-node-extensions/src/utils/Constants.ts 100.00% <100.00%> (ø)
...sions/msal-node-extensions/src/utils/TypeGuards.ts 100.00% <100.00%> (ø)
...msal-node-extensions/src/error/PersistenceError.ts 62.50% <66.66%> (-11.58%) ⬇️
...node-extensions/src/persistence/BasePersistence.ts 64.28% <50.00%> (-2.39%) ⬇️
...e-extensions/src/persistence/PersistenceCreator.ts 91.42% <92.85%> (+0.25%) ⬆️
... and 8 more

... and 215 files with indirect coverage changes

@tnorling tnorling merged commit 25472ea into AzureAD:dev Nov 2, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensions Related to extensions for the base libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants