fix(auth): add support for OAuth authentication in parallel to JWT #691
Conversation
kritikash18
force-pushed
the
issues/oauth-authentication
branch
2 times, most recently
from
f19ef6b to
97bb566
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #691 +/- ##
=======================================
Coverage 99.67% 99.67%
=======================================
Files 64 64
Lines 1542 1551 +9
Branches 214 218 +4
=======================================
+ Hits 1537 1546 +9
Misses 5 5 ☔ View full report in Codecov by Sentry. |
kritikash18
force-pushed
the
issues/oauth-authentication
branch
from
97bb566 to
1c71a0d
Compare
kritikash18
force-pushed
the
issues/oauth-authentication
branch
from
1c71a0d to
fa73f5a
Compare
anastasiapintilie
left a comment
anastasiapintilie
left a comment
There was a problem hiding this comment.
did you manage to test the changes?
|
|
||
| console.log(chalk.dim(' - plugin-cloudmanager list-programs ..')) | ||
|
|
||
| // let result |
There was a problem hiding this comment.
is this commented code needed?
There was a problem hiding this comment.
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
| } | ||
| } else { | ||
| const metaScopes = config.meta_scopes | ||
| if (!metaScopes.includes || !metaScopes.includes(requiredMetaScopeForJWTIntegration)) { |
There was a problem hiding this comment.
why do we need to do !metaScopes.includes?
won't metaScopes.includes always be true?
There was a problem hiding this comment.
yeah, I think so. It was already there, so I didn't remove it in case there was a situation which needed this
There was a problem hiding this comment.
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, requiredMetaScope] }) | ||
| if (config.oauth_enabled) { | ||
| const oauthScopes = config.scopes | ||
| if (!oauthScopes.includes || !requiredScopesForOAuthIntegration.every(scope => oauthScopes.includes(scope))) { |
There was a problem hiding this comment.
same question: why do we need the check !oauthScopes.includes?
kritikash18
force-pushed
the
issues/oauth-authentication
branch
from
fa73f5a to
3a401af
Compare
Description
Related Issue
Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: