-
Notifications
You must be signed in to change notification settings - Fork 13
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
#75 Add back configurable access token creation plugin support #76
base: master
Are you sure you want to change the base?
Conversation
* For now only in the CLI context not in the StateAction context * Fully lazily load plugins on use * Additional plugins are "labeled" after their location from where they are required as plugins are configured as an array rather than a map
only added in NodeJS 12 .. requiring NodeJS 12
Codecov Report
@@ Coverage Diff @@
## master #76 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 426 464 +38
Branches 56 62 +6
=========================================
+ Hits 426 464 +38
Continue to review full report at Codecov.
|
@@ -13,7 +13,7 @@ governing permissions and limitations under the License. | |||
const { Ims, ACCESS_TOKEN, REFRESH_TOKEN } = require('./ims') | |||
const aioLogger = require('@adobe/aio-lib-core-logging')('@adobe/aio-lib-ims:token-helper', { provider: 'debug' }) | |||
const { getContext } = require('./context') | |||
const imsJwtPlugin = require('@adobe/aio-lib-ims-jwt') | |||
const imsJwtPlugin = '@adobe/aio-lib-ims-jwt' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you had the chance to test the ims lib in a runtime action? Otherwise we will test it, the reason we do a static require is to let webpack optimizer know it has to include the '@adobe/aio-lib-ims-jwt'
module when bundling/compiling the action code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, only tested the CLI, actually.
That is a good point regarding web pack. Can web pack be instructed to consider this dependency also in another way ?
If not we could revert this to be a static import and adapt the loadPlugin
function accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to understand better how web pack is working, but a quick and dirty solution to this issue would be :
diff --git a/src/token-helper.js b/src/token-helper.js
index c2c37ae..9b774a3 100644
--- a/src/token-helper.js
+++ b/src/token-helper.js
@@ -13,7 +13,7 @@ governing permissions and limitations under the License.
const { Ims, ACCESS_TOKEN, REFRESH_TOKEN } = require('./ims')
const aioLogger = require('@adobe/aio-lib-core-logging')('@adobe/aio-lib-ims:token-helper', { provider: 'debug' })
const { getContext } = require('./context')
-const imsJwtPlugin = '@adobe/aio-lib-ims-jwt'
+const imsJwtPlugin = require('@adobe/aio-lib-ims-jwt')
const { codes: errors } = require('./errors')
/**
@@ -80,6 +80,10 @@ async function getMergedPlugins (context) {
function loadPlugin (name, location) {
aioLogger.debug('loadPlugin(%s, %s)', name, location)
+ if (name === 'jwt') {
+ return imsJwtPlugin
+ }
+
try {
return require(location)
} catch (error) {
@fmeschbe Do you have pseudocode of how a plugin would be used here? |
@purplecabbage I am not sure, where this question goes as the usage of plugins is here in the
The main changes to existing code are (a) getting a merged list of plugins from the static configuration in the code and the dynamic configuration in the config and (b) on-demand loading the plugin as needed/required |
Updated the PR with current |
Description
Implementing the feature proposal of issue #75:
setPlugins
andgetPlugins
added to theContext
classConfigCliContext
class using theplugins
configuration property for pesistenceStateActionContext
does not overwrite these methods as configurable token creation plugins are not part of this proposal. They might be added in the future.token-helper.js
to support configurable plugins:Related Issue
#75 Add back configurable access token creation plugin support
Motivation and Context
I have use cases for different ways of creation access tokens beyond what the default plugins offer. Having configurable token creation plugins solves my needs for easy extensibility.
How Has This Been Tested?
Added test cases and tested with my own token creation plugins.
Screenshots (if appropriate):
n/a
Types of changes
Checklist: