[AC-2797] Bulk device approval - deny - throw if request not found

[eliykat]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/AC-2797

📔 Objective

Address the following feedback:

When you submit a request to device-approval/:orgId/approve/:reqId and the :reqId is invalid, the response’s success is false and it includes a message that the request ID is invalid:

{ "success": false, "message": "Invalid request id" }

However, when you do the same scenario with the /deny endpoint, you get a success response:

{ "success": true }

I would expect the deny endpoint to behave the same as the approval endpoint.

This occurs because:

• to approve a request, we have to fetch that request from the server to do a key exchange. If no request is returned by the server, we throw the "Invalid request id" error.
• to deny a request, we don't need to do a key exchange, so we just send the request id to the server. The server silently skips any missing device requests.

To his credit, @vincentsalucci raised this difference in code review, but I didn't want to add the api call just to make sure that the request exists.

That is still a potential solution, however I thought it might be better for the server to return a list of the device requests that were successfully denied. That way, the client can decide how to handle a partial result. However, I don't feel strongly about it.

If anyone thinks that copy/pasting the check in the approve command is a better solution, we can do that instead.

Depends on https://github.com/bitwarden/server/pull/4211.

📸 Screenshots

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 29.27%. Comparing base (1826403) to head (9b252a3).

Files	Patch %	Lines
.../src/admin-console/device-approval/deny.command.ts	0.00%	4 Missing :warning:
...c/admin-console/device-approval/approve.command.ts	0.00%	1 Missing :warning:
...-requests/organization-auth-request-api.service.ts	0.00%	1 Missing :warning:
...auth-requests/organization-auth-request.service.ts	0.00%	1 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #9763 +/- ## ========================================== - Coverage 29.28% 29.27% -0.02% ========================================== Files 2532 2532 Lines 73822 73826 +4 Branches 13781 13782 +1 ========================================== - Hits 21620 21613 -7 - Misses 50580 50591 +11 Partials 1622 1622 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[addisonbeck]

Since this deny command only handles one auth request it should hit the API endpoint for single request updates here. Right now it hits the dedicated bulk deny endpoint, which is going to be removed during post-release work anyway.

Instead of going down the currently committed route I would:

1. Rename the approvePendingRequest() method in organization-auth-request-api.service to updatePendingRequest(). Supply it with an approved bool to send to the API endpoint (currently this is just hardcoded to true). That endpoint handles approvals and denials, and will return a 404 if the auth request it is given is not found.
2. Add a denyPendingRequest() method that mirrors this approvePendingRequest method in organization-auth-request.service without the request.isApproved == true specific logic. Have it reference the updatePendingRequest() method on the api service that handles single-request updates. You could probably combine the approvePendingRequest() and denyPendingRequest() service methods into an updatePendingRequest() method if you wanted. If you want to go this direction just supply updatePendingRequest() with a boolean from your command and only do the key exchange if it's true.
3. Handle the 404 error that will be returned from the API's single-request method if the auth request is not found in whatever way satisfies the QA need.

This approach has the following advantages:

• It stop you from having to make any changes to preexisting methods (the bulk deny API method),
• It stops you from needing a server PR at all
• It better syncs up the cli and server code by making use of the single-request update endpoint for single requests, and the bulk-request update endpoint for bulk requests, instead of mixing-and matching endpoints in client operations.
• It better syncs up the responses clients give between the two operations. Single requests can be loud, letting users know when they messed up an id or something, without interrupting bulk requests if an individual request in their list is bad.

---

We could use the whole "return a list of unprocessed requests from bulk methods" approach still, but that is different from the original design decision that we made that bulk operations should be mostly silent and ignore anything that fails or isn't found. I'd suggest we make that an enhancement for post-release if we want to change it.

[eliykat]

Thanks @addisonbeck , that's a much better solution. I was getting turned around a bit with the different endpoints we have. I think we should probably delete the bulk deny endpoint in follow-up work (so that we only have single update or bulk update).

The only change I made was to keep separate approve/deny methods in the api service, because they have different interfaces and I think it's clearer.

[github-actions[bot]]

Checkmarx One – Scan Summary & Details – 11f0407e-2ba7-45d7-97d1-955c427434bc

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Angular_Improper_Type_Pipe_Usage	/bitwarden_license/bit-web/src/app/admin-console/providers/providers-layout.component.html: 50	Attack Vector