Fix defects found in session review of GE-Enforce plugin
Consolidated fixes from a three-dimension adversarial review. Data-loss (HIGH): the manifest entry editor stripped fields the form did not expose, because PUT /entries is a full reset-then-apply. The form now captures everything - InUseCheck processes as structured name/ExePath/timeout rows (not just names), LogFile, and the three preinstall flags as checkboxes; the dead payload-source control (never wired) is removed. New regression test proves an edit preserves ExePath/timeout/LogFile/PreEnrollment/PCTypesStrict. Update-entry crash (found by that regression test): replacing an entry's one-to-one InUseCheck (unique entryid) collided with the old row mid-flush -> IntegrityError -> 400. update_entry now frees the old InUseCheck (delete+flush) before populate re-inserts it. Export truncation (MEDIUM): export_scope_to_share used a plain truncating open, so a failed/partial write left the live on-share manifest (every PC reads it) empty. Now writes a temp file in the same dir and os.replace() atomically. Report dedup case bug (MEDIUM, confirmed by scratch test): the iscurrent demote matched hostname case-sensitively while the read path uses ilike, so a PC reporting different casing left two iscurrent rows and double-counted. Demote is now case-insensitive; regression test added. Simulator fidelity (MEDIUM): PCTypesStrict was captured but ignored by the filter mirror, so the simulator wrongly matched a collections-only strict entry to a nocollections PC via the shared Standard alias group. matches_pctype now honors PCTypesStrict (disables alias expansion); test added. Hardening: removed the dead/unscoped GEENFORCE_API_KEY env fallback (never wired into config; tokens are the only path); create/update entry return 400 on a duplicate Name instead of 500; parity now asserts scope-level Version/Site; a new test guards real-manifest field lengths against column limits (the DB-free parity harness can't see truncation); error handling added to the previously unguarded editor + reports API calls. Full suite green; naming + frontend build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -11,8 +11,9 @@ Two audiences:
|
||||
|
||||
from functools import wraps
|
||||
|
||||
from flask import Blueprint, request, current_app, Response
|
||||
from flask import Blueprint, request, Response
|
||||
from flask_jwt_extended import jwt_required
|
||||
from sqlalchemy.exc import IntegrityError
|
||||
|
||||
from shopdb.api import (
|
||||
db, success_response, error_response, ErrorCodes, require_permission,
|
||||
@@ -40,15 +41,17 @@ REPORT_SCOPE = 'geenforce.report'
|
||||
|
||||
|
||||
def _require_service_token(scope):
|
||||
"""Decorator factory: require `scope` service token OR the env bootstrap key."""
|
||||
"""Decorator factory: require a managed service token scoped for `scope`.
|
||||
|
||||
Tokens are the only client auth path (the client kit provisions a
|
||||
geenforce.fetch/report token). No env-key fallback: it was never wired into
|
||||
config and an unscoped shared key is a needless backdoor.
|
||||
"""
|
||||
def wrapper(f):
|
||||
@wraps(f)
|
||||
def decorated(*args, **kwargs):
|
||||
if service_token_authorized(scope):
|
||||
return f(*args, **kwargs)
|
||||
expected = current_app.config.get('GEENFORCE_API_KEY')
|
||||
if expected and request.headers.get('X-API-Key') == expected:
|
||||
return f(*args, **kwargs)
|
||||
return error_response(ErrorCodes.UNAUTHORIZED, 'Invalid API key',
|
||||
http_code=401)
|
||||
return decorated
|
||||
@@ -278,7 +281,13 @@ def create_entry(scopeid):
|
||||
nextorder = max([e.sortorder for e in scope.entries], default=-1) + 1
|
||||
entry = build_entry(payload, nextorder)
|
||||
scope.entries.append(entry)
|
||||
db.session.commit()
|
||||
try:
|
||||
db.session.commit()
|
||||
except IntegrityError:
|
||||
db.session.rollback()
|
||||
return error_response(ErrorCodes.VALIDATION_ERROR,
|
||||
'an entry with that Name already exists in this scope',
|
||||
http_code=400)
|
||||
return success_response(_entry_payload(entry), http_code=201)
|
||||
|
||||
|
||||
@@ -293,8 +302,20 @@ def update_entry(entryid):
|
||||
invalid = _validate_entry(payload)
|
||||
if invalid:
|
||||
return error_response(ErrorCodes.VALIDATION_ERROR, invalid, http_code=400)
|
||||
# Free the one-to-one InUseCheck (unique entryid) before populate re-inserts
|
||||
# it, so the replacement does not collide with the old row mid-flush.
|
||||
if entry.inusecheck is not None:
|
||||
db.session.delete(entry.inusecheck)
|
||||
entry.inusecheck = None
|
||||
db.session.flush()
|
||||
populate_entry(entry, payload)
|
||||
db.session.commit()
|
||||
try:
|
||||
db.session.commit()
|
||||
except IntegrityError:
|
||||
db.session.rollback()
|
||||
return error_response(ErrorCodes.VALIDATION_ERROR,
|
||||
'an entry with that Name already exists in this scope',
|
||||
http_code=400)
|
||||
return success_response(_entry_payload(entry))
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user