From 08825ddb125ff51c4e4d1bbf0153b0fd82536ff4 Mon Sep 17 00:00:00 2001 From: wangziqi Date: Fri, 31 Jul 2026 06:02:48 +0800 Subject: [PATCH] fix(education): secure collection publication boundaries --- .../catalog/QuestionCollectionMapper.java | 11 +++++--- .../dal/mysql/catalog/QuestionMapper.java | 7 +++-- .../catalog/provider/JavaCatalogProvider.java | 3 +++ .../TenantQuestionLifecycleServiceImpl.java | 12 ++++++--- .../V4110__add_manual_question_collection.sql | 26 ++++++++----------- ...ionAuthoringPostgreSqlIntegrationTest.java | 8 +++++- ...ucationFlywayMigrationIntegrationTest.java | 3 ++- 7 files changed, 45 insertions(+), 25 deletions(-) diff --git a/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/dal/mysql/catalog/QuestionCollectionMapper.java b/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/dal/mysql/catalog/QuestionCollectionMapper.java index d3ebd544..8ee3c9c7 100644 --- a/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/dal/mysql/catalog/QuestionCollectionMapper.java +++ b/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/dal/mysql/catalog/QuestionCollectionMapper.java @@ -59,9 +59,14 @@ public interface QuestionCollectionMapper extends BaseMapperX().eq(QuestionCollectionDO::getId, id) + .eq(QuestionCollectionDO::getTenantId, tenantId).eq(QuestionCollectionDO::getScope, "TENANT_OWNED") + .eq(QuestionCollectionDO::getDeleted, false).eq(QuestionCollectionDO::getPublicationStatus, "DRAFT") + .eq(QuestionCollectionDO::getAuthoringVersion, expectedVersion) + .set(QuestionCollectionDO::getQuestionCount, count) + .set(QuestionCollectionDO::getAuthoringVersion, expectedVersion + 1)); + } default int updateLifecycleCas(Long tenantId, Long id, String from, String to, boolean active, int expectedVersion) { return update(null, new LambdaUpdateWrapper().eq(QuestionCollectionDO::getId, id) diff --git a/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/dal/mysql/catalog/QuestionMapper.java b/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/dal/mysql/catalog/QuestionMapper.java index d4917aea..fb8efb12 100644 --- a/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/dal/mysql/catalog/QuestionMapper.java +++ b/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/dal/mysql/catalog/QuestionMapper.java @@ -124,13 +124,16 @@ public interface QuestionMapper extends BaseMapperX { long countTenantOwnedPublishedByIds(@Param("tenantId") Long tenantId, @Param("ids") List ids); @Select(""" - SELECT count(*) FROM education_question_collection_question membership + SELECT min(collection.id) FROM education_question_collection_question membership JOIN education_question_collection collection ON collection.id = membership.collection_id WHERE membership.question_id = #{questionId} AND membership.tenant_id = #{tenantId} AND membership.scope = 'TENANT_OWNED' AND membership.deleted = false AND collection.publication_status = 'ACTIVE' AND collection.deleted = false """) - long countActiveCollectionMembership(@Param("tenantId") Long tenantId, @Param("questionId") Long questionId); + Long selectFirstActiveCollectionId(@Param("tenantId") Long tenantId, @Param("questionId") Long questionId); + + @Select("SELECT pg_advisory_xact_lock(#{collectionId})::text") + String lockCollectionAdvisory(@Param("collectionId") Long collectionId); default QuestionDO selectPublishedById(Long tenantId, Long id) { return selectOne(visible(tenantId).eq(QuestionDO::getId, id)); diff --git a/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/service/catalog/provider/JavaCatalogProvider.java b/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/service/catalog/provider/JavaCatalogProvider.java index 0f44b1a7..688c7ae6 100644 --- a/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/service/catalog/provider/JavaCatalogProvider.java +++ b/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/service/catalog/provider/JavaCatalogProvider.java @@ -188,6 +188,9 @@ public class JavaCatalogProvider implements CatalogProvider, QuestionCatalogProv public CatalogPracticeBlueprintDTO getPracticeBlueprint(String collectionId, String nodeId, String type, String difficulty) { Long cid = parseOptionalLong(collectionId); Long nid = parseOptionalLong(nodeId); if (cid == null && nid == null) throw exception(CATALOG_INVALID_IDENTIFIER, "parent"); + if (cid != null && readWithExplicitCatalogScope(() -> questionCollectionMapper.selectAvailableForStudent(tenantId(), cid)) == null) { + return null; + } PracticeBlueprintDO bp = readWithExplicitCatalogScope(() -> practiceBlueprintMapper.selectByCollectionOrNode( tenantId(), cid, nid, null, type, difficulty)); if (bp == null) return null; diff --git a/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/service/question/authoring/TenantQuestionLifecycleServiceImpl.java b/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/service/question/authoring/TenantQuestionLifecycleServiceImpl.java index 50e2c4be..27c0e501 100644 --- a/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/service/question/authoring/TenantQuestionLifecycleServiceImpl.java +++ b/yudao-module-education/src/main/java/cn/iocoder/yudao/module/education/service/question/authoring/TenantQuestionLifecycleServiceImpl.java @@ -156,9 +156,15 @@ public class TenantQuestionLifecycleServiceImpl implements TenantQuestionLifecyc } if (PUBLISHED.equals(toStatus)) { validatePublishable(question); - } else if (ARCHIVED.equals(toStatus) - && TenantUtils.executeIgnore(() -> questionMapper.countActiveCollectionMembership(tenantId, questionId)) > 0) { - throw exception(QUESTION_ACTIVE_COLLECTION_CONFLICT); + } else if (ARCHIVED.equals(toStatus)) { + Long activeCollectionId = TenantUtils.executeIgnore(() -> + questionMapper.selectFirstActiveCollectionId(tenantId, questionId)); + if (activeCollectionId != null) { + TenantUtils.executeIgnore(() -> questionMapper.lockCollectionAdvisory(activeCollectionId)); + if (TenantUtils.executeIgnore(() -> questionMapper.selectFirstActiveCollectionId(tenantId, questionId)) != null) { + throw exception(QUESTION_ACTIVE_COLLECTION_CONFLICT); + } + } } if (questionMapper.updateLifecycle(tenantId, questionId, fromStatus, toStatus, published, question.getPlacementVersion()) != 1) { diff --git a/yudao-module-education/src/main/resources/db/migration/education/V4110__add_manual_question_collection.sql b/yudao-module-education/src/main/resources/db/migration/education/V4110__add_manual_question_collection.sql index 9158dc86..fcf233f2 100644 --- a/yudao-module-education/src/main/resources/db/migration/education/V4110__add_manual_question_collection.sql +++ b/yudao-module-education/src/main/resources/db/migration/education/V4110__add_manual_question_collection.sql @@ -65,7 +65,13 @@ BEGIN NEW.question_count IS DISTINCT FROM OLD.question_count OR NEW.duration_minutes IS DISTINCT FROM OLD.duration_minutes OR NEW.access_rules IS DISTINCT FROM OLD.access_rules OR NEW.sort_order IS DISTINCT FROM OLD.sort_order OR NEW.metadata IS DISTINCT FROM OLD.metadata) THEN RAISE EXCEPTION 'active collection content is immutable' USING ERRCODE='23514'; END IF; + IF NEW.publication_status IS NOT DISTINCT FROM OLD.publication_status AND OLD.publication_status='DRAFT' + AND NEW.question_count IS DISTINCT FROM OLD.question_count THEN + EXECUTE format('INSERT INTO %I.education_question_collection_membership_token VALUES ($1,$2,$3,$4)',TG_TABLE_SCHEMA) + USING pg_current_xact_id()::text::BIGINT,NEW.tenant_id,NEW.id,NEW.authoring_version; + END IF; IF NEW.publication_status IS DISTINCT FROM OLD.publication_status THEN + PERFORM pg_advisory_xact_lock(NEW.id); IF NOT ((OLD.publication_status='DRAFT' AND NEW.publication_status='ACTIVE') OR (OLD.publication_status='ACTIVE' AND NEW.publication_status='ARCHIVED')) THEN RAISE EXCEPTION 'invalid collection lifecycle transition' USING ERRCODE='23514'; END IF; IF NEW.publication_status='ACTIVE' THEN @@ -81,18 +87,6 @@ BEGIN RETURN NEW; END $$; -CREATE FUNCTION education_claim_question_collection_membership(p_schema TEXT, p_tenant_id BIGINT, p_collection_id BIGINT, p_expected_version INTEGER, p_question_count INTEGER) RETURNS INTEGER -LANGUAGE plpgsql SECURITY DEFINER SET search_path=pg_catalog,pg_temp AS $$ -DECLARE claimed_version INTEGER; -BEGIN - EXECUTE format('UPDATE %I.education_question_collection SET question_count=$1,authoring_version=$2 WHERE id=$3 AND tenant_id=$4 AND scope=''TENANT_OWNED'' AND publication_status=''DRAFT'' AND deleted=false AND authoring_version=$5 RETURNING authoring_version',p_schema) - INTO claimed_version USING p_question_count,p_expected_version+1,p_collection_id,p_tenant_id,p_expected_version; - IF claimed_version IS NULL THEN RETURN 0; END IF; - EXECUTE format('INSERT INTO %I.education_question_collection_membership_token VALUES ($1,$2,$3,$4)',p_schema) - USING pg_current_xact_id()::text::BIGINT,p_tenant_id,p_collection_id,claimed_version; - RETURN 1; -END $$; - CREATE FUNCTION education_enforce_collection_membership_mutation() RETURNS TRIGGER LANGUAGE plpgsql SECURITY DEFINER SET search_path=pg_catalog,pg_temp AS $$ DECLARE c_status VARCHAR(16); c_version INTEGER; c_tenant BIGINT; c_scope VARCHAR(20); q_tenant BIGINT; q_scope VARCHAR(20); q_status VARCHAR(20); q_published BOOLEAN; q_deleted BOOLEAN; token_count INTEGER; @@ -118,9 +112,12 @@ LANGUAGE plpgsql SECURITY DEFINER SET search_path=pg_catalog,pg_temp AS $$ DECLARE active_collection BIGINT; BEGIN IF OLD.status='PUBLISHED' AND NEW.status='ARCHIVED' THEN - EXECUTE format('SELECT collection.id FROM %I.education_question_collection_question membership JOIN %I.education_question_collection collection ON collection.id=membership.collection_id WHERE membership.question_id=$1 AND NOT membership.deleted AND collection.publication_status=''ACTIVE'' AND NOT collection.deleted LIMIT 1 FOR SHARE OF collection',TG_TABLE_SCHEMA,TG_TABLE_SCHEMA) + EXECUTE format('SELECT min(collection.id) FROM %I.education_question_collection_question membership JOIN %I.education_question_collection collection ON collection.id=membership.collection_id WHERE membership.question_id=$1 AND NOT membership.deleted AND collection.publication_status=''ACTIVE'' AND NOT collection.deleted',TG_TABLE_SCHEMA,TG_TABLE_SCHEMA) INTO active_collection USING NEW.id; - IF active_collection IS NOT NULL THEN RAISE EXCEPTION 'question belongs to active collection' USING ERRCODE='23514'; END IF; + IF active_collection IS NOT NULL THEN + PERFORM pg_advisory_xact_lock(active_collection); + RAISE EXCEPTION 'question belongs to active collection' USING ERRCODE='23514'; + END IF; END IF; RETURN NEW; END $$; @@ -166,7 +163,6 @@ LANGUAGE plpgsql SET search_path=pg_catalog,pg_temp AS $$ DECLARE n INTEGER; BEG CREATE FUNCTION education_prevent_question_collection_audit_mutation() RETURNS TRIGGER LANGUAGE plpgsql SET search_path=pg_catalog,pg_temp AS $$ BEGIN RAISE EXCEPTION 'collection audit is append-only' USING ERRCODE='23514'; END $$; REVOKE ALL ON FUNCTION education_enforce_question_collection_authoring() FROM PUBLIC; -GRANT EXECUTE ON FUNCTION education_claim_question_collection_membership(TEXT,BIGINT,BIGINT,INTEGER,INTEGER) TO PUBLIC; REVOKE ALL ON FUNCTION education_enforce_collection_membership_mutation() FROM PUBLIC; REVOKE ALL ON FUNCTION education_block_active_collection_question_archive() FROM PUBLIC; REVOKE ALL ON FUNCTION education_validate_question_collection_audit() FROM PUBLIC; diff --git a/yudao-module-education/src/test/java/cn/iocoder/yudao/module/education/service/collection/authoring/QuestionCollectionAuthoringPostgreSqlIntegrationTest.java b/yudao-module-education/src/test/java/cn/iocoder/yudao/module/education/service/collection/authoring/QuestionCollectionAuthoringPostgreSqlIntegrationTest.java index 333e54a0..1ae9e578 100644 --- a/yudao-module-education/src/test/java/cn/iocoder/yudao/module/education/service/collection/authoring/QuestionCollectionAuthoringPostgreSqlIntegrationTest.java +++ b/yudao-module-education/src/test/java/cn/iocoder/yudao/module/education/service/collection/authoring/QuestionCollectionAuthoringPostgreSqlIntegrationTest.java @@ -15,7 +15,7 @@ import java.util.List; import static cn.iocoder.yudao.module.education.enums.ErrorCodeConstants.*; import static org.junit.jupiter.api.Assertions.*; -@Import({QuestionCollectionAuthoringServiceImpl.class, cn.iocoder.yudao.module.education.service.contentnode.authoring.ContentNodeAuthoringServiceImpl.class, cn.iocoder.yudao.module.education.service.question.authoring.TenantQuestionLifecycleServiceImpl.class, EducationProperties.class}) +@Import({QuestionCollectionAuthoringServiceImpl.class, cn.iocoder.yudao.module.education.service.contentnode.authoring.ContentNodeAuthoringServiceImpl.class, cn.iocoder.yudao.module.education.service.question.authoring.TenantQuestionLifecycleServiceImpl.class, cn.iocoder.yudao.module.education.service.catalog.provider.JavaCatalogProvider.class, EducationProperties.class}) @TestPropertySource(properties = {"yudao.education.enabled=true", "yudao.education.catalog-mode=JAVA_READ"}) class QuestionCollectionAuthoringPostgreSqlIntegrationTest extends PostgreSqlDbIntegrationTest { @Resource EducationProperties properties; @@ -29,6 +29,7 @@ class QuestionCollectionAuthoringPostgreSqlIntegrationTest extends PostgreSqlDbI @Resource ContentNodeMapper nodeMapper; @Resource cn.iocoder.yudao.module.education.service.question.authoring.TenantQuestionLifecycleService questionService; @Resource QuestionMapper questionMapper; + @Resource cn.iocoder.yudao.module.education.service.catalog.provider.JavaCatalogProvider catalogProvider; private Long activeNodeId; private List publishedQuestionIds; @@ -150,6 +151,11 @@ class QuestionCollectionAuthoringPostgreSqlIntegrationTest extends PostgreSqlDbI assertNotNull(questionMapper.selectPublishedById(10L, publishedQuestionIds.get(0))); } + @Test void unavailableCollectionDoesNotReturnBlueprint() { + Long id = service.createDraft(command(null)); + assertNull(catalogProvider.getPracticeBlueprint(String.valueOf(id), null, null, null)); + } + @Test void staleAndActiveMutationAreRejected() { Long id = service.createDraft(command(null)); service.replaceMembership(id, List.of(publishedQuestionIds.get(0)), 0); service.activate(id, 1, 7L); diff --git a/yudao-module-education/src/test/java/cn/iocoder/yudao/module/education/test/EducationFlywayMigrationIntegrationTest.java b/yudao-module-education/src/test/java/cn/iocoder/yudao/module/education/test/EducationFlywayMigrationIntegrationTest.java index 32f8dd7e..331c5b9b 100644 --- a/yudao-module-education/src/test/java/cn/iocoder/yudao/module/education/test/EducationFlywayMigrationIntegrationTest.java +++ b/yudao-module-education/src/test/java/cn/iocoder/yudao/module/education/test/EducationFlywayMigrationIntegrationTest.java @@ -709,7 +709,6 @@ class EducationFlywayMigrationIntegrationTest { execute(schema, "GRANT SELECT, INSERT, UPDATE, DELETE ON ALL TABLES IN SCHEMA " + schema + " TO " + runtimeRole); execute(schema, "GRANT USAGE, SELECT ON ALL SEQUENCES IN SCHEMA " + schema + " TO " + runtimeRole); execute(schema, "REVOKE ALL ON " + schema + ".education_question_collection_lifecycle_transition_token, " + schema + ".education_question_collection_membership_token FROM " + runtimeRole); - execute(schema, "GRANT EXECUTE ON FUNCTION " + schema + ".education_claim_question_collection_membership(TEXT,BIGINT,BIGINT,INTEGER,INTEGER) TO " + runtimeRole); try (Connection connection = DriverManager.getConnection(jdbcUrl(schema), runtimeRole, "runtime_test"); var statement = connection.createStatement()) { statement.execute(""" DO $activate$ BEGIN @@ -717,6 +716,8 @@ class EducationFlywayMigrationIntegrationTest { INSERT INTO education_question_collection_lifecycle_audit(tenant_id,collection_id,authoring_version,actor_id,from_status,to_status) VALUES(10,300,2,7,'DRAFT','ACTIVE'); END $activate$; """); + assertThatThrownBy(() -> statement.execute("SELECT education_claim_question_collection_membership(current_schema(),10,300,1,0)")) + .hasMessageContaining("does not exist"); assertThatThrownBy(() -> statement.execute("INSERT INTO education_question_collection_lifecycle_transition_token VALUES(1,10,300,2,'ACTIVE','ARCHIVED')")) .hasMessageContaining("permission denied"); assertThatThrownBy(() -> statement.execute("UPDATE education_question_collection_lifecycle_audit SET actor_id=8 WHERE collection_id=300"))