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

Add check for empty object string in claims request parameter #3579

Merged
merged 7 commits into from
May 10, 2021

Conversation

pkanher617
Copy link
Contributor

This PR checks whether the object passed to the claims parameter is not an empty object. This prevents unnecessary cache skips when the empty object is provided, as we are only required to skip if additional claims are requested.

@github-actions github-actions bot added the msal-common Related to msal-common package label May 4, 2021
@@ -77,8 +77,9 @@ describe("AuthToken.ts Class Unit Tests", () => {
it("Throws error if rawIdToken is null or empty", () => {
expect(() => new AuthToken("", cryptoInterface)).to.throw(ClientAuthErrorMessage.nullOrEmptyToken.desc);
expect(() => new AuthToken("", cryptoInterface)).to.throw(ClientAuthError);

// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

exception to function params being non-nullable?

Copy link
Member

Choose a reason for hiding this comment

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

Can we just add an exception to tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is more to test vanilla JS scenarios. Typescript will catch them regardless. We are making some changes to our tests, one of them may be to test vanilla JS and typescript separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we just add an exception to tests?

Not sure what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

Allow null in function params for test files in lint rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think this is lint, this is typescript. We are doing what you are saying by adding ts-ignore to the file.

@coveralls
Copy link

coveralls commented May 4, 2021

Coverage Status

Coverage increased (+0.1%) to 83.91% when pulling 5b4fb88 on claims-empty-object-check into 73ec6b5 on dev.

@jasonnutter jasonnutter added this to the @azure/msal-common@4.2.2 milestone May 10, 2021
@jasonnutter jasonnutter merged commit c79505c into dev May 10, 2021
@jasonnutter jasonnutter deleted the claims-empty-object-check branch May 10, 2021 22:50
@ghost
Copy link

ghost commented May 12, 2021

🎉@azure/msal-common@v4.3.0 has been released which incorporates this pull request.:tada:

We recommend upgrading to the latest version of @azure/msal-browser or @azure/msal-node to take advantage of this change.

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-common Related to msal-common package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants