feat: Get promise token for CS autofix requests #830
Conversation
| @@ -86,7 +71,7 @@ function BrandsController(dataAccess, log, env) { | |||
| const imsOrgId = organization.getImsOrgId(); | |||
| const imsUserToken = getImsUserToken(context); | |||
| const brandClient = BrandClient.createFrom(context); | |||
| const brands = await brandClient.getBrandsForOrganization(imsOrgId, imsUserToken); | |||
| const brands = await brandClient.getBrandsForOrganization(imsOrgId, `Bearer ${imsUserToken}`); | |||
There was a problem hiding this comment.
the fact that an imsUserToken is at some point used as a bearer token in an authorization header should not be a signature requirement, please remove.
There was a problem hiding this comment.
I don't understand what your ask is here. I refactored out the getImsUserToken function to utils to re-use and made it return only the token not the whole authorization header, because going by the function name that would be more expected. But the brands client actually expects the whole authorization header, so I need to add back the Bearer string to make the code functionally equivalent.
src/controllers/suggestions.js
Outdated
| if (!context.env?.AUTOFIX_CRYPT_SECRET || !context.env?.AUTOFIX_CRYPT_SALT) { | ||
| encryptedPromiseToken = promiseToken; | ||
| } else { | ||
| const algorithm = context.env?.AUTOFIX_CRYPT_ALG || 'aes-192-cbc'; |
There was a problem hiding this comment.
why this default algorithm? it is an uncommon choice, more standard would be AES-256-GCM?
There was a problem hiding this comment.
No particular reason. I updated to use the algorithm you propose and made the key length also configurable. In the end the promise token is not considered a secret, so encrypting it is an additional, optional layer of safety which @alinarublea proposed to add. The tokens are also fairly short lived, so the algorithm used won't matter too much.
src/controllers/suggestions.js
Outdated
| context.env.AUTOFIX_CRYPT_SALT, | ||
| 24, | ||
| ); | ||
| const iv = crypto.randomBytes(16); |
There was a problem hiding this comment.
Even though a random IV is generated, it's not stored or sent with the ciphertext. Without the IV, decryption later becomes problematic. Typically, you’d prepend the IV to the ciphertext so that it can be used for decryption.
There was a problem hiding this comment.
True, added it, also refactored to have the encryption in the IMS client to have encryption/decryption closer together, see PR there adobe/spacecat-shared#667
src/controllers/suggestions.js
Outdated
| const key = await promisify(crypto.scrypt)( | ||
| context.env.AUTOFIX_CRYPT_SECRET, | ||
| context.env.AUTOFIX_CRYPT_SALT, | ||
| 24, |
There was a problem hiding this comment.
AES-256 uses a 32-byte key, providing stronger encryption, in case you plan to switch
There was a problem hiding this comment.
I made the algorithm fixed, having it configurable isn't really feasible, nor needed, moved encryption code to adobe/spacecat-shared#667
| * @returns {string} imsUserToken - The IMS User access token. | ||
| * @throws {ErrorWithStatusCode} - If the Authorization header is missing. | ||
| */ | ||
| export function getImsUserToken(context) { |
There was a problem hiding this comment.
similar code is to be found in spacecat-shared-ims-client - please re-use
There was a problem hiding this comment.
I did not find any similar code in spacecat-shared-ims-client. There is private method 'getBearerToken' somewhere in spacecat-shared-http-utils, I'd rather not refactor a completely unrelated module within this PR. Already refactored to re-use within this module, so I am not adding additional code.
|
Moved encryption related code to the IMS client for better re-use, see adobe/spacecat-shared#667 |
For sites of delivery type cloud service a promise token needs to be stored in the autofix job to be able to authenticate against the cloud service AEM instance.
This PR uses the IMS Promise client to exchange the user's token against a promise token which is stored in the sqs message payload to be used in the autofix worker later on. The promise token is not considered a secret, but may be optionally encrypted using a shared secret.