AT Proof-Of-Possession #3: Browser Implementation of token signing and key storage #2154
Conversation
pkanher617
requested review from
DarylThayil,
jasonnutter,
jmckennon,
jo-arroyo,
sameerag,
hectormmg and
tnorling
as code owners
| @@ -26,23 +34,20 @@ export class CryptoOps implements ICrypto { | |||
| private static EXTRACTABLE: boolean = true; | |||
| private static POP_HASH_LENGTH = 43; // 256 bit digest / 6 bits per char = 43 | |||
|
|
|||
| private static DB_VERSION = 1; | |||
| private static DB_NAME = "msal.db"; | |||
| private static TABLE_NAME =`${CryptoOps.DB_NAME}.keys`; | |||
There was a problem hiding this comment.
These should be in constants
There was a problem hiding this comment.
These aren't used outside the context of the crypto APIs, so I chose to keep them here.
| /** | ||
| * Storage wrapper for IndexedDB storage in browsers: https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API | ||
| */ | ||
| export class DatabaseStorage<T>{ |
There was a problem hiding this comment.
Since someone might want to use IndexedDB as a storage option in the future, should we have this be an implementation of CacheManager? I understand there's complexities around async storage but curious if others think it's worth the investment to figure it out now instead of refactoring later?
There was a problem hiding this comment.
I think async storage will be a separate discussion. For now I think I will leave this as a specific implementation in our lib.
| /** | ||
| * Storage wrapper for IndexedDB storage in browsers: https://developer.mozilla.org/en-US/docs/Web/API/IndexedDB_API | ||
| */ | ||
| export class DatabaseStorage<T>{ |
There was a problem hiding this comment.
Nit Pick Could we call this IndexDatabaseStorage to be more specific
There was a problem hiding this comment.
That is leaking implementation details, which I think we should avoid doing when naming things when possible.
| * @param key | ||
| * @param data | ||
| */ | ||
| async sign(key: CryptoKey, data: ArrayBuffer): Promise<ArrayBuffer> { |
There was a problem hiding this comment.
nit pick
I wonder if this could be more specific "signToken"
There was a problem hiding this comment.
I was trying to keep naming consistent with the browser crypto APIs.
| * @param extractable | ||
| * @param usages | ||
| */ | ||
| private async msCryptoImportKey(keyBuffer: ArrayBuffer, extractable: boolean, usages: Array<KeyUsage>): Promise<CryptoKey> { |
There was a problem hiding this comment.
Should all of the IE stuff be its own file?
There was a problem hiding this comment.
I can separate it into it's own file for readability, I thought it would be easier to have all the crypto APIs in a single file.
|
mostly looks good, I was rely on @jasonnutter for final sign off |
| * @param key | ||
| */ | ||
| async get(key: string): Promise<T> { | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Do you need to pass the generic type to the Promise?
| private _db : IDBDatabase; | ||
| private _dbName: string; | ||
| private _tableName: string; | ||
| private _version: number; |
There was a problem hiding this comment.
I think we should avoid using underscores for private variables.
| this._tableName = tableName; | ||
| this._version = version; | ||
| } | ||
|
|
There was a problem hiding this comment.
resolved offline.
This PR is a follow up to PR #2153, and includes the
msal-browserimplementation of the token signing and key storage APIs introduced in the previous code.Specifically, this PR includes the following: