-
Notifications
You must be signed in to change notification settings - Fork 15
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
feat: add new directive to validate auth token with feature flag #138
Conversation
Hi! I'm VTEX IO CI/CD Bot and I'll be helping you to publish your app! 🤖 Please select which version do you want to release:
And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.
|
Beep boop 🤖 I noticed you didn't make any changes at the
In order to keep track, I'll create an issue if you decide now is not a good time
|
Kudos, SonarCloud Quality Gate passed! 0 Bugs 3.4% Coverage The version of Java (11.0.17) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
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.
Could you add a comment on this PR explaining why some operations do not have the new @checkAccessWithFeatureFlag
directive? e.g. getOrganizationByIdStorefront
has but getCostCentersByOrganizationIdStorefront
does not.
@@ -1,6 +1,7 @@ | |||
import axios from 'axios' | |||
|
|||
const ANALYTICS_URL = 'https://rc.vtex.com/api/analytics/schemaless-events' | |||
const ANALYTICS_URL = | |||
'https://analytics.vtex.com/api/analytics/schemaless-events' |
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.
why did this change? What is the difference between rc
and analytics
urls?
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 found some issues with using rc
and the documentation suggests using analytics
.
"This can be done by sending events to the following endpoint: https://analytics.vtex.com/api/analytics/schemaless-events (For applications on the VTEX Network) or to https://rc.vtex.com/api/analytics/schemaless-events (For applications outside the VTEX Network, such as Front End Applications)"
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.
Good point,
Good point. I added in |
@@ -78,26 +88,29 @@ type Query { | |||
getPaymentTerms: [PaymentTerm] | |||
@cacheControl(scope: PUBLIC, maxAge: SHORT) | |||
@auditAccess | |||
|
|||
@checkAccessWithFeatureFlag | |||
getOrganizationsByEmail(email: String): [B2BOrganization] | |||
@checkUserAccess | |||
@cacheControl(scope: PRIVATE) | |||
@auditAccess |
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.
getOrganizationsByEmail
is on https://vtex-dev.atlassian.net/browse/B2BTEAM-1285 but we did not add the @checkAccessWithFeatureFlag
for it
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.
It is not necessary because there is already a directive @checkUserAccess
. I am removing it from the task.
error: err, | ||
message: `CheckUserAccess: Invalid admin token for ${operation}`, | ||
operation, | ||
token, |
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.
This PR is just missing the changes you made on that other similar for storefront-permissions, where you removed the token
field from the logs and also removed operation
from the log messages.
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.
Leaving this approved, but don't forget the log adjustments, ok?
We deployed the version without a feature flag and it doesn’t need to deploy this one. |
What problem is this solving?
Enable the auth token validation for insecure operations. Previously, we put the directive
@auditAccess
and now we add the directive@checkAccessWithFeatureFlag
.How to test it?
Call an operation that has the new directive.
We called the
getOrganizationByIdStorefront
in the performance test and the difference between them was 100 ms for average, 90 ms for mediana, and 160 ms for P95.scenarios: (100.00%) 1 scenario, 2 max VUs, 45s max duration (incl. graceful stop):
* default: 2 looping VUs for 15s (gracefulStop: 30s)
Performance test without querying masterdata to get the feature flag:
Workspace