feat: add per-token delete controls to API token management page#1205
feat: add per-token delete controls to API token management page#1205mitre88 wants to merge 1 commit intoRunestoneInteractive:mainfrom
Conversation
- Adds DELETE /delete_token/{token_id} endpoint for single-token deletion.
- Displays each stored token in a table with provider, masked token,
last used date, and a Delete button.
- Keeps existing bulk delete functionality intact.
Closes RunestoneInteractive#1135
There was a problem hiding this comment.
Pull request overview
Adds per-token delete functionality to the instructor API token management page, enabling instructors to remove individual stored tokens (in addition to the existing bulk delete) and view token metadata in a table.
Changes:
- Render a “Current Tokens” table with per-token Delete actions in the instructor token management template.
- Add a new
DELETE /delete_token/{token_id}instructor API endpoint and pass token records into the template context.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| components/rsptx/templates/assignment/instructor/add_token.html | Adds token table UI and a deleteToken() client-side handler that calls the new delete endpoint. |
| bases/rsptx/assignment_server_api/routers/instructor.py | Adds per-token delete route and includes tokens in the add-token page template context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| deleted_count = await delete_api_token(course_id=course.id, token_id=token_id) | ||
|
|
||
| if deleted_count == 0: | ||
| return make_json_response( | ||
| status=status.HTTP_404_NOT_FOUND, | ||
| detail={ | ||
| "status": "error", | ||
| "message": "Token not found", | ||
| }, | ||
| ) | ||
|
|
||
| return make_json_response( | ||
| status=status.HTTP_200_OK, | ||
| detail={ | ||
| "status": "success", | ||
| "message": "Token deleted successfully", | ||
| "deleted_count": deleted_count, | ||
| }, | ||
| ) | ||
| except Exception as e: | ||
| rslogger.error(f"Error deleting API token {token_id} for course {course.id}: {e}") | ||
| return make_json_response( | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| detail=f"Error deleting token: {str(e)}", | ||
| ) |
There was a problem hiding this comment.
delete_single_token mixes response shapes: success/404 return detail as an object with message, but the exception path returns detail as a plain string. This inconsistency makes client handling brittle (and currently the UI assumes a consistent shape). Return a consistent JSON object for all outcomes (e.g., always {status, message, ...} under detail).
| return make_json_response( | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| detail=f"Error deleting token: {str(e)}", | ||
| ) |
There was a problem hiding this comment.
There’s no blank line between the end of delete_single_token and the next route decorator @router.delete("/delete_tokens"). This breaks the spacing pattern used elsewhere in this module (e.g., a blank line separates route functions around instructor.py:1340-1348 and 1444-1448) and makes the route boundary easy to miss. Add a blank line before the next decorator for readability/consistency.
| ) | |
| ) |
| const data = await response.json(); | ||
|
|
||
| if (response.ok) { | ||
| showAlert(data.detail.message, 'success'); | ||
| // Remove the row from the table without reloading | ||
| const row = document.querySelector(`tr[data-token-id="${tokenId}"]`); | ||
| if (row) row.remove(); | ||
| // Reload the page after a short delay to ensure counts are updated | ||
| setTimeout(() => { | ||
| window.location.reload(); | ||
| }, 1500); | ||
| } else { | ||
| throw new Error(data.detail || 'Failed to delete token'); | ||
| } |
There was a problem hiding this comment.
In deleteToken, the non-OK path throws new Error(data.detail || ...), but this endpoint can return detail as an object (e.g., 404 returns {status,message}), which will surface to users as [object Object]. Update the error handling to extract a message consistently (e.g., data.detail.message when detail is an object), or standardize the API response shape so the frontend can always read detail.message.
| try { | ||
| const response = await fetch(`/assignment/instructor/delete_token/${tokenId}`, { | ||
| method: 'DELETE', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| } | ||
| }); | ||
|
|
||
| const data = await response.json(); | ||
|
|
There was a problem hiding this comment.
deleteToken unconditionally calls await response.json(). If the session expires or the server returns a non-JSON response (e.g., HTML redirect/login), this will throw a JSON parse error and show a confusing message to the user. Consider checking response.headers.get('content-type') before parsing, and fall back to await response.text() (or a generic auth error) when the response isn't JSON.
|
Thanks for the PR, please have a look at Copilots comments above. I have a big concern about including the actual tokens on the page. This would make it way to easy for someone to view the source and steal the tokens. It would be better to just include the index of the token from the DB, and maybe the first few characters of the token for help in selecting. |
Closes #1135