Remove redundant controller internal methods and classes from internals #6413

konstantin-msft · 2023-08-30T15:07:15Z

Remove redundant controller internal methods.
Remove redundant classes from internals.ts.
Add MemoryStorage and BrowserStorage to internals.ts.
Update tests.

Note
Will push a follow-up 1p PR right after to reconcile changes.

…tion-library-for-js into wrap-with-telem-logging

…microsoft-authentication-library-for-js into wrap-with-telem-logging

- Remove redundant classes from internals.ts. - Add `MemoryStorage` and `BrowserStorage` to internals.ts. - Update tests.

codecov-commenter · 2023-08-30T15:18:38Z

Codecov Report

Merging #6413 (c2649ef) into dev (81d34b4) will increase coverage by 1.91%.
Report is 283 commits behind head on dev.
The diff coverage is n/a.

Flag	Coverage Δ
msal-angular	`96.73% <ø> (+0.22%)`	⬆️
msal-browser	`85.79% <ø> (-0.68%)`	⬇️
msal-common	`?`
msal-core	`?`
msal-node	`?`
msal-node-extensions	`?`
msal-react	`94.24% <ø> (-0.45%)`	⬇️
node-token-validation	`?`

Files Changed	Coverage Δ
lib/msal-angular/src/constants.ts	`100.00% <ø> (ø)`
lib/msal-angular/src/msal.broadcast.service.ts	`100.00% <ø> (ø)`
lib/msal-angular/src/msal.guard.ts	`90.78% <ø> (+0.64%)`	⬆️
lib/msal-angular/src/msal.interceptor.ts	`100.00% <ø> (ø)`
lib/msal-angular/src/msal.module.ts	`100.00% <ø> (ø)`
lib/msal-angular/src/msal.navigation.client.ts	`93.33% <ø> (+0.47%)`	⬆️
lib/msal-angular/src/msal.redirect.component.ts	`100.00% <ø> (ø)`
lib/msal-angular/src/msal.service.ts	`100.00% <ø> (ø)`
lib/msal-angular/src/packageMetadata.ts	`100.00% <ø> (ø)`
...b/msal-browser/src/app/IPublicClientApplication.ts	`41.17% <ø> (-2.58%)`	⬇️
... and 45 more

... and 176 files with indirect coverage changes

konstantin-msft · 2023-08-30T15:21:58Z

lib/msal-browser/src/controllers/StandardController.ts

-     */
-    public getBrowserCrypto(): ICrypto {
-        return this.browserCrypto;
+    public setInteractionInProgress(progress: boolean): void {


Public access for 1p.

1P shouldn't need to set this, if it does we should see what we need to do to make it not needed

I would like that in a separate PR though. @konstantin-msft please correct me but my assumption is that this is reflecting current 1P status?

No, but this is adding new functionality to 3P. We shouldn't add things we don't need and since this is public we won't be able to remove it later

Removed the API after brief discussion.

tnorling · 2023-08-30T16:06:02Z

lib/msal-browser/src/controllers/IController.ts

-
-    /** @internal */
-    getRedirectResponse(): Map<string, Promise<AuthenticationResult | null>>;
-
    /** @internal */
    preflightBrowserEnvironmentCheck(


What about the rest of these internal functions? Future PR?

I think this is only for the current changes already made in 1P?

The rest are still used in 1P.

I just want to confirm removing the rest of these is still on our radar, just isn't ready yet?

tnorling · 2023-08-30T16:08:16Z

lib/msal-browser/src/internals.ts


 // Clients
-export { StandardInteractionClient } from "./interaction_client/StandardInteractionClient";
-export { RedirectClient } from "./interaction_client/RedirectClient";
-export { PopupClient } from "./interaction_client/PopupClient";
 export { SilentIframeClient } from "./interaction_client/SilentIframeClient";


Are the rest of these exports still being used in 1P?

Yes, other exports are still used being used in 1P.

sameerag · 2023-08-30T16:17:32Z

lib/msal-browser/src/controllers/StandardController.ts

-     */
-    public getBrowserCrypto(): ICrypto {
-        return this.browserCrypto;
+    public setInteractionInProgress(progress: boolean): void {


I would like that in a separate PR though. @konstantin-msft please correct me but my assumption is that this is reflecting current 1P status?

tnorling

Please remove new API

tnorling · 2023-08-30T16:25:15Z

lib/msal-browser/src/internals.ts

-// Cache entities
-export { CacheRecord } from "@azure/msal-common";
+// Storage
+export { MemoryStorage } from "./cache/MemoryStorage";


Why are we adding new things to internals? Goal is to remove internals entirely

We need that for temporary broker storage in 1p.

Then let's export this in the main exports

konstantin-msft · 2023-08-30T16:38:51Z

Please remove new API

New API will be used in 1P. Instead of exposing BrowserStorage that is solely used for managing the interaction status we can create an API to so directly.

tnorling · 2023-08-30T16:42:48Z

Please remove new API

New API will be used in 1P. Instead of exposing BrowserStorage that is solely used for managing the interaction status we can create an API to so directly.

My question is - why do we need to manage interaction status in 1P at all? Those types of details should be managed solely by 3P and if 1P is taking a requirement on these details something is wrong with the design

- Make `BrowserStorage` and `MemoryStorage` external.

konstantin-msft · 2023-08-30T17:28:17Z

Removed the API.

tnorling · 2023-08-30T18:03:31Z

lib/msal-browser/src/controllers/StandardController.ts

@@ -524,7 +524,7 @@ export class StandardController implements IController {
                        );
                        return redirectClient.acquireToken(request);
                    }
-                    this.browserStorage.setInteractionInProgress(false);
+                    this.getBrowserStorage().setInteractionInProgress(false);


Reason for this change?

Bumps [axios](https://github.com/axios/axios) from 0.21.4 to 1.7.2. <details> <summary>Release notes</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/releases">axios's releases</a>.</em></p> <blockquote> <h2>Release v1.7.2</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>fetch:</strong> enhance fetch API detection; (<a href="https://redirect.github.com/axios/axios/issues/6413">#6413</a>) (<a href="https://github.com/axios/axios/commit/4f79aef81b7c4644328365bfc33acf0a9ef595bc">4f79aef</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+3/-3 ([#6413](axios/axios#6413) )">Dmitriy Mozgovoy</a></li> </ul> <h2>Release v1.7.1</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>fetch:</strong> fixed ReferenceError issue when TextEncoder is not available in the environment; (<a href="https://redirect.github.com/axios/axios/issues/6410">#6410</a>) (<a href="https://github.com/axios/axios/commit/733f15fe5bd2d67e1fadaee82e7913b70d45dc5e">733f15f</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+14/-9 ([#6410](axios/axios#6410) )">Dmitriy Mozgovoy</a></li> </ul> <h2>Release v1.7.0</h2> <h2>Release notes:</h2> <h3>Features</h3> <ul> <li><strong>adapter:</strong> add fetch adapter; (<a href="https://redirect.github.com/axios/axios/issues/6371">#6371</a>) (<a href="https://github.com/axios/axios/commit/a3ff99b59d8ec2ab5dd049e68c043617a4072e42">a3ff99b</a>)</li> </ul> <h3>Bug Fixes</h3> <ul> <li><strong>core/axios:</strong> handle un-writable error stack (<a href="https://redirect.github.com/axios/axios/issues/6362">#6362</a>) (<a href="https://github.com/axios/axios/commit/81e0455b7b57fbaf2be16a73ebe0e6591cc6d8f9">81e0455</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+1015/-127 ([#6371](axios/axios#6371) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/jasonsaayman" title="+30/-14 ()">Jay</a></li> <li> <a href="https://github.com/alexandre-abrioux" title="+56/-6 ([#6362](axios/axios#6362) )">Alexandre ABRIOUX</a></li> </ul> <h2>Release v1.7.0-beta.2</h2> <h2>Release notes:</h2> <h3>Bug Fixes</h3> <ul> <li><strong>fetch:</strong> capitalize HTTP method names; (<a href="https://redirect.github.com/axios/axios/issues/6395">#6395</a>) (<a href="https://github.com/axios/axios/commit/ad3174a3515c3c2573f4bcb94818d582826f3914">ad3174a</a>)</li> <li><strong>fetch:</strong> fix & optimize progress capturing for cases when the request data has a nullish value or zero data length (<a href="https://redirect.github.com/axios/axios/issues/6400">#6400</a>) (<a href="https://github.com/axios/axios/commit/95a3e8e346cfd6a5548e171f2341df3235d0e26b">95a3e8e</a>)</li> <li><strong>fetch:</strong> fix headers getting from a stream response; (<a href="https://redirect.github.com/axios/axios/issues/6401">#6401</a>) (<a href="https://github.com/axios/axios/commit/870e0a76f60d0094774a6a63fa606eec52a381af">870e0a7</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+99/-46 ([#6405](axios/axios#6405) [#6404](axios/axios#6404) [#6401](axios/axios#6401) [#6400](axios/axios#6400) [#6395](axios/axios#6395) )">Dmitriy Mozgovoy</a></li> </ul> <h2>Release v1.7.0-beta.1</h2> <h2>Release notes:</h2>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/axios/axios/blob/v1.x/CHANGELOG.md">axios's changelog</a>.</em></p> <blockquote> <h2><a href="https://github.com/axios/axios/compare/v1.7.1...v1.7.2">1.7.2</a> (2024-05-21)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>fetch:</strong> enhance fetch API detection; (<a href="https://redirect.github.com/axios/axios/issues/6413">#6413</a>) (<a href="https://github.com/axios/axios/commit/4f79aef81b7c4644328365bfc33acf0a9ef595bc">4f79aef</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+3/-3 ([#6413](axios/axios#6413) )">Dmitriy Mozgovoy</a></li> </ul> <h2><a href="https://github.com/axios/axios/compare/v1.7.0...v1.7.1">1.7.1</a> (2024-05-20)</h2> <h3>Bug Fixes</h3> <ul> <li><strong>fetch:</strong> fixed ReferenceError issue when TextEncoder is not available in the environment; (<a href="https://redirect.github.com/axios/axios/issues/6410">#6410</a>) (<a href="https://github.com/axios/axios/commit/733f15fe5bd2d67e1fadaee82e7913b70d45dc5e">733f15f</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+14/-9 ([#6410](axios/axios#6410) )">Dmitriy Mozgovoy</a></li> </ul> <h1><a href="https://github.com/axios/axios/compare/v1.7.0-beta.2...v1.7.0">1.7.0</a> (2024-05-19)</h1> <h3>Features</h3> <ul> <li><strong>adapter:</strong> add fetch adapter; (<a href="https://redirect.github.com/axios/axios/issues/6371">#6371</a>) (<a href="https://github.com/axios/axios/commit/a3ff99b59d8ec2ab5dd049e68c043617a4072e42">a3ff99b</a>)</li> </ul> <h3>Bug Fixes</h3> <ul> <li><strong>core/axios:</strong> handle un-writable error stack (<a href="https://redirect.github.com/axios/axios/issues/6362">#6362</a>) (<a href="https://github.com/axios/axios/commit/81e0455b7b57fbaf2be16a73ebe0e6591cc6d8f9">81e0455</a>)</li> </ul> <h3>Contributors to this release</h3> <ul> <li> <a href="https://github.com/DigitalBrainJS" title="+1015/-127 ([#6371](axios/axios#6371) )">Dmitriy Mozgovoy</a></li> <li> <a href="https://github.com/jasonsaayman" title="+30/-14 ()">Jay</a></li> <li> <a href="https://github.com/alexandre-abrioux" title="+56/-6 ([#6362](axios/axios#6362) )">Alexandre ABRIOUX</a></li> </ul> <h1><a href="https://github.com/axios/axios/compare/v1.7.0-beta.1...v1.7.0-beta.2">1.7.0-beta.2</a> (2024-05-19)</h1> <h3>Bug Fixes</h3> <ul> <li><strong>fetch:</strong> capitalize HTTP method names; (<a href="https://redirect.github.com/axios/axios/issues/6395">#6395</a>) (<a href="https://github.com/axios/axios/commit/ad3174a3515c3c2573f4bcb94818d582826f3914">ad3174a</a>)</li> <li><strong>fetch:</strong> fix & optimize progress capturing for cases when the request data has a nullish value or zero data length (<a href="https://redirect.github.com/axios/axios/issues/6400">#6400</a>) (<a href="https://github.com/axios/axios/commit/95a3e8e346cfd6a5548e171f2341df3235d0e26b">95a3e8e</a>)</li> <li><strong>fetch:</strong> fix headers getting from a stream response; (<a href="https://redirect.github.com/axios/axios/issues/6401">#6401</a>) (<a href="https://github.com/axios/axios/commit/870e0a76f60d0094774a6a63fa606eec52a381af">870e0a7</a>)</li> </ul> <h3>Contributors to this release</h3>  </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/axios/axios/commit/0e4f9fa29077ebee4499facea6be1492b42e8a26"><code>0e4f9fa</code></a> chore(release): v1.7.2 (<a href="https://redirect.github.com/axios/axios/issues/6414">#6414</a>)</li> <li><a href="https://github.com/axios/axios/commit/4f79aef81b7c4644328365bfc33acf0a9ef595bc"><code>4f79aef</code></a> fix(fetch): enhance fetch API detection; (<a href="https://redirect.github.com/axios/axios/issues/6413">#6413</a>)</li> <li><a href="https://github.com/axios/axios/commit/67d1373131962d1f1f5b8d91f9a2f80ed3923bc8"><code>67d1373</code></a> chore(release): v1.7.1 (<a href="https://redirect.github.com/axios/axios/issues/6411">#6411</a>)</li> <li><a href="https://github.com/axios/axios/commit/733f15fe5bd2d67e1fadaee82e7913b70d45dc5e"><code>733f15f</code></a> fix(fetch): fixed ReferenceError issue when TextEncoder is not available in t...</li> <li><a href="https://github.com/axios/axios/commit/3041c61adaaac6d2c43eba28c134e7f4d43ab012"><code>3041c61</code></a> [Release] v1.7.0 (<a href="https://redirect.github.com/axios/axios/issues/6408">#6408</a>)</li> <li><a href="https://github.com/axios/axios/commit/18b13cbaef66d8c266cf681165afe31787420100"><code>18b13cb</code></a> chore(docs): add fetch adapter docs; (<a href="https://redirect.github.com/axios/axios/issues/6407">#6407</a>)</li> <li><a href="https://github.com/axios/axios/commit/e62099bc8b640acf47fba639366bbcd3bf87f831"><code>e62099b</code></a> fix(fetch): fixed a possible memory leak in the AbortController for the strea...</li> <li><a href="https://github.com/axios/axios/commit/b49aa8e3d837c36e4728a9fa8a5e23a1162e96ec"><code>b49aa8e</code></a> chore(release): v1.7.0-beta.2 (<a href="https://redirect.github.com/axios/axios/issues/6403">#6403</a>)</li> <li><a href="https://github.com/axios/axios/commit/d57f03a77fef1eb3cd9a17e2973c4305e105a42e"><code>d57f03a</code></a> chore(ci): bump create-pull-request version to fix a bug; (<a href="https://redirect.github.com/axios/axios/issues/6405">#6405</a>)</li> <li><a href="https://github.com/axios/axios/commit/097b0d18e93d12c53b77741d6bfdc8a1fc11828b"><code>097b0d1</code></a> chore(ci): add tag resolution for npm releases based on package version; (<a href="https://redirect.github.com/axios/axios/issues/6404">#6404</a>)</li> <li>Additional commits viewable in <a href="https://github.com/axios/axios/compare/v0.21.4...v1.7.2">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=axios&package-manager=npm_and_yarn&previous-version=0.21.4&new-version=1.7.2)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/AzureAD/microsoft-authentication-library-for-js/network/alerts). </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

tnorling and others added 13 commits August 23, 2023 16:52

Add wrapper functions for telem & logging

cfae35c

rename to invoke

3644984

Merge branch 'dev' of https://github.com/AzureAD/microsoft-authentica…

a537e2e

…tion-library-for-js into wrap-with-telem-logging

instrument the rest of ssoSilent

5e30566

Change files

f11a226

Add tests

1bf3494

fix CI

999da05

move httpVerToken to token request

9b3e55e

Merge branch 'dev' into wrap-with-telem-logging

b59eb8a

formatting

7795fc6

Merge branch 'wrap-with-telem-logging' of https://github.com/AzureAD/…

78c8d8a

…microsoft-authentication-library-for-js into wrap-with-telem-logging

- Remove redundant controller internal methods.

bf5d7d7

- Remove redundant classes from internals.ts. - Add `MemoryStorage` and `BrowserStorage` to internals.ts. - Update tests.

Merge branch 'dev' into telem_wrapper

1ab896c

github-actions bot added the msal-browser Related to msal-browser package label Aug 30, 2023

konstantin-msft changed the title ~~Remove redundant controller internal methods and classes from internals.~~ Remove redundant controller internal methods and classes from internals Aug 30, 2023

konstantin-msft added 2 commits August 30, 2023 11:09

Change files

6a05665

- Add return type for controller.setInteractionInProgress().

8f9fac7

konstantin-msft marked this pull request as ready for review August 30, 2023 15:20

konstantin-msft requested review from sameerag, tnorling, hectormmg, jo-arroyo and peterzenz as code owners August 30, 2023 15:20

konstantin-msft commented Aug 30, 2023

View reviewed changes

peterzenz approved these changes Aug 30, 2023

View reviewed changes

tnorling reviewed Aug 30, 2023

View reviewed changes

sameerag approved these changes Aug 30, 2023

View reviewed changes

tnorling requested changes Aug 30, 2023

View reviewed changes

tnorling reviewed Aug 30, 2023

View reviewed changes

- Remove setInteractiveInProgress API.

c2649ef

- Make `BrowserStorage` and `MemoryStorage` external.

konstantin-msft requested a review from tnorling August 30, 2023 17:28

tnorling reviewed Aug 30, 2023

View reviewed changes

tnorling approved these changes Aug 30, 2023

View reviewed changes

konstantin-msft merged commit 2096e5a into dev Aug 30, 2023
28 of 29 checks passed

konstantin-msft deleted the telem_wrapper branch August 30, 2023 18:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove redundant controller internal methods and classes from internals #6413

Remove redundant controller internal methods and classes from internals #6413

konstantin-msft commented Aug 30, 2023 •

edited

codecov-commenter commented Aug 30, 2023 •

edited

konstantin-msft Aug 30, 2023

tnorling Aug 30, 2023

sameerag Aug 30, 2023

tnorling Aug 30, 2023 •

edited

konstantin-msft Aug 30, 2023

tnorling Aug 30, 2023

sameerag Aug 30, 2023

konstantin-msft Aug 30, 2023

tnorling Aug 30, 2023

tnorling Aug 30, 2023

konstantin-msft Aug 30, 2023

sameerag Aug 30, 2023

tnorling left a comment

tnorling Aug 30, 2023

konstantin-msft Aug 30, 2023

tnorling Aug 30, 2023

konstantin-msft commented Aug 30, 2023

tnorling commented Aug 30, 2023

konstantin-msft commented Aug 30, 2023

tnorling Aug 30, 2023

Remove redundant controller internal methods and classes from internals #6413

Remove redundant controller internal methods and classes from internals #6413

Conversation

konstantin-msft commented Aug 30, 2023 • edited

codecov-commenter commented Aug 30, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnorling Aug 30, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnorling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konstantin-msft commented Aug 30, 2023

tnorling commented Aug 30, 2023

konstantin-msft commented Aug 30, 2023

Choose a reason for hiding this comment

konstantin-msft commented Aug 30, 2023 •

edited

codecov-commenter commented Aug 30, 2023 •

edited

tnorling Aug 30, 2023 •

edited