Skip to content
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: enable tenant check on AMT operations #1076

Merged
merged 3 commits into from
Sep 25, 2023
Merged

feat: enable tenant check on AMT operations #1076

merged 3 commits into from
Sep 25, 2023

Conversation

rsdmike
Copy link
Member

@rsdmike rsdmike commented Sep 20, 2023

PR Checklist

  • Unit Tests have been added for new changes
  • API tests have been updated if applicable
  • All commented code has been removed
  • If you've added a dependency, you've ensured license is compatible with Apache 2.0 and clearly outlined the added dependency.

What are you changing?

Anything the reviewer should know when reviewing this PR?

If the there are associated PRs in other repositories, please link them here (i.e. open-amt-cloud-toolkit/repo#365 )

@rsdmike
Copy link
Member Author

rsdmike commented Sep 20, 2023

@fmiu-imperosoftware can you give this branch a try?

@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.26% 🎉

Comparison is base (d29d945) 94.19% compared to head (526deeb) 94.45%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1076      +/-   ##
==========================================
+ Coverage   94.19%   94.45%   +0.26%     
==========================================
  Files          83       83              
  Lines        3514     3518       +4     
  Branches      601      603       +2     
==========================================
+ Hits         3310     3323      +13     
+ Misses        198      189       -9     
  Partials        6        6              
Files Changed Coverage Δ
src/utils/constants.ts 100.00% <ø> (ø)
src/middleware/cira.ts 100.00% <100.00%> (+56.25%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fmiu-imperosoftware
Copy link

fmiu-imperosoftware commented Sep 21, 2023

@rsdmike unfortunately I cannot deploy the toolkit to test it, but checking the tenant in the cira middleware should do the trick.
Some remarks

  • deviceResult.tenantId may throw error if deviceResult is null (someone just used the device delete api and removed the entry from db);
  • ErrorResponse(401, 'Unauthorized', 'device') is not ok, 401 in utils/constants does not have a 'device' member, only 404

Variant:
mps devices is a map of ConnectedDevice which has the tenantId (https://github.com/open-amt-cloud-toolkit/mps/blob/ciraTenantCheck/src/amt/ConnectedDevice.ts#L24). So the tenant may be checked without quering the db (req.tenantId !== device.tenantId)

@rsdmike rsdmike force-pushed the ciraTenantCheck branch 2 times, most recently from 907b29b to 49101a7 Compare September 21, 2023 16:08
@rsdmike rsdmike enabled auto-merge (rebase) September 25, 2023 15:58
@rsdmike rsdmike merged commit a4010b1 into main Sep 25, 2023
11 checks passed
@rsdmike rsdmike deleted the ciraTenantCheck branch September 25, 2023 16:02
@github-actions
Copy link

🎉 This PR is included in version 2.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants