Cmd authorization support #1423
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1423 +/- ##
==========================================
- Coverage 75.40% 75.35% -0.05%
==========================================
Files 600 603 +3
Lines 44700 44813 +113
Branches 777 787 +10
==========================================
+ Hits 33705 33769 +64
- Misses 10907 10957 +50
+ Partials 88 87 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| begin | ||
| authorize( | ||
| permission: permission, | ||
| target_name: target_name, | ||
| scope: params[:scope], | ||
| token: request.headers['HTTP_AUTHORIZATION'], |
| # This sets a global flag $openc3_authorize = true | ||
| # which is used by authorization.rb to bypass the | ||
| # role and permission checks. This is because the Cts | ||
| # is an internal microservice inside the trust zone. |
There was a problem hiding this comment.
This comment is wrong. = true means that authorization matters to the Cts, not that it doesn't matter.
| @@ -47,7 +47,7 @@ class Auth { | |||
| return { name: 'Anonymous' } | |||
| } | |||
| userroles() { | |||
| return ['ALLSCOPES__admin'] | |||
| return ['admin'] | |||
There was a problem hiding this comment.
Are you sure this shouldn't include the scope?
There was a problem hiding this comment.
Did you change the implementation of this in Enterprise to do all the filtering and such?
There was a problem hiding this comment.
Yes, the new implementation in Enterprise just returns the roles without the scope attached so this change matches the new implementation. I found that we were repeating this logic in 3 different places.
| takeAll() { | ||
| Api.post('/openc3-api/cmdauth/take-all', { | ||
| data: { | ||
| target_names: this.data.map((target) => target.name), |
There was a problem hiding this comment.
Why does this need a list of target names?
There was a problem hiding this comment.
Probably doesn't but I already had the list in the frontend. Probably more robust to ask for the list in the backend.
| releaseAll() { | ||
| Api.post('/openc3-api/cmdauth/release-all', { | ||
| data: { | ||
| target_names: this.data.map((target) => target.name), |
openc3/lib/openc3/api/cmd_api.rb
Outdated
| @@ -173,7 +177,7 @@ def send_raw(interface_name, data, scope: $openc3_scope, token: $openc3_token) | |||
| # @param target_name [String] Target name of the command | |||
| # @param command_name [String] Packet name of the command | |||
| # @return [Hash] command hash with last command buffer | |||
| def get_cmd_buffer(*args, scope: $openc3_scope, token: $openc3_token) | |||
| def get_cmd_buffer(*args, manual: false, scope: $openc3_scope, token: $openc3_token) | |||
| target_name, command_name = _extract_target_command_names('get_cmd_buffer', *args) | |||
| authorize(permission: 'cmd_info', target_name: target_name, packet_name: command_name, scope: scope, token: token) | |||
There was a problem hiding this comment.
Any harm in passing manual down to authorize always?
There was a problem hiding this comment.
We're always passing the manual flag now and I moved the logic into the Enterprise authorize method like we talked about
openc3/python/tester.py
Outdated
| @@ -0,0 +1 @@ | |||
| print('Hello World') | |||
There was a problem hiding this comment.
This file is probably accidentally here?
| require 'openc3/topics/topic' | ||
|
|
||
| module OpenC3 | ||
| class SystemTopic < Topic |
|
This is a lot to review but I'd appreciate a close look at the new comment blocks because I was describing things that I found out along the way.
Also there is probably additional testing needed and some frontend changes coming.