Rohitnarula7176/token cache #1

rohitnarula7176 · 2017-03-21T19:36:19Z

No description provided.

msftclas · 2017-03-21T19:36:23Z

@rohitnarula7176,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

tushargupta51 · 2017-03-21T21:20:46Z

lib/msaljs.ts

+    }
+
+    export class Constants {
+        public static get ERROR_DESCRIPTION(): string { return "error_description"; }


Please check the naming conventions for constants in TS. I don't think we should use "_".

tushargupta51 · 2017-03-21T21:21:11Z

lib/msaljs.ts

+    export class Constants {
+        public static get ERROR_DESCRIPTION(): string { return "error_description"; }
+        public static get ID_TOKEN(): string { return "id_token"; }
+        public static get ACCESS_TOKEN(): string { return "access_token"; }


I think all of these should be private except loadframe timeout and popup width and height.

Also, see if using readonly attribute makes more sense. That way, you can define these as properties instead of methods.

static readonly AccessToken: string = "access_token"

tushargupta51 · 2017-03-21T21:23:12Z

lib/msaljs.ts

+    export class ClientApplication {
+
+        private loginInProgress: boolean;
+        User: User;


I should have checked this earlier. Guideline is to use camelcase for method and property names. https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines

tushargupta51 · 2017-03-21T21:24:28Z

lib/msaljs.ts

+        public static get UNKNOWN(): string { return "UNKNOWN"; }
+    }
+
+    export class ClientApplication {


rename this to UserAgentApplication.

tushargupta51 · 2017-03-21T21:26:59Z

lib/msaljs.ts

+        private loginInProgress: boolean;
+        User: User;
+        ClientId: string;
+        Authority: string;


This should be readonly

tushargupta51 · 2017-03-21T21:27:24Z

lib/msaljs.ts

+        Authority: string;
+        AuthenticationMode: AuthenticationModes;
+        RedirectUri: string;
+        ClockSkew: number;


This should be private, default to 5 minutes

tushargupta51 · 2017-03-21T21:28:38Z

lib/msaljs.ts

+        ActiveRenewals: Object;
+        SessionState: string;
+        CacheStorage: Storage;
+


Add ValidateAuthority boolean property to turn on/off authority validation. Default should be true but I think until build, default is false.

tushargupta51 · 2017-03-21T21:30:12Z

lib/msaljs.ts

+        User: User;
+        ClientId: string;
+        Authority: string;
+        AuthenticationMode: AuthenticationModes;


There was a suggestion to reword this to something else maybe InteractionMode.

tushargupta51 · 2017-03-21T21:31:18Z

lib/msaljs.ts

+        RedirectUri: string;
+        ClockSkew: number;
+        PostLogoutRedirectUri: string;
+        CorrelationId: string;


I am not sure where we landed on this one. Should it be here or at acquireToken level only. Check what other MSALs have landed upon.

tushargupta51 · 2017-03-21T21:32:16Z

lib/msaljs.ts

+        CorrelationId: string;
+        CacheLocation: CacheLocations;
+        CheckSessionIframe: HTMLIFrameElement;
+        RenewStates: Array<string>;


tushargupta51 · 2017-03-21T21:32:21Z

lib/msaljs.ts

+        CacheLocation: CacheLocations;
+        CheckSessionIframe: HTMLIFrameElement;
+        RenewStates: Array<string>;
+        ActiveRenewals: Object;


tushargupta51 · 2017-03-21T21:32:28Z

lib/msaljs.ts

+        CheckSessionIframe: HTMLIFrameElement;
+        RenewStates: Array<string>;
+        ActiveRenewals: Object;
+        SessionState: string;


tushargupta51 · 2017-03-21T21:32:49Z

lib/msaljs.ts

+        PostLogoutRedirectUri: string;
+        CorrelationId: string;
+        CacheLocation: CacheLocations;
+        CheckSessionIframe: HTMLIFrameElement;


What is this used for? Should this be public or private?

tushargupta51 · 2017-03-21T21:48:57Z

lib/msaljs.ts

+            this.CacheStorage = new Storage(this.CacheLocation);
+        }
+
+        Login(scopes: Array<string>): void {


camelCasing for method names.

tushargupta51 · 2017-03-22T18:19:48Z

lib/msaljs.ts

+        constructor(clientId: string) {
+            this.ClientId = clientId;
+            this.Authority = "https://login.microsoftonline.com/common";
+            this.RedirectUri = window.location.href;


We should process the redirect uri to remove query parameters and hashes, the way we do it in adal.js

tushargupta51 · 2017-03-22T18:20:59Z

lib/msaljs.ts

+        RenewStates: Array<string>;
+        ActiveRenewals: Object;
+        SessionState: string;
+        CacheStorage: Storage;


This should be private

tushargupta51 · 2017-03-22T20:15:28Z

lib/msaljs.ts

+            3. redirect user to AAD
+            */
+
+            let parameters: AuthenticationRequestParameters = new AuthenticationRequestParameters(this.Authority, this.ClientId, this.AuthenticationMode, null, ResponseTypes[ResponseTypes.id_token], this.RedirectUri);


can we please rename this to "request" or "authenticationRequest"? I named it parameters earlier, that does not look like an apt name now.

tushargupta51 · 2017-03-22T20:16:13Z

lib/msaljs.ts

+            console.log(urlNavigate);
+            this.loginInProgress = true;
+            if (this.AuthenticationMode == AuthenticationModes.PopUp) {
+                return;


does this need to be implemented?

Yes it still needs to be implemented

tushargupta51 · 2017-03-22T20:30:50Z

lib/msaljs.ts

+                window.callBackMappedToRenewStates[expectedState] = function (message: string, token: string) {
+                    self.ActiveRenewals[resource] = null;
+                    for (var i = 0; i < window.callBacksMappedToRenewStates[expectedState].length; ++i) {
+                        window.callBacksMappedToRenewStates[expectedState][i](message, token);


We should add a try catch around this call and reset the window properties even when the exception occurs.

Since we are not relying on user provided callback methods and using promises instead, we don't need to worry about the case in which their code throws an error and we stop execution.

tushargupta51 · 2017-03-22T20:41:51Z

lib/msaljs.ts

+        }
+
+        private getCachedToken(scopes: Array<string>): string {
+            let accessTokenCacheItems = this.CacheStorage.getAllAccessTokens(this.ClientId, this.Authority);


We should pass user if also in the method since we are filtering the tokens based on that afterwards.

tushargupta51 · 2017-03-22T20:42:06Z

lib/msaljs.ts

+        private getCachedToken(scopes: Array<string>): string {
+            let accessTokenCacheItems = this.CacheStorage.getAllAccessTokens(this.ClientId, this.Authority);
+            let accessTokenItems: Array<AccessTokenCacheItem> = []; // Array to store multiple accessTokens for the ssame set of scopes
+            if (accessTokenCacheItems.length > 0) {


I don't think we need this check

We need this check if acquireToken is called without logging in or app reload.

tushargupta51 · 2017-03-22T20:42:53Z

lib/msaljs.ts

+                    }
+                }
+            }
+            if (accessTokenItems.length == 0)


Can we combine the two statements (== 0) and (> 1) in one if block?

tushargupta51 · 2017-03-22T20:47:58Z

lib/msaljs.ts

+
+        private getCachedToken(scopes: Array<string>): string {
+            let accessTokenCacheItems = this.CacheStorage.getAllAccessTokens(this.ClientId, this.Authority);
+            let accessTokenItems: Array<AccessTokenCacheItem> = []; // Array to store multiple accessTokens for the ssame set of scopes


Should this be an array? Since more than 1 matching tokens is not supported, I think this can be just a variable. Starting value can be null. In line 230, we can check if the variable is not null before assigning. If so, we return null there, otherwise continue. Line 235 check can be changed to check if variable is null.

It's just defining it as an array when we only expect one value does not seem right.

We can get multiple access tokens for the same set of scopes so we need an array to store them. But in the case we get more than one access token we return null from this method.

As we discussed, lets change that to single object.

tushargupta51 · 2017-03-22T20:51:33Z

lib/msaljs.ts

+
+        AcquireToken(scopes: Array<string>, callback: Function): void;
+
+        AcquireToken(scopes: Array<string>, callback: Function, loginHint: string): void;


Please add one more override that has the below signature

AcquireToken(scopes: Array, callback: Function, loginHint: string, extraQueryParameters: string)): void;

tushargupta51 · 2017-03-22T20:52:17Z

lib/msaljs.ts

+                this.registerCallback(this.ActiveRenewals[scope], scope, callback);
+            }
+            else {
+                let parameters: AuthenticationRequestParameters = new AuthenticationRequestParameters(this.Authority, this.ClientId, this.AuthenticationMode, null, ResponseTypes[ResponseTypes.token], this.RedirectUri);


please rename this to "request" or "authenticationRequest" or "tokenRequest"

tushargupta51 · 2017-03-22T20:52:53Z

lib/msaljs.ts

+            else {
+                let parameters: AuthenticationRequestParameters = new AuthenticationRequestParameters(this.Authority, this.ClientId, this.AuthenticationMode, null, ResponseTypes[ResponseTypes.token], this.RedirectUri);
+                let expectedState = parameters.State + '|' + scope;
+                this.CacheStorage.saveItem(Constants.NONCE_IDTOKEN, parameters.Nonce);


I don't think we need to save the nonce for accessToken request.

tushargupta51 · 2017-03-22T20:54:11Z

lib/msaljs.ts

+                if (popupWindow == null) {
+                    return;
+                }
+                if (this.RedirectUri.indexOf('#') != -1)


This should be handled in the constructor.

tushargupta51 · 2017-03-22T20:55:54Z

lib/msaljs.ts

+                var frameHandle = self.addAdalFrame(frameCheck);
+                if (frameHandle.src === '' || frameHandle.src === 'about:blank') {
+                    frameHandle.src = urlNavigate;
+                    self.loadFrame(urlNavigate, frameCheck);


Should we change this recursive call behavior now?

I am not sure about what benefit we get from this check. I would want to keep this there as it has not caused any problems in Adaljs.

As we discussed, it might be useful to get rid of this now instead of taking it along in msal

Add typings field in package.json to expose msal.d.ts

[msal-node][#1] SilentFlow support in msal-common

[msal-extensions] Extensions #1

Telemetry #1: Enable telemetry in common

AT Proof-Of-Possession #1: Adding req_cnf to the token request

Broker #1: Handshake between child and parent

msal-react #1: account changes

Yaml issue templates

* Add KeyManager class * Add STK JWK to BaseAuthRequest * Add STK generation logic to common and browser * Update PublicClientApplication tests to mock out STK generation from Auth Code requests * Undo msal-node-samples changes * Move generateCnf from PopTokenGenerator to KeyManager * Refactor crypto key generation to use different key generation algorithm options for AT and RT PoP * Add missing API from Crypto Interface to msal-node * Fix linter issues * Fix merge conflicts * Refactor Cryptographic constants out of BrowserConstants and CryptoOps * Fix tests after merge * Add feature flag to make RT Binding opt-in * Add error handling to STK generation step * Refactor crypto enum names * Add error handling for crypto key generation * Put KeyManager instance in BaseClient instead of AuthCode and Refresh Clients * Fix import in BaseClient * Extend KeyManager tests * Increase test coverage * Update lib/msal-browser/src/utils/CryptoConstants.ts * Update lib/msal-common/test/client/RefreshTokenClient.spec.ts Co-authored-by: Thomas Norling <[email protected]> * Fix incorrect typing and checks for private key on getPublicKeyThumbprint * Refactor cryptographic constants to have more consistent casing * Fix CryptoOps tests around getPublicKeyThumbprint * Move refreshTokenBinding feature flag to system config * Update browser client config to move refreshTokenBinding flag to system config * Rename KeyManager to CryptoKeyManager for more specificity * Update BrowserAuthError to remove keyId from error message and avoid Pii * Update lib/msal-browser/src/config/Configuration.ts * Refactor CryptoKeyManager into PopTokenGenerator * Change files * Replace crypto key type switch in getPublicKeyThumbprint with a ternary assignment Co-authored-by: Jason Nutter <[email protected]> * Update changefiles * Extend CryptoOps logging * Update exception handling and logging in CryptoOps getPublicKeyThumbprint API * Fix failing test in CryptoOps spec Co-authored-by: Thomas Norling <[email protected]> Co-authored-by: Sameera Gajjarapu <[email protected]> Co-authored-by: Jason Nutter <[email protected]>

Making request generation more flexible in preparation for upcoming EAR protocol support.

This PR updates the BrowserAuthOptions for MSAL Browser v5, including: - Removal of references, implementations, and tests for `skipAuthorityMetadataCache` - Moves `protocolMode` to SystemOptions - Removal of documentation references to `supportsNestedAppAuth`

msftclas added the cla-not-required label Mar 21, 2017

tushargupta51 reviewed Mar 21, 2017

View reviewed changes

tushargupta51 reviewed Mar 22, 2017

View reviewed changes

rohitnarula7176 requested a review from weijjia March 28, 2017 00:10

First commit for MSAL

76eb5b3

brentschmaltz force-pushed the rohitnarula7176/tokenCache branch from 9b99c03 to 76eb5b3 Compare March 29, 2017 18:13

brentschmaltz merged commit 76eb5b3 into dev Mar 29, 2017

rohitnarula7176 deleted the rohitnarula7176/tokenCache branch April 4, 2017 01:24

denyo pushed a commit to denyo/microsoft-authentication-library-for-js that referenced this pull request Sep 15, 2017

Merge pull request AzureAD#1 from felixbillon/felixbillon-AddTypings

6ec8483

Add typings field in package.json to expose msal.d.ts

PoyaManouchehri mentioned this pull request Jun 20, 2018

acquireTokenRedirect Fails with error AADSTS700023 (invalid openid and profile scopes) #333

Closed

deregtd mentioned this pull request Jun 27, 2018

Breaking out a custom StorageProvider option + misc Cleanup #331

Closed

wjdavis5 mentioned this pull request Dec 27, 2019

AADSTS50196: The server terminated an operation because it encountered a loop while processing a request #547

Closed

5 tasks

roneesh mentioned this pull request Apr 2, 2020

loginRedirect is not working #1453

Closed

5 tasks

pkanher617 pushed a commit that referenced this pull request Jun 17, 2020

Merge pull request #1711 from AzureAD/msal-node-cache-silentFlow

189cbe7

[msal-node][#1] SilentFlow support in msal-common

sameerag mentioned this pull request Jun 24, 2020

msal-node PR tracker #1258

Closed

1 task

sameerag mentioned this pull request Jul 2, 2020

[msal-common] End-to-end common code works for msal-browser and msal-node #1492

Closed

3 tasks

sameerag added a commit that referenced this pull request Jul 13, 2020

Merge pull request #1830 from AzureAD/extensions-1

c6db556

[msal-extensions] Extensions #1

tnorling added a commit that referenced this pull request Aug 3, 2020

Merge pull request #1917 from AzureAD/msal-common-MSER-Telemetry

3e27237

Telemetry #1: Enable telemetry in common

jo-arroyo mentioned this pull request Aug 17, 2020

AT Proof-Of-Possession #2: Handle proof-of-possession tokens correctly in responses #2153

Merged

pkanher617 pushed a commit that referenced this pull request Aug 20, 2020

Merge pull request #2151 from AzureAD/pop-req-thumbprint

134b0e6

AT Proof-Of-Possession #1: Adding req_cnf to the token request

pkanher617 pushed a commit that referenced this pull request Aug 26, 2020

Merge pull request #2136 from AzureAD/broker-handshake

a6d29d4

Broker #1: Handshake between child and parent

jmckennon mentioned this pull request Sep 25, 2020

B2C support for MSAL.js 2.0 with Authorization Code Flow #2340

Closed

5 tasks

tnorling added a commit that referenced this pull request Oct 19, 2020

Merge pull request #2356 from AzureAD/msal-react-account

bda2451

msal-react #1: account changes

Crio1979 mentioned this pull request Mar 22, 2021

ES5 with Angular / Ivy breaks build with getInternalNameOfClass() called on a non-ES5 class #3277

Closed

26 tasks

tnorling added a commit that referenced this pull request Apr 5, 2021

Merge pull request #1 from tnorling/yaml_issue_templates

5f72f42

Yaml issue templates

vvolodin mentioned this pull request Mar 27, 2022

Getting stuck in 'interaction is currently in progress' after refusing to give consent #4634

Closed

Robbie-Microsoft mentioned this pull request Dec 13, 2022

Added functionality to MSAL-Node's HttpClient that supports custom http(s) agents #5472

Merged

Robbie-Microsoft mentioned this pull request Apr 8, 2024

ClientCredential and OBO acquireToken requests with claims will now skip the cache #6999

Merged

tnorling mentioned this pull request Apr 8, 2024

Skip "login_hint" opaque claim if domainHint param is set #7008

Merged

tnorling mentioned this pull request Sep 19, 2024

Hackathon: msal-common - Replaced sinon with jest #7322

Closed

tnorling added a commit that referenced this pull request Mar 12, 2025

Refactor RequestParameterBuilder (EAR Pre-Req #1) (#7621)

770cc12

Making request generation more flexible in preparation for upcoming EAR protocol support.


		AcquireToken(scopes: Array<string>, callback: Function): void;

		AcquireToken(scopes: Array<string>, callback: Function, loginHint: string): void;

Rohitnarula7176/token cache #1

Rohitnarula7176/token cache #1

Conversation

rohitnarula7176 commented Mar 21, 2017

msftclas commented Mar 21, 2017

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

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

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

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

tushargupta51 Mar 24, 2017 • edited Loading

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

tushargupta51 Mar 24, 2017 •

edited

Loading