fix(education): secure collection publication boundaries
This commit is contained in:
@@ -59,9 +59,14 @@ public interface QuestionCollectionMapper extends BaseMapperX<QuestionCollection
|
||||
.eq(QuestionCollectionDO::getAuthoringVersion, expectedVersion)
|
||||
.set(QuestionCollectionDO::getAuthoringVersion, expectedVersion + 1));
|
||||
}
|
||||
@Select("SELECT education_claim_question_collection_membership(current_schema(), #{tenantId}, #{id}, #{expectedVersion}, #{count})")
|
||||
int advanceMembershipCas(@Param("tenantId") Long tenantId, @Param("id") Long id,
|
||||
@Param("expectedVersion") int expectedVersion, @Param("count") int count);
|
||||
default int advanceMembershipCas(Long tenantId, Long id, int expectedVersion, int count) {
|
||||
return update(null, new LambdaUpdateWrapper<QuestionCollectionDO>().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<QuestionCollectionDO>().eq(QuestionCollectionDO::getId, id)
|
||||
|
||||
@@ -124,13 +124,16 @@ public interface QuestionMapper extends BaseMapperX<QuestionDO> {
|
||||
long countTenantOwnedPublishedByIds(@Param("tenantId") Long tenantId, @Param("ids") List<Long> 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));
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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) {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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<Long> 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);
|
||||
|
||||
@@ -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"))
|
||||
|
||||
Reference in New Issue
Block a user