-
-
Notifications
You must be signed in to change notification settings - Fork 254
Api v2 create object comment #914
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
base: develop
Are you sure you want to change the base?
Changes from all commits
4604cd5
559c152
da7830e
0ef9c57
16bf676
f99dbda
2b417a0
9574766
3a1c273
cd5af30
b03a688
97fdd28
1b7669c
b879318
cb81bac
6ce6f58
4bd52fd
5f48689
bf7b08d
ddc239d
21356d3
7c189c7
04ee659
e9c15b5
f68b34d
18f5a1f
7d3adf6
5b1a351
b166162
1031b31
1ffc405
8477e36
9d2aa34
bb68852
09029aa
a976c39
c96b9c6
bb9f43f
9dd165d
59fa6b4
d1d4738
0b7214b
37edc1a
f1a8576
353e339
dbaf980
d797adb
a0414aa
c751620
977590b
aa1e580
61a93b1
943a136
798036d
5e54cab
a3e5979
d416cdf
374dddb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -185,7 +185,13 @@ jobs: | |||||||||||||||||||||||
PYTHONUNBUFFERED=true python -m unittest --verbose | ||||||||||||||||||||||||
- name: Stop development server | ||||||||||||||||||||||||
run: | | ||||||||||||||||||||||||
docker compose logs app > ${{ runner.temp }}/iriswebapp_app.log | ||||||||||||||||||||||||
docker compose down | ||||||||||||||||||||||||
- name: Upload artifact | ||||||||||||||||||||||||
uses: actions/upload-artifact@v4 | ||||||||||||||||||||||||
with: | ||||||||||||||||||||||||
name: Test API iriswebapp_app logs | ||||||||||||||||||||||||
path: ${{ runner.temp }}/iriswebapp_app.log | ||||||||||||||||||||||||
Comment on lines
+191
to
+194
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Ensure artifact upload always executes and doesn’t fail job if file missing Run regardless of previous failures and avoid hard-failing when the log file isn’t present; set a short retention. - - name: Upload artifact
- uses: actions/upload-artifact@v4
- with:
- name: Test API iriswebapp_app logs
- path: ${{ runner.temp }}/iriswebapp_app.log
+ - name: Upload artifact
+ if: ${{ always() }}
+ uses: actions/upload-artifact@v4
+ with:
+ name: Test API iriswebapp_app logs
+ path: ${{ runner.temp }}/iriswebapp_app.log
+ if-no-files-found: ignore
+ retention-days: 7 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
whikernel marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||
test-database-migration: | ||||||||||||||||||||||||
name: Database migration tests | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,17 +18,21 @@ | |||||||||||||||||||||||||
|
||||||||||||||||||||||||||
from flask import Blueprint | ||||||||||||||||||||||||||
from flask import request | ||||||||||||||||||||||||||
from marshmallow.exceptions import ValidationError | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
from app.blueprints.access_controls import ac_api_requires | ||||||||||||||||||||||||||
from app.blueprints.rest.endpoints import response_api_paginated | ||||||||||||||||||||||||||
from app.blueprints.rest.endpoints import response_api_not_found | ||||||||||||||||||||||||||
from app.blueprints.rest.endpoints import response_api_created | ||||||||||||||||||||||||||
from app.blueprints.rest.endpoints import response_api_error | ||||||||||||||||||||||||||
from app.blueprints.rest.parsing import parse_pagination_parameters | ||||||||||||||||||||||||||
from app.blueprints.access_controls import ac_api_return_access_denied | ||||||||||||||||||||||||||
from app.business.comments import comments_get_filtered_by_asset | ||||||||||||||||||||||||||
from app.business.comments import comments_create_for_asset | ||||||||||||||||||||||||||
from app.business.assets import assets_get | ||||||||||||||||||||||||||
from app.business.errors import ObjectNotFoundError | ||||||||||||||||||||||||||
from app.schema.marshables import CommentSchema | ||||||||||||||||||||||||||
from app.iris_engine.access_control.utils import ac_fast_check_current_user_has_case_access | ||||||||||||||||||||||||||
from app.business.access_controls import ac_fast_check_current_user_has_case_access | ||||||||||||||||||||||||||
from app.iris_engine.access_control.iris_user import iris_current_user | ||||||||||||||||||||||||||
from app.models.authorization import CaseAccessLevel | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
@@ -37,20 +41,37 @@ class CommentsOperations: | |||||||||||||||||||||||||
def __init__(self): | ||||||||||||||||||||||||||
self._schema = CommentSchema() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
@staticmethod | ||||||||||||||||||||||||||
def _get_asset(asset_identifier, possible_case_access_levels): | ||||||||||||||||||||||||||
asset = assets_get(asset_identifier) | ||||||||||||||||||||||||||
if not ac_fast_check_current_user_has_case_access(asset.case_id, possible_case_access_levels): | ||||||||||||||||||||||||||
raise ObjectNotFoundError() | ||||||||||||||||||||||||||
return asset | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
Comment on lines
+44
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainConfirm access-check API accepts a list; otherwise pass a single threshold level. The call site passes a list of levels, but - @staticmethod
- def _get_asset(asset_identifier, possible_case_access_levels):
+ @staticmethod
+ def _get_asset(asset_identifier, required_level):
asset = assets_get(asset_identifier)
- if not ac_fast_check_current_user_has_case_access(asset.case_id, possible_case_access_levels):
+ if not ac_fast_check_current_user_has_case_access(asset.case_id, required_level):
raise ObjectNotFoundError()
return asset - asset = self._get_asset(asset_identifier, [CaseAccessLevel.read_only, CaseAccessLevel.full_access])
+ asset = self._get_asset(asset_identifier, CaseAccessLevel.read_only) - asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access])
+ asset = self._get_asset(asset_identifier, CaseAccessLevel.full_access) Also applies to: 53-53, 64-64 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== target file snippet: source/app/blueprints/rest/v2/assets_routes/comments.py ==="
sed -n '1,200p' source/app/blueprints/rest/v2/assets_routes/comments.py || true
echo
echo "=== locating definition(s) of ac_fast_check_current_user_has_case_access ==="
rg -n --hidden -S "def ac_fast_check_current_user_has_case_access\b" -g '!**/node_modules/**' -g '!**/.venv/**' || true
echo
echo "=== showing usages of ac_fast_check_current_user_has_case_access (context) ==="
rg -n --hidden -S "ac_fast_check_current_user_has_case_access\(" -C3 -g '!**/node_modules/**' -g '!**/.venv/**' || true
echo
echo "=== locating CaseAccessLevel definition/usages ==="
rg -n --hidden -S "class CaseAccessLevel\b|CaseAccessLevel\." -C3 -g '!**/node_modules/**' -g '!**/.venv/**' || true
echo
echo "=== locate assets_get definition/usages ==="
rg -n --hidden -S "def assets_get\b|assets_get\(" -C3 -g '!**/node_modules/**' -g '!**/.venv/**' || true
echo
# If a definition file for the function was found, print its surrounding function block (up to 200 lines).
for file in $(rg -n --hidden -S "def ac_fast_check_current_user_has_case_access\b" -l || true); do
echo "---- contents of $file (first 300 lines) ----"
sed -n '1,300p' "$file"
done Length of output: 5820 Fix: pass a single CaseAccessLevel to ac_fast_check_current_user_has_case_access (don't pass lists) ac_fast_check_current_user_has_case_access(cid, access_level) accepts a single level (it delegates to ac_fast_check_user_has_case_access in source/app/business/access_controls.py); current code passes lists — change _get_asset to take a single required level and update callers. Location: source/app/blueprints/rest/v2/assets_routes/comments.py (lines ~44-50; call sites at ~53 and ~64) - @staticmethod
- def _get_asset(asset_identifier, possible_case_access_levels):
+ @staticmethod
+ def _get_asset(asset_identifier, required_level):
asset = assets_get(asset_identifier)
- if not ac_fast_check_current_user_has_case_access(asset.case_id, possible_case_access_levels):
+ if not ac_fast_check_current_user_has_case_access(asset.case_id, required_level):
raise ObjectNotFoundError()
return asset - asset = self._get_asset(asset_identifier, [CaseAccessLevel.read_only, CaseAccessLevel.full_access])
+ asset = self._get_asset(asset_identifier, CaseAccessLevel.read_only) - asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access])
+ asset = self._get_asset(asset_identifier, CaseAccessLevel.full_access) 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||
def get(self, asset_identifier): | ||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||
asset = assets_get(asset_identifier) | ||||||||||||||||||||||||||
if not ac_fast_check_current_user_has_case_access(asset.case_id, | ||||||||||||||||||||||||||
[CaseAccessLevel.read_only, CaseAccessLevel.full_access]): | ||||||||||||||||||||||||||
return ac_api_return_access_denied(caseid=asset.case_id) | ||||||||||||||||||||||||||
asset = self._get_asset(asset_identifier, [CaseAccessLevel.read_only, CaseAccessLevel.full_access]) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
pagination_parameters = parse_pagination_parameters(request) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
comments = comments_get_filtered_by_asset(asset_identifier, pagination_parameters) | ||||||||||||||||||||||||||
comments = comments_get_filtered_by_asset(asset, pagination_parameters) | ||||||||||||||||||||||||||
return response_api_paginated(self._schema, comments) | ||||||||||||||||||||||||||
except ObjectNotFoundError: | ||||||||||||||||||||||||||
return response_api_not_found() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
def create(self, asset_identifier): | ||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||
asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access]) | ||||||||||||||||||||||||||
comment = self._schema.load(request.get_json()) | ||||||||||||||||||||||||||
comments_create_for_asset(iris_current_user, asset, comment) | ||||||||||||||||||||||||||
Comment on lines
+65
to
+66
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Block mass-assignment of ownership fields (“user”, “user_id”, “case_id”).
- comment = self._schema.load(data, unknown=RAISE)
+ # Prevent client-controlled ownership fields
+ comment = CommentSchema(
+ exclude=('user', 'user_id', 'case_id', 'comment_id')
+ ).load(data, unknown=RAISE) Follow-up (in schema): mark 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
result = self._schema.dump(comment) | ||||||||||||||||||||||||||
return response_api_created(result) | ||||||||||||||||||||||||||
except ValidationError as e: | ||||||||||||||||||||||||||
return response_api_error('Data error', data=e.normalized_messages()) | ||||||||||||||||||||||||||
except ObjectNotFoundError: | ||||||||||||||||||||||||||
return response_api_not_found() | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
Comment on lines
+62
to
+74
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Harden JSON parsing and fail on unknown fields.
def create(self, asset_identifier):
try:
- asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access])
- comment = self._schema.load(request.get_json())
+ asset = self._get_asset(asset_identifier, [CaseAccessLevel.full_access])
+ try:
+ data = request.get_json()
+ except BadRequest:
+ return response_api_error('Invalid JSON payload')
+ if data is None:
+ return response_api_error('Expected application/json payload')
+ comment = self._schema.load(data, unknown=RAISE)
comments_create_for_asset(iris_current_user, asset, comment)
result = self._schema.dump(comment)
return response_api_created(result)
except ValidationError as e:
return response_api_error('Data error', data=e.normalized_messages())
except ObjectNotFoundError:
return response_api_not_found() Add imports (outside this hunk): from marshmallow import RAISE
from werkzeug.exceptions import BadRequest 🤖 Prompt for AI Agents
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
assets_comments_blueprint = Blueprint('assets_comments', __name__, url_prefix='/<int:asset_identifier>/comments') | ||||||||||||||||||||||||||
comments_operations = CommentsOperations() | ||||||||||||||||||||||||||
|
@@ -60,3 +81,9 @@ def get(self, asset_identifier): | |||||||||||||||||||||||||
@ac_api_requires() | ||||||||||||||||||||||||||
def get_assets_comments(asset_identifier): | ||||||||||||||||||||||||||
return comments_operations.get(asset_identifier) | ||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
|
||||||||||||||||||||||||||
@assets_comments_blueprint.post('') | ||||||||||||||||||||||||||
@ac_api_requires() | ||||||||||||||||||||||||||
def create_assets_comment(asset_identifier): | ||||||||||||||||||||||||||
return comments_operations.create(asset_identifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Make log capture resilient and step always-run
Logs step can fail if the service isn’t named “app,” causing the stop step to error and skip teardown. Add always-run, fallback to all services, quote paths, add useful flags, and reference the same compose file used for up.
📝 Committable suggestion
🤖 Prompt for AI Agents