Authorization Code with PKCE in Single-Page Applications: Proof-of-Concept #1106
Conversation
pkanher617
requested review from
jasonnutter,
DarylThayil,
jmckennon,
sameerag and
negoe
| const useStrictHeader = `'use strict';`; | ||
| const fileHeader = `${libraryHeader}\n${useStrictHeader}`; | ||
|
|
||
| export default [ |
There was a problem hiding this comment.
looking forward to hearing / reading your thoughts on why roll up may be a good choice for the lib
| this.networkClient = new FetchClient(); | ||
|
|
||
| // Initialize crypto module | ||
| this.crypto = new BrowserCrypto(); |
There was a problem hiding this comment.
do any of these have to be set on the instance can they just be passed along ?
| @@ -0,0 +1,34 @@ | |||
| /* | |||
There was a problem hiding this comment.
I wonder if they errors need to be split by browser or non browser.
for instance a common/errors/ConfigurationError, could be used by both and then browser specific classes might life in msal-browser such as IframeError (or something)
There was a problem hiding this comment.
That's the idea, I will flesh this out more in future PRs.
| export class XhrClient implements IXhrClient { | ||
| public sendRequestAsync(url: string, requestParams: RequestInit, enableCaching?: boolean): Promise<any> { | ||
| return new Promise<string>((resolve, reject) => { | ||
| const xhr = new XMLHttpRequest(); |
There was a problem hiding this comment.
This implementation is for the implicit flow, in the auth code library we will switch to using fetch.
| * @returns TConfiguration object | ||
| */ | ||
| export function buildConfiguration({ auth, storageInterface, networkInterface, cryptoInterface }: MsalConfiguration): MsalConfiguration { | ||
| const overlayedConfig: MsalConfiguration = { |
There was a problem hiding this comment.
do we think configuration between server and browser will use the same common object?
There was a problem hiding this comment.
I think we will have different auth options for server and browser, but everything else will be the same. So I am separating that into two different config objects in PR #1109.
| "scripts": { | ||
| "clean": "shx rm -rf dist lib", | ||
| "lint": "eslint src --ext .ts", | ||
| "build:all": "npm run build:common && npm link msal-common && npm run build", |
There was a problem hiding this comment.
lerna bootstrap will take care of the linking for you.
There was a problem hiding this comment.
I will implement lerna in a future PR, just trying to get POC working for now.
| "rollup-plugin-uglify": "^6.0.3", | ||
| "shx": "^0.3.2", | ||
| "tslib": "^1.10.0", | ||
| "tslint": "^5.20.0", |
There was a problem hiding this comment.
Thought this needs to be a normal dep?
There was a problem hiding this comment.
You're correct, I will rectify this in PR #1109.
| "module": "./dist/index.es.js", | ||
| "types": "./dist/index.d.ts", | ||
| "engines": { | ||
| "node": ">=0.8.0" |
There was a problem hiding this comment.
We should change this to whatever version is run in CI
There was a problem hiding this comment.
Ok is there a recommendation for what version we should be using? In CI we are just using latest stable.
| @@ -0,0 +1,62 @@ | |||
| { | |||
| "name": "msal-browser", | |||
There was a problem hiding this comment.
We need to decide if we want this or @azure/msal-browser (which is probably better for branding/consistency)
There was a problem hiding this comment.
We should decide on this in PR #1108.
This is a Proof of Concept for the using the OAuth 2.0 Authorization Code Flow with PKCE in Single-Page Applications described in issue #1000. This PR is the first of many to continually improve the MSAL library and bring this flow to parity.
As a part of this POC, we have also separated much of the platform-specific code from the protocol code, and created separate packages. This will enable us to use this flow for other platforms in the future, including Electron, Node.js, and more.
This PR should eventually have a working version of the POC for Electron using the Auth Code implementation in the msal-common project.