feat: add directive to validate auth token and feature flag #122

Rudge · 2023-11-15T20:43:03Z

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?

Configure the feature flag in masterdata

curl --location 'https://tokenvalidation--b2bsuite.vtexcommercestable.com.br/api/dataentities/auth_validation_config/documents' \
--header 'VtexIdclientAutCookie: {{TOKEN}}' \
--header 'Content-Type: application/json' \
--data '{
    "id": "storefront-permissions",
    "enable": false
 }
'

Update the config

curl --location --request PATCH 'https://tokenvalidation--b2bsuite.vtexcommercestable.com.br/api/dataentities/auth_validation_config/documents/storefront-permissions' \
--header 'VtexIdclientAutCookie: {{token}}' \
--header 'Content-Type: application/json' \
--data '{
    "enable": true
 }
'

Call an operation that has the new directive.

jira: B2BTEAM-1484

vtex-io-ci-cd · 2023-11-15T20:43:05Z

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:

Patch (backwards-compatible bug fixes)
Minor (backwards-compatible functionality)
Major (incompatible API changes)

And then you just need to merge your PR when you are ready! There is no need to create a release commit/tag.

No thanks, I would rather do it manually 😞

vtex-io-docs-bot · 2023-11-15T20:43:11Z

Beep boop 🤖

I noticed you didn't make any changes at the docs/ folder

There's nothing new to document 🤔
I'll do it later 😞

In order to keep track, I'll create an issue if you decide now is not a good time

I just updated 🎉🎉

github-actions · 2023-11-15T20:43:48Z

	Messages
📖	❤️ Thanks!
📖	🎉 PR additions = `130`, PR deletions = `46`

Generated by 🚫 dangerJS against d8055a7

enzomerca

I'm approving, but if you could just add to the PR why some APIs have the new directive and why other don't, for future reference...

mairatma · 2023-11-16T12:09:25Z

node/directives/checkUserAccess.ts

+
+  if (adminUserAuthToken) {
+    try {
+      await identity.validateToken({ token: adminUserAuthToken })


Can this cause us to send duplicated checks for validating these tokens? Or are we not already doing this validation on any of these fields today?

It was implemented previously. I refactored it into a new function to reuse in another directive.

I saw that, but I mean if there are other files that are doing these checks inside field resolvers. Some of the fields where you added this new checkAccessFeatureFlag directive may already be doing these checks internally, so we'd do it twice now, duplicating the work. If the only place doing this was the function you refactored than we should be good.

mairatma · 2023-11-16T12:15:25Z

node/directives/checkUserAccess.ts

-          throw new ForbiddenError('Unauthorized Access')
-        }
-      }
+      await checkUserOrAdminTokenAccess(context, field.astNode?.name?.value)


I think I'm missing something, here, but from what I understood we now have both directives:

@auditAccess. Always checks the token and throws errors when it's not present or valid.

@checkAccessWithFeatureFlag. Does the same as the other one, but only when the feature flag is enabled.

Why have both? The auditAccess will already do the check and throw before checkAccessWithFeatureFlag ever runs right? Shouldn't we only have checkAccessWithFeatureFlag, so that we can control when the check is made? And having both will make us call vtexid/identity twice when the feature flag is enabled and the check passes, since we'll check the same thing that has been checked by the first directive.

I think I misunderstood something though, can you explain the idea of the two directives with more details?

I think you said about the @checkUserAccess instead of @auditAccess, is it?

@checkUserAccess - Validates the storeUserToken or adminToken, but is not applied for all operations.
@auditAccess - Generate metrics about the use of the operation and if it uses the token or not.
@checkAccessWithFeatureFlag - It was created for temporary use. We put in some operations with @auditAccess, without token validation, and mapped in the task. The intention is to control whether we can enable or disable token validation for these operations in production.

Ah, got it! I was confusing auditAccess with checkUserAccess, then. I get it now!

Final question then: will we be able to remove this auditAccess directive after all the missing access checks are fully launched? We won't need those metrics anymore when that happens right?

Yes, this is the intention. I almost removed it in this PR, but I let it go for the next one.

Rudge · 2023-11-16T18:47:32Z

I'm approving, but if you could just add to the PR why some APIs have the new directive and why other don't, for future reference...

I putted for all operations declared in this task and that has the @auditAccess previously. For some, it isn't necessary because they have the directive @checkUserAccess or @checkAdminAccess. Do you have an example that doesn't fit in this case?

mairatma

Some final comments before approving. They also apply for the other PR that's similar to this one.

Btw it would be nice if we could share this logic between these apps, but I don't have a good enough suggestion for now. Just throwing it for the future, we need to determine a way to share code between these B2B Suite IO apps, they have too much duplicated code already.

mairatma · 2023-11-23T16:54:20Z

node/directives/checkUserAccess.ts

+    } catch (err) {
+      logger.warn({
+        error: err,
+        message: `CheckUserAccess: Invalid admin token for ${operation}`,


My only remaining suggestion here is to remove this ${operation} text from these log messages. You already have the operation as one of the fields being logged here. It's a good practice to keep log messages static, without variables within them, using just fields instead. That helps analyze and aggregate them all, and you can use the field when you want to focus on the the differences between different operations. You'll also won't lose historic log data for these log messages, you'll just be adding a field to it.

Same for the other log messages in this file.

Yes, I know, but just a reminder that this is an IO app, so it isn't easy to add a new field to index. If everybody wants to put a new field, there is a problem.

mairatma · 2023-11-23T16:54:43Z

node/directives/checkUserAccess.ts

+        error: err,
+        message: `CheckUserAccess: Invalid admin token for ${operation}`,
+        operation,
+        token: adminUserAuthToken,


Auth tokens shouldn't be logged, that's sensitive data so it's a security guideline never to log them.

Same for the other log messages in this file.

I agree, and I will remove it. The problem was in the previous solution.

- remove token from field and operation from message

mairatma

Thanks, lgtm!

sonarqubecloud · 2023-11-30T01:53:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

0.0% Coverage
0.0% Duplication

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.
Read more here

Rudge · 2023-12-19T14:47:24Z

We deployed the version without a feature flag and it doesn’t need to deploy this one.

feat: add directive to validate auth token and feature flag

49e4711

jira: B2BTEAM-1484

Rudge requested a review from a team November 15, 2023 20:43

enzomerca approved these changes Nov 16, 2023

View reviewed changes

mairatma reviewed Nov 16, 2023

View reviewed changes

mairatma mentioned this pull request Nov 16, 2023

feat: add new directive to validate auth token with feature flag vtex-apps/b2b-organizations-graphql#138

Closed

mairatma reviewed Nov 23, 2023

View reviewed changes

fix: adjust check user logs.

d8055a7

- remove token from field and operation from message

mairatma approved these changes Nov 28, 2023

View reviewed changes

Rudge force-pushed the feature/B2BTEAM-1484-flag-enable-token-validation branch from e38ab2b to d8055a7 Compare November 30, 2023 01:51

Rudge closed this Dec 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add directive to validate auth token and feature flag #122

feat: add directive to validate auth token and feature flag #122

Rudge commented Nov 15, 2023 •

edited

Loading

vtex-io-ci-cd bot commented Nov 15, 2023 •

edited by Rudge

Loading

vtex-io-docs-bot bot commented Nov 15, 2023 •

edited by Rudge

Loading

github-actions bot commented Nov 15, 2023 •

edited

Loading

enzomerca left a comment

mairatma Nov 16, 2023

Rudge Nov 16, 2023 •

edited

Loading

mairatma Nov 16, 2023

mairatma Nov 16, 2023

Rudge Nov 16, 2023

Rudge Nov 16, 2023

mairatma Nov 16, 2023

mairatma Nov 16, 2023

Rudge Nov 21, 2023

Rudge commented Nov 16, 2023 •

edited

Loading

mairatma left a comment

mairatma Nov 23, 2023 •

edited

Loading

mairatma Nov 23, 2023

Rudge Nov 27, 2023 •

edited

Loading

mairatma Nov 23, 2023

mairatma Nov 23, 2023

Rudge Nov 27, 2023 •

edited

Loading

mairatma left a comment

sonarqubecloud bot commented Nov 30, 2023

Rudge commented Dec 19, 2023

feat: add directive to validate auth token and feature flag #122

feat: add directive to validate auth token and feature flag #122

Conversation

Rudge commented Nov 15, 2023 • edited Loading

What problem is this solving?

How to test it?

vtex-io-ci-cd bot commented Nov 15, 2023 • edited by Rudge Loading

vtex-io-docs-bot bot commented Nov 15, 2023 • edited by Rudge Loading

github-actions bot commented Nov 15, 2023 • edited Loading

enzomerca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rudge Nov 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rudge commented Nov 16, 2023 • edited Loading

mairatma left a comment

Choose a reason for hiding this comment

mairatma Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rudge Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rudge Nov 27, 2023 • edited Loading

Choose a reason for hiding this comment

mairatma left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Nov 30, 2023

Rudge commented Dec 19, 2023

Rudge commented Nov 15, 2023 •

edited

Loading

vtex-io-ci-cd bot commented Nov 15, 2023 •

edited by Rudge

Loading

vtex-io-docs-bot bot commented Nov 15, 2023 •

edited by Rudge

Loading

github-actions bot commented Nov 15, 2023 •

edited

Loading

Rudge Nov 16, 2023 •

edited

Loading

Rudge commented Nov 16, 2023 •

edited

Loading

mairatma Nov 23, 2023 •

edited

Loading

Rudge Nov 27, 2023 •

edited

Loading

Rudge Nov 27, 2023 •

edited

Loading