Conversation
Contributor
Author
|
seems like there is a bug where revoking doesn't work. it causes an error: |
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR replaces full-page reloads with targeted DataTable refreshes for Zero Trust AT management, adds new backend endpoints to filter requests by state/type, and enables conditional Adminer deployment in local Helm scripts.
- Add
DEPLOY_ADMINERflag in deploy-helm.sh for toggling Adminer - Expose
/list/{state}/{type}API and corresponding service methods for denied/approved JIT requests - Refactor
view_ztats.htmlto reload tables via AJAX and update DataTable initializations
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ops-scripts/local/deploy-helm.sh | Introduce DEPLOY_ADMINER default and use in Helm install |
| dataplane/src/main/java/.../ZeroTrustAccessTokenService.java | Add methods for denied/approved terminal and ops requests |
| api/src/main/resources/templates/sso/ztats/view_ztats.html | Replace location.reload() with reload* functions; restructure DataTable setup |
| api/src/main/java/.../ZeroTrustATApiController.java | Add listTypedZtatRequests endpoint with state/type filtering |
Comments suppressed due to low confidence (4)
api/src/main/java/io/sentrius/sso/controllers/api/ZeroTrustATApiController.java:330
- The path parameters are declared as
{state}/{type}but client calls use/list/{type}or/list/{state}/{type}inconsistently. Consider swapping the order to/list/{type}/{state}or overloading an endpoint for default state to match client usage.
@GetMapping("/list/{state}/{type}")
api/src/main/java/io/sentrius/sso/controllers/api/ZeroTrustATApiController.java:399
- The default switch case logs an invalid type but still returns 200 OK with an empty list. It would be clearer to return a 400 Bad Request for unknown types.
default:
api/src/main/java/io/sentrius/sso/controllers/api/ZeroTrustATApiController.java:332
- This new endpoint and its state/type branching logic lack corresponding unit or integration tests. Consider adding tests for each combination of type and state, as well as invalid values.
public ResponseEntity<?> listTypedZtatRequests(@RequestHeader("Authorization") String token,
dataplane/src/main/java/io/sentrius/sso/core/services/security/ZeroTrustAccessTokenService.java:284
- [nitpick] New service methods lack Javadoc. Adding brief method descriptions will help maintainers understand each new endpoint’s purpose.
public List<ZtatDTO> getDeniedJITRequests(User operatingUser) {
Comment on lines
256
to
+340
| <script> | ||
| $(document).ready(function () { | ||
| $('#open-terminal-table, #open-ops-table, #approved-terminal-table, #approved-ops-table, #denied-terminal-table, #denied-ops-table').DataTable(); | ||
| function reloadTerminalTATs() { | ||
| $.get('/api/v1/zerotrust/accesstoken/list/terminal', function (data) { | ||
| const $table = $('#open-terminal-table').DataTable(); | ||
| $table.clear(); | ||
|
|
||
| for (let s of data) { | ||
| let actions = ''; | ||
| if (s.canApprove) { | ||
| actions += `<button id="app_btn_${s.id}" class="btn-secondary app_btn">Approve</button>`; | ||
| } | ||
| if (s.canDeny) { | ||
| actions += `<button id="den_btn_${s.id}" class="btn-secondary den_btn">Deny</button>`; | ||
| } | ||
| if (s.isCurrentUser) { | ||
| actions += `<button id="rev_btn_${s.id}" class="btn-secondary rev_btn">Revoke</button>`; | ||
| } | ||
|
|
||
| $table.row.add([ | ||
| s.command, | ||
| s.userName, | ||
| s.hostName, | ||
| actions | ||
| ]); | ||
| } | ||
|
|
||
| $table.draw(); | ||
| attachZtatButtonHandlers(); // re-bind the click events | ||
| }); | ||
|
|
||
| $.get('/api/v1/zerotrust/accesstoken/list/approved/terminal', function (data) { | ||
| const $table = $('#approved-terminal-table').DataTable(); | ||
| $table.clear(); | ||
|
|
||
| for (let s of data) { | ||
| let actions = ''; | ||
| if (s.canApprove) { | ||
| actions += `<button id="app_btn_${s.id}" class="btn-secondary app_btn">Approve</button>`; | ||
| } | ||
| if (s.canDeny) { | ||
| actions += `<button id="den_btn_${s.id}" class="btn-secondary den_btn">Deny</button>`; | ||
| } | ||
| if (s.isCurrentUser) { | ||
| actions += `<button id="rev_btn_${s.id}" class="btn-secondary rev_btn">Revoke</button>`; | ||
| } | ||
|
|
||
| $table.row.add([ | ||
| s.command, | ||
| s.userName, | ||
| s.hostName, | ||
| s.usesRemaining, | ||
| actions | ||
| ]); | ||
| } | ||
|
|
||
| $table.draw(); | ||
| attachZtatButtonHandlers(); // re-bind the click events | ||
| }); | ||
|
|
||
| $.get('/api/v1/zerotrust/accesstoken/list/denied/terminal', function (data) { | ||
| const $table = $('#denied-terminal-table').DataTable(); | ||
| $table.clear(); | ||
|
|
||
| for (let s of data) { | ||
| let actions = ''; | ||
| if (s.canApprove) { | ||
| actions += `<button id="app_btn_${s.id}" class="btn-secondary app_btn">Approve</button>`; | ||
| } | ||
| if (s.canDeny) { | ||
| actions += `<button id="den_btn_${s.id}" class="btn-secondary den_btn">Deny</button>`; | ||
| } | ||
|
|
||
| $table.row.add([ | ||
| s.command, | ||
| s.userName, | ||
| s.hostName, | ||
| actions | ||
| ]); | ||
| } | ||
|
|
||
| $table.draw(); | ||
| attachZtatButtonHandlers(); // re-bind the click events | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
[nitpick] The reload functions for terminal and ops share a lot of duplicate logic. Consider extracting common table-refresh behavior into a helper to reduce repetition.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Marc <phrocker@apache.org>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Marc <phrocker@apache.org>
Contributor
Author
|
After committing suggestions: DataTables warning: table id=approved-ops-table - Requested unknown parameter '0' for row 0, column 0. For more information about this error, please see http://datatables.net/tn/4 Occurs when the page is loaded @copilot |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.