-
Notifications
You must be signed in to change notification settings - Fork 34
fix(auth): add support for OAuth authentication in parallel to JWT #691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,8 +43,9 @@ const exec = (cmd, args) => { | |
| /* | ||
| Used in list-programs, which is stubbed out. | ||
| Env vars need to be defined and code enabled | ||
| Test using JWT integrations; will be removed when JWT is discontinued | ||
| */ | ||
| const bootstrapAuthContext = async () => { | ||
| const bootstrapAuthContextWithJWTIntegration = async () => { | ||
| const contextObj = { | ||
| client_id: process.env.E2E_CLIENT_ID, | ||
| client_secret: process.env.E2E_CLIENT_SECRET, | ||
|
|
@@ -54,6 +55,32 @@ const bootstrapAuthContext = async () => { | |
| 'ent_cloudmgr_sdk', | ||
| ], | ||
| // private_key: Buffer.from(process.env.E2E_PRIVATE_KEY_B64, 'base64').toString(), | ||
| oauth_enabled: false, | ||
| } | ||
|
|
||
| await context.set(CONTEXT_NAME, contextObj) | ||
| } | ||
|
|
||
| /* | ||
| Used in list-programs, which is stubbed out. | ||
| Env vars need to be defined and code enabled | ||
| Test using OAuth integrations | ||
| */ | ||
| const bootstrapAuthContextWithOAuthIntegration = async () => { | ||
| const contextObj = { | ||
| client_id: process.env.OAUTH_E2E_CLIENT_ID, | ||
| client_secrets: [process.env.OAUTH_E2E_CLIENT_SECRET], | ||
| technical_account_id: process.env.OAUTH_E2E_TA_ID, | ||
| technical_account_email: process.env.OAUTH_E2E_TA_EMAIL, | ||
| ims_org_id: process.env.OAUTH_E2E_IMS_ORG_ID, | ||
| scopes: [ | ||
| 'openid', | ||
| 'AdobeID', | ||
| 'read_organizations', | ||
| 'additional_info.projectedProductContext', | ||
| 'read_pc.dma_aem_ams', | ||
| ], | ||
| oauth_enabled: true, | ||
| } | ||
|
|
||
| await context.set(CONTEXT_NAME, contextObj) | ||
|
|
@@ -76,11 +103,38 @@ test('plugin-cloudmanager help test', async () => { | |
| */ | ||
| /* | ||
| * Note: this test cannot be run by the bot, since it requires setup which the bot can't provide | ||
| * If wanting to rn the test, the evironment variables have to be set with the required authentication information | ||
| * If wanting to run the test, the environment variables have to be set with the required authentication information | ||
| * Uses JWT integration which is deprecated; will be removed when JWT is discontinued | ||
| */ | ||
|
|
||
| test('plugin-cloudmanager list-programs', async () => { | ||
| await bootstrapAuthContext() | ||
| test('plugin-cloudmanager list-programs using JWT integration', async () => { | ||
| await bootstrapAuthContextWithJWTIntegration() | ||
| const packagejson = JSON.parse(fs.readFileSync('package.json').toString()) | ||
| const name = `${packagejson.name}` | ||
| console.log(chalk.blue(`> e2e tests for ${chalk.bold(name)}`)) | ||
|
|
||
| console.log(chalk.dim(' - plugin-cloudmanager list-programs ..')) | ||
|
|
||
| // let result | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this commented code needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so the way i've seen it work and based on some past pull requests; these tests are disabled when pushing code to PR. If someone needs to test e2e, they need to uncomment all commented part and run e2e. If you see current test, it does nothing until the main code is uncommented
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. check PR: #669 |
||
| // expect(() => { result = exec('./bin/run', ['cloudmanager:list-programs', ...CONTEXT_ARGS, '--json']) }).not.toThrow() | ||
| // const parsed = JSON.parse(result.stdout) | ||
| const parsed = '{}' | ||
| expect(parsed).toSatisfy(arr => arr.length > 0) | ||
|
|
||
| console.log(chalk.green(` - done for ${chalk.bold(name)}`)) | ||
| }) | ||
|
|
||
| /* | ||
| Side condition: debug log output must not be enabled (DEBUG=* or LOG_LEVEL=debug), | ||
| or else the result in result.stdout is not valid JSON and cannot be parsed (line: JSON.parse...) | ||
| */ | ||
| /* | ||
| * Note: this test cannot be run by the bot, since it requires setup which the bot can't provide | ||
| * If wanting to run the test, the environment variables have to be set with the required authentication information | ||
| * Uses OAuth integrations | ||
| */ | ||
| test('plugin-cloudmanager list-programs using OAuth integration', async () => { | ||
| await bootstrapAuthContextWithOAuthIntegration() | ||
| const packagejson = JSON.parse(fs.readFileSync('package.json').toString()) | ||
| const name = `${packagejson.name}` | ||
| console.log(chalk.blue(`> e2e tests for ${chalk.bold(name)}`)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,9 +16,10 @@ const { isThisPlugin } = require('../../cloudmanager-hook-helpers') | |
| const { defaultImsContextName: defaultContextName } = require('../../constants') | ||
| const { codes: configurationCodes } = require('../../ConfigurationErrors') | ||
|
|
||
| const requiredKeys = ['client_id', 'client_secret', 'technical_account_id', 'meta_scopes', 'ims_org_id', 'private_key'] | ||
|
|
||
| const requiredMetaScope = 'ent_cloudmgr_sdk' | ||
| const requiredKeysForJWTIntegration = ['client_id', 'client_secret', 'technical_account_id', 'meta_scopes', 'ims_org_id', 'private_key'] | ||
| const requiredKeysForOAuthIntegration = ['client_id', 'client_secrets', 'technical_account_email', 'technical_account_id', 'scopes', 'ims_org_id'] | ||
| const requiredMetaScopeForJWTIntegration = 'ent_cloudmgr_sdk' | ||
| const requiredScopesForOAuthIntegration = ['openid', 'AdobeID', 'read_organizations', 'additional_info.projectedProductContext', 'read_pc.dma_aem_ams'] | ||
|
|
||
| function getContextName (options) { | ||
| if (options.Command.flags && options.Command.flags.imsContextName) { | ||
|
|
@@ -46,6 +47,7 @@ module.exports = function (hookOptions) { | |
| } | ||
|
|
||
| const missingKeys = [] | ||
| const requiredKeys = config.oauth_enabled ? requiredKeysForOAuthIntegration : requiredKeysForJWTIntegration | ||
|
|
||
| requiredKeys.forEach(key => { | ||
| if (!config[key]) { | ||
|
|
@@ -57,9 +59,16 @@ module.exports = function (hookOptions) { | |
| throw new configurationCodes.IMS_CONTEXT_MISSING_FIELDS({ messageValues: [configKey, missingKeys.join(', ')] }) | ||
| } | ||
|
|
||
| const metaScopes = config.meta_scopes | ||
| if (!metaScopes.includes || !metaScopes.includes(requiredMetaScope)) { | ||
| throw new configurationCodes.IMS_CONTEXT_MISSING_METASCOPE({ messageValues: [configKey, requiredMetaScope] }) | ||
| if (config.oauth_enabled) { | ||
| const oauthScopes = config.scopes | ||
| if (!oauthScopes.includes || !requiredScopesForOAuthIntegration.every(scope => oauthScopes.includes(scope))) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same question: why do we need the check |
||
| throw new configurationCodes.IMS_CONTEXT_MISSING_OAUTH_SCOPES({ messageValues: [configKey, requiredScopesForOAuthIntegration.join(', ')] }) | ||
| } | ||
| } else { | ||
| const metaScopes = config.meta_scopes | ||
| if (!metaScopes.includes || !metaScopes.includes(requiredMetaScopeForJWTIntegration)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need to do won't
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, I think so. It was already there, so I didn't remove it in case there was a situation which needed this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. probably, in case someone defines metascopes as some other data type that doesn't support .includes then it would return false |
||
| throw new configurationCodes.IMS_CONTEXT_MISSING_METASCOPE({ messageValues: [configKey, requiredMetaScopeForJWTIntegration] }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.