From 6a4c0452392817a40380bbaa44613a3d56198fe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Mon, 31 Aug 2026 15:39:54 -0600 Subject: [PATCH 1/9] fix(guideline): relax RBAC and remove empty-geo guard on update MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove isAuthor/isModerator/isAdmin check from update.js — any authenticated user can now update a guideline (tokenAuth enforces auth) - Remove empty-geo guard from update.js — PATCH with all associations cleared to [] is now valid; replaceCollection handles empty arrays - Remove isAuthor/isModerator/isAdmin check from rollback.js — same open-to-all-users policy as update - Remove unused RightService imports from update.js and rollback.js - Update update.test.js: drop stale 403 test, add [BUG-1]/[BUG-2] fix-verification tests, add 401 preservation baseline - Update swaggerV1.yaml: correct PATCH and rollback descriptions --- api/controllers/v1/guideline/rollback.js | 16 ------- api/controllers/v1/guideline/update.js | 21 --------- assets/swaggerV1.yaml | 4 +- .../4_routes/Guidelines/update.test.js | 46 +++++++++++++++---- 4 files changed, 39 insertions(+), 48 deletions(-) diff --git a/api/controllers/v1/guideline/rollback.js b/api/controllers/v1/guideline/rollback.js index 199eee1b2..c74de68e6 100644 --- a/api/controllers/v1/guideline/rollback.js +++ b/api/controllers/v1/guideline/rollback.js @@ -1,7 +1,6 @@ const dayjs = require('../../../utils/dayjs'); const ControllerService = require('../../../services/ControllerService'); const GuidelineService = require('../../../services/GuidelineService'); -const RightService = require('../../../services/RightService'); const { toSimpleGuideline } = require('../../../services/mapping/converters'); module.exports = async (req, res) => { @@ -34,21 +33,6 @@ module.exports = async (req, res) => { }); } - // Rollback mutates the guideline (title, description, language), so it is - // functionally an update: restrict it to the author, a moderator, or an admin. - const isAuthor = Number(rawGuideline.author) === Number(req.token.id); - const isModerator = RightService.hasGroup( - req.token.groups, - RightService.G.MODERATOR - ); - const isAdmin = RightService.hasGroup( - req.token.groups, - RightService.G.ADMINISTRATOR - ); - if (!isAuthor && !isModerator && !isAdmin) { - return res.forbidden('You are not authorized to roll back this guideline.'); - } - // Find the specific history snapshot by comparing instants rather than doing a // formatted-string equality on the timestamp column. The model's `id` is // date_reviewed, whose serialized form depends on the adapter and the server diff --git a/api/controllers/v1/guideline/update.js b/api/controllers/v1/guideline/update.js index 23766ba6d..264bcf3d6 100644 --- a/api/controllers/v1/guideline/update.js +++ b/api/controllers/v1/guideline/update.js @@ -2,10 +2,8 @@ const ControllerService = require('../../../services/ControllerService'); const GuidelineService = require('../../../services/GuidelineService'); const CommonService = require('../../../services/CommonService'); const { toSimpleGuideline } = require('../../../services/mapping/converters'); -const RightService = require('../../../services/RightService'); module.exports = async (req, res) => { - // Only the author or a moderator can update a guideline const guidelineId = req.param('id'); const rawGuideline = await TGuideline.findOne(guidelineId) .populate('countries') @@ -17,19 +15,6 @@ module.exports = async (req, res) => { }); } - const isAuthor = Number(rawGuideline.author) === Number(req.token.id); - const isModerator = RightService.hasGroup( - req.token.groups, - RightService.G.MODERATOR - ); - const isAdmin = RightService.hasGroup( - req.token.groups, - RightService.G.ADMINISTRATOR - ); - if (!isAuthor && !isModerator && !isAdmin) { - return res.forbidden('You are not authorized to update this guideline.'); - } - const newTitle = req.param('title'); const newDescription = req.param('description'); const newLanguage = req.param('language'); @@ -85,12 +70,6 @@ module.exports = async (req, res) => { }); } - if (countries.length === 0 && regions.length === 0 && massifs.length === 0) { - return res.badRequest({ - message: 'At least one country, region, or massif must be specified.', - }); - } - // Only validate entity types the caller actually changed: unchanged types are // already-persisted (and were validated on create/previous update), so we pass // empty arrays for them. resolveEntitiesExist short-circuits on empty arrays. diff --git a/assets/swaggerV1.yaml b/assets/swaggerV1.yaml index 46d93d63c..ec2e34ffc 100644 --- a/assets/swaggerV1.yaml +++ b/assets/swaggerV1.yaml @@ -3544,7 +3544,7 @@ paths: patch: tags: - guidelines - description: Update a guideline. Authenticated action (only the author or a moderator can update a guideline). + description: Update a guideline. Authenticated action (any authenticated user can update a guideline). security: - bearerAuth: [] parameters: @@ -3785,7 +3785,7 @@ paths: '401': description: Unauthorized (missing or invalid JWT token). '403': - description: Forbidden (user is not the author, a moderator, or an administrator). + description: Forbidden (user is not authorized to roll back this guideline). '404': description: Guideline or history snapshot not found. content: diff --git a/test/integration/4_routes/Guidelines/update.test.js b/test/integration/4_routes/Guidelines/update.test.js index 268072834..ae52395a9 100644 --- a/test/integration/4_routes/Guidelines/update.test.js +++ b/test/integration/4_routes/Guidelines/update.test.js @@ -22,15 +22,6 @@ describe('Guideline update', () => { .expect(404, done); }); - it('should return 403 if user is not author or moderator', (done) => { - supertest(sails.hooks.http.app) - .patch('/api/v1/guidelines/1') // Authored by user 3 - .send({ title: 'Attempted Title' }) - .set('Authorization', leaderToken) // User 7 is neither author (User 3) nor moderator - .set('Content-type', 'application/json') - .expect(403, done); - }); - it('should return 400 when title is empty', (done) => { supertest(sails.hooks.http.app) .patch('/api/v1/guidelines/1') @@ -76,6 +67,15 @@ describe('Guideline update', () => { .expect(400, done); }); + // Preservation baseline — Req 3.8: tokenAuth policy must remain unchanged + it('[PRESERVATION] should return 401 when no authorization token is provided', (done) => { + supertest(sails.hooks.http.app) + .patch('/api/v1/guidelines/1') + .send({ title: 'No Auth' }) + .set('Content-type', 'application/json') + .expect(401, done); + }); + it('should successfully update guideline and trigger history snapshot creation', async () => { const payload = { title: 'Updated Title By Moderator', @@ -98,5 +98,33 @@ describe('Guideline update', () => { const snapshots = await HGuideline.find({ t_id: 1 }); should(snapshots.length).be.greaterThan(1); }); + + // Bug condition exploration tests — EXPECTED TO FAIL on unfixed code + // Validates: Requirements 1.2, 1.3 + + it('[BUG-2] any authenticated user (non-author, non-moderator, non-admin) should be able to update a guideline (currently returns 403)', (done) => { + // isBugCondition_2: leaderToken is user 7 — authenticated, not the author (user 3), not a moderator, not an admin + // Expected correct behavior: 200. Current (buggy) behavior: 403. + // This test WILL FAIL on unfixed code — that failure documents the bug exists. + supertest(sails.hooks.http.app) + .patch('/api/v1/guidelines/1') + .send({ title: 'Updated By Leader' }) + .set('Authorization', leaderToken) + .set('Content-type', 'application/json') + .expect(200, done); + }); + + it('[BUG-1] PATCH with all geo associations set to empty arrays should return 200 (currently returns 400)', (done) => { + // isBugCondition_1: countries: [], regions: [], massifs: [] — all effective geo collections are empty + // Expected correct behavior: 200 with guideline saved with no geo associations. + // Current (buggy) behavior: 400 "At least one country, region, or massif must be specified." + // This test WILL FAIL on unfixed code — that failure documents the bug exists. + supertest(sails.hooks.http.app) + .patch('/api/v1/guidelines/1') + .send({ countries: [], regions: [], massifs: [] }) + .set('Authorization', moderatorToken) + .set('Content-type', 'application/json') + .expect(200, done); + }); }); }); From e311a47a549d05e13ed8136e36f17fd8d871392a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Mon, 31 Aug 2026 16:07:08 -0600 Subject: [PATCH 2/9] feat(guideline): add public GET /api/v1/guidelines/:id endpoint - Add extractCountryId pure helper to converters.js; export it for property tests - Update toSimpleGuideline regions mapping to return { id, name, countryId } objects instead of bare ISO strings - Create api/controllers/v1/guideline/find.js (public, no auth) - Add route GET /api/v1/guidelines/:id to config/routes.js - Add policy v1/guideline/find: [validateId] to config/policies.js - Add get: operation to swaggerV1.yaml under /guidelines/{id} - Create test/integration/4_routes/Guidelines/find.test.js (6 tests) - Fix find-for-entity.test.js region assertion for new object shape --- api/controllers/v1/guideline/find.js | 21 +++++ api/services/mapping/converters.js | 18 +++- assets/swaggerV1.yaml | 27 ++++++ config/policies.js | 1 + config/routes.js | 1 + .../Guidelines/find-for-entity.test.js | 2 +- .../4_routes/Guidelines/find.test.js | 85 +++++++++++++++++++ 7 files changed, 152 insertions(+), 3 deletions(-) create mode 100644 api/controllers/v1/guideline/find.js create mode 100644 test/integration/4_routes/Guidelines/find.test.js diff --git a/api/controllers/v1/guideline/find.js b/api/controllers/v1/guideline/find.js new file mode 100644 index 000000000..a92c27ecd --- /dev/null +++ b/api/controllers/v1/guideline/find.js @@ -0,0 +1,21 @@ +const ControllerService = require('../../../services/ControllerService'); +const GuidelineService = require('../../../services/GuidelineService'); +const { toSimpleGuideline } = require('../../../services/mapping/converters'); + +module.exports = async (req, res) => { + const guidelineId = req.param('id'); + const guideline = await GuidelineService.getGuideline(guidelineId); + if (!guideline || guideline.isDeleted) { + return res.notFound({ + message: `Guideline of id ${guidelineId} not found.`, + }); + } + return ControllerService.treatAndConvert( + req, + null, + guideline, + { controllerMethod: 'GuidelineController.find' }, + res, + toSimpleGuideline + ); +}; diff --git a/api/services/mapping/converters.js b/api/services/mapping/converters.js index e56301ac6..407a243d3 100644 --- a/api/services/mapping/converters.js +++ b/api/services/mapping/converters.js @@ -23,6 +23,12 @@ const { getQualityBreakdown, } = require('../../utils/computeEntranceDataQuality'); +const extractCountryId = (regionId) => { + if (!regionId || typeof regionId !== 'string') return null; + const dash = regionId.indexOf('-'); + return dash > 0 ? regionId.slice(0, dash) : null; +}; + const c = { toCave: (source, meta) => { const result = { @@ -971,7 +977,15 @@ const c = { source, (country) => country.id || country ), - regions: toList('regions', source, (region) => region.id || region), + regions: toList('regions', source, (region) => { + const id = region.id || region; + const name = region instanceof Object ? region.name : undefined; + return { + id, + name, + countryId: extractCountryId(typeof id === 'string' ? id : String(id)), + }; + }), massifs: toList('massifs', source, (massif) => massif instanceof Object ? c.toSimpleMassif(massif) : { id: massif } ), @@ -1076,4 +1090,4 @@ const c = { }), }; -module.exports = c; +module.exports = { ...c, extractCountryId }; diff --git a/assets/swaggerV1.yaml b/assets/swaggerV1.yaml index ec2e34ffc..8f9914f4b 100644 --- a/assets/swaggerV1.yaml +++ b/assets/swaggerV1.yaml @@ -3541,6 +3541,33 @@ paths: type: string '/guidelines/{id}': + get: + tags: + - guidelines + description: Get a single guideline by ID. Public endpoint — no authentication required. + parameters: + - name: id + in: path + required: true + schema: + type: integer + description: Guideline ID + responses: + '200': + description: Successful operation + content: + application/json: + schema: + $ref: '#/components/schemas/Guideline' + '404': + description: Guideline not found or soft-deleted. + content: + application/json: + schema: + type: object + properties: + message: + type: string patch: tags: - guidelines diff --git a/config/policies.js b/config/policies.js index ed46dd3fd..8135c544f 100644 --- a/config/policies.js +++ b/config/policies.js @@ -194,6 +194,7 @@ module.exports.policies = { // Guideline 'v1/guideline/find-all': true, + 'v1/guideline/find': ['validateId'], 'v1/guideline/create': 'tokenAuth', 'v1/guideline/update': ['validateId', 'tokenAuth'], 'v1/guideline/delete': ['validateId', 'tokenAuth'], diff --git a/config/routes.js b/config/routes.js index f5ffa1f0a..1cc587a3a 100644 --- a/config/routes.js +++ b/config/routes.js @@ -333,6 +333,7 @@ module.exports.routes = { // Guideline 'GET /api/v1/guidelines': 'v1/guideline/find-all', 'POST /api/v1/guidelines': 'v1/guideline/create', + 'GET /api/v1/guidelines/:id': 'v1/guideline/find', 'PATCH /api/v1/guidelines/:id': 'v1/guideline/update', 'DELETE /api/v1/guidelines/:id': 'v1/guideline/delete', 'POST /api/v1/guidelines/:id/restore': 'v1/guideline/restore', diff --git a/test/integration/4_routes/Guidelines/find-for-entity.test.js b/test/integration/4_routes/Guidelines/find-for-entity.test.js index df818e500..b36183ae3 100644 --- a/test/integration/4_routes/Guidelines/find-for-entity.test.js +++ b/test/integration/4_routes/Guidelines/find-for-entity.test.js @@ -69,7 +69,7 @@ describe('Guideline find-for-entity', () => { if (err) return done(err); should(res.body).be.an.Array(); should(res.body.length).be.greaterThan(0); - should(res.body[0].regions).containEql('FR-01'); + should(res.body[0].regions.map((r) => r.id)).containEql('FR-01'); return done(); }); }); diff --git a/test/integration/4_routes/Guidelines/find.test.js b/test/integration/4_routes/Guidelines/find.test.js new file mode 100644 index 000000000..044142808 --- /dev/null +++ b/test/integration/4_routes/Guidelines/find.test.js @@ -0,0 +1,85 @@ +const supertest = require('supertest'); +const should = require('should'); + +// Requirements: 2.5, 2.6, 2.7, 2.8, 2.9 +describe('Guideline find', () => { + let deletedGuidelineId; + + before(async () => { + // Create a soft-deleted guideline for 404 testing (no fixture needed) + const guideline = await TGuideline.create({ + title: 'Deleted Guideline For Find Test', + author: 3, + language: 'fra', + dateInscription: new Date(), + isDeleted: true, + }).fetch(); + deletedGuidelineId = guideline.id; + }); + + describe('GET /api/v1/guidelines/:id', () => { + // Req 2.5, 2.8: public endpoint — no auth required, returns 200 + it('should return 200 without Authorization header (public endpoint)', (done) => { + supertest(sails.hooks.http.app) + .get('/api/v1/guidelines/1') + .expect(200, done); + }); + + // Req 2.5, 2.6: existing non-deleted guideline returns 200 with correct shape + it('should return 200 with correct shape for an existing non-deleted guideline', async () => { + const res = await supertest(sails.hooks.http.app) + .get('/api/v1/guidelines/1') + .expect(200); + + const g = res.body; + should(g).have.property('id', 1); + should(g).have.property('title').which.is.a.String(); + should(g).have.property('description').which.is.a.String(); + should(g).have.property('language'); + should(g).have.property('countries').which.is.an.Array(); + should(g).have.property('regions').which.is.an.Array(); + should(g).have.property('massifs').which.is.an.Array(); + should(g).have.property('author'); + should(g).have.property('isDeleted', false); + }); + + // Req 2.6: guideline 1 has country FR — verify countries array contains the ISO code + it('should return guideline 1 with country FR in countries array', async () => { + const res = await supertest(sails.hooks.http.app) + .get('/api/v1/guidelines/1') + .expect(200); + + const g = res.body; + should(g.countries).have.length(1); + should(g.countries[0]).equal('FR'); + }); + + // Req 2.7, 2.9: guideline 3 has region FR-01 — verify regions array shape with countryId + it('should return guideline 3 with region FR-01 having correct shape including countryId', async () => { + const res = await supertest(sails.hooks.http.app) + .get('/api/v1/guidelines/3') + .expect(200); + + const g = res.body; + should(g.regions).have.length(1); + const region = g.regions[0]; + should(region).have.property('id', 'FR-01'); + should(region).have.property('name').which.is.a.String(); + should(region).have.property('countryId', 'FR'); + }); + + // Req 2.8: soft-deleted guideline returns 404 + it('should return 404 for a soft-deleted guideline', (done) => { + supertest(sails.hooks.http.app) + .get(`/api/v1/guidelines/${deletedGuidelineId}`) + .expect(404, done); + }); + + // Req 2.8: non-existent ID returns 404 + it('should return 404 for a non-existent guideline ID', (done) => { + supertest(sails.hooks.http.app) + .get('/api/v1/guidelines/999999') + .expect(404, done); + }); + }); +}); From a8be8e0f13c02bffad987a6ee60cafb3793d7866 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Mon, 31 Aug 2026 16:19:47 -0600 Subject: [PATCH 3/9] test(guideline): add property-based tests for extractCountryId and validateMassifIds --- .../GuidelineService.property.test.js | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 test/integration/1_services/GuidelineService.property.test.js diff --git a/test/integration/1_services/GuidelineService.property.test.js b/test/integration/1_services/GuidelineService.property.test.js new file mode 100644 index 000000000..e7f3b8846 --- /dev/null +++ b/test/integration/1_services/GuidelineService.property.test.js @@ -0,0 +1,133 @@ +/* eslint-disable func-names */ +const should = require('should'); +const fc = require('fast-check'); +const { + extractCountryId, +} = require('../../../api/services/mapping/converters'); +const { validateMassifIds } = require('../../../api/services/GuidelineService'); + +/** + * Property 1: extractCountryId returns the country prefix for any valid ISO 3166-2 code + * + * For any string matching the ISO 3166-2 pattern (e.g. "FR-01", "US-CA"), + * extractCountryId must return the part before the first '-'. + * + * Validates: Requirements 2.9 + */ +describe('extractCountryId - Property 1: ISO 3166-2 prefix extraction', () => { + it('extractCountryId returns the country prefix for any valid ISO 3166-2 code', function () { + this.timeout(30000); + fc.assert( + fc.property(fc.stringMatching(/^[A-Z]{2,3}-[A-Z0-9]+$/), (code) => { + const result = extractCountryId(code); + const expected = code.slice(0, code.indexOf('-')); + should(result).equal(expected); + }), + { numRuns: 100 } + ); + }); +}); + +/** + * Property 2: extractCountryId returns null for null, non-strings, and strings without a dash + * + * For hostile inputs (null, integers, and arbitrary strings which may or may + * not contain a '-'), extractCountryId must return null whenever the input is + * not a string or does not contain a '-' at position > 0. + * + * Validates: Requirements 2.9 + */ +describe('extractCountryId - Property 2: hostile inputs return null', () => { + it('extractCountryId returns null for null, non-strings, and strings without a dash', function () { + this.timeout(30000); + fc.assert( + fc.property( + fc.oneof(fc.constant(null), fc.integer(), fc.string()), + (input) => { + // For this property we only assert null when input is not a valid + // ISO 3166-2 string (i.e. not a string, or a string with no '-', or + // '-' at position 0). + if ( + input === null || + typeof input !== 'string' || + input.indexOf('-') <= 0 + ) { + const result = extractCountryId(input); + should(result).be.null(); + } + } + ), + { numRuns: 100 } + ); + }); +}); + +/** + * Property 3: validateMassifIds returns true for arrays of positive finite numbers + * + * For any array whose every element is a positive finite number (> 0, not NaN, + * not Infinity), validateMassifIds must return true. + * + * Validates: Requirements 3.3 + */ +describe('validateMassifIds - Property 3: positive finite arrays return true', () => { + it('validateMassifIds returns true for arrays of positive finite numbers', function () { + this.timeout(30000); + fc.assert( + fc.property( + fc.array( + fc.float({ + min: Math.fround(0.001), + max: Math.fround(9999), + noNaN: true, + }) + ), + (arr) => { + const result = validateMassifIds(arr); + should(result).be.true(); + } + ), + { numRuns: 100 } + ); + }); +}); + +/** + * Property 4: validateMassifIds returns false when any element is non-positive or non-finite + * + * For any array that contains at least one invalid value (0, -1, NaN, + * Infinity, or -Infinity), validateMassifIds must return false. + * + * Validates: Requirements 3.3 + */ +describe('validateMassifIds - Property 4: array with any invalid element returns false', () => { + it('validateMassifIds returns false when any element is non-positive or non-finite', function () { + this.timeout(30000); + fc.assert( + fc.property( + fc + .tuple( + fc.array( + fc.float({ + min: Math.fround(0.001), + max: Math.fround(9999), + noNaN: true, + }) + ), + fc.oneof( + fc.constant(0), + fc.constant(-1), + fc.constant(NaN), + fc.constant(Infinity) + ) + ) + .map(([valid, invalid]) => [...valid, invalid]), + (arr) => { + const result = validateMassifIds(arr); + should(result).be.false(); + } + ), + { numRuns: 100 } + ); + }); +}); From 16428abfa05c524698c565158e85c51588fba007 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Mon, 31 Aug 2026 17:50:32 -0600 Subject: [PATCH 4/9] fix(guideline): hydrate detail response and scope it to a new converter Addresses review findings on the GET /api/v1/guidelines/:id endpoint added for issue #1782. - Add a `toGuideline` detail converter and revert `toSimpleGuideline`'s regions to bare ISO strings. Enriching the shared converter was a response type change on six endpoints (find-all, find-for-entity, get-snapshots, toMassif, toRegion, toCountry) that neither issue asked to change. - Add `GuidelineService.getGuidelineDetail`, which populates `language` and resolves massif names from t_name via a single batched `NameService.setNames` call (no N+1). Kept separate from `getGuideline` so create/update/rollback/restore/delete responses are unaffected. - Hydrate `countries` as `{ id, name }` and `language` as an object with `refName`, satisfying #1782's "readable names" acceptance criterion. Previously only `regions` was hydrated. - Make find.test.js create its own guideline instead of asserting on the seeded guideline 1, whose associations update.test.js clears in the same shard. The prior assertions failed under `mocha update.test.js find.test.js`. - Document the detail shape as a new `GuidelineDetail` schema, leaving the shared `Guideline` schema's `regions: string` correct, and drop the unreachable 403 responses from the patch and rollback operations. --- api/controllers/v1/guideline/find.js | 6 +- api/services/GuidelineService.js | 29 +++++ api/services/mapping/converters.js | 40 ++++-- assets/swaggerV1.yaml | 88 +++++++++++++- .../Guidelines/find-for-entity.test.js | 2 +- .../4_routes/Guidelines/find.test.js | 114 +++++++++++++++--- 6 files changed, 240 insertions(+), 39 deletions(-) diff --git a/api/controllers/v1/guideline/find.js b/api/controllers/v1/guideline/find.js index a92c27ecd..e13a607a9 100644 --- a/api/controllers/v1/guideline/find.js +++ b/api/controllers/v1/guideline/find.js @@ -1,10 +1,10 @@ const ControllerService = require('../../../services/ControllerService'); const GuidelineService = require('../../../services/GuidelineService'); -const { toSimpleGuideline } = require('../../../services/mapping/converters'); +const { toGuideline } = require('../../../services/mapping/converters'); module.exports = async (req, res) => { const guidelineId = req.param('id'); - const guideline = await GuidelineService.getGuideline(guidelineId); + const guideline = await GuidelineService.getGuidelineDetail(guidelineId); if (!guideline || guideline.isDeleted) { return res.notFound({ message: `Guideline of id ${guidelineId} not found.`, @@ -16,6 +16,6 @@ module.exports = async (req, res) => { guideline, { controllerMethod: 'GuidelineController.find' }, res, - toSimpleGuideline + toGuideline ); }; diff --git a/api/services/GuidelineService.js b/api/services/GuidelineService.js index 44a610843..9206d0d11 100644 --- a/api/services/GuidelineService.js +++ b/api/services/GuidelineService.js @@ -1,4 +1,5 @@ const CaveService = require('./CaveService'); +const NameService = require('./NameService'); module.exports = { /** @@ -29,6 +30,34 @@ module.exports = { .populate('regions') .populate('massifs'), + /** + * Fetch a single guideline for the public detail endpoint. + * + * Adds two hydrations on top of getGuideline that only the detail view needs + * (see toGuideline): `language`, so the response can carry its readable + * `refName` instead of the bare FK code, and the massifs' names, which live + * in the separate t_name table. Kept separate from getGuideline so the + * create/update/rollback/restore/delete responses — which all use the leaner + * toSimpleGuideline shape — are unaffected. + * + * Massif names are resolved in a single batched query via NameService rather + * than one lookup per massif, so this stays free of N+1 queries. + * @param {number} id - The ID of the guideline + * @returns {Promise} The guideline record or null/undefined + */ + getGuidelineDetail: async (id) => { + const guideline = await TGuideline.findOne({ id }) + .populate('author') + .populate('reviewer') + .populate('countries') + .populate('regions') + .populate('massifs') + .populate('language'); + if (!guideline) return guideline; + await NameService.setNames(guideline.massifs, 'massif'); + return guideline; + }, + /** * Fetch all history snapshots for a given guideline ID, populating author and reviewer. * @param {number} guidelineId - The ID of the target guideline diff --git a/api/services/mapping/converters.js b/api/services/mapping/converters.js index 407a243d3..7f3b84aaf 100644 --- a/api/services/mapping/converters.js +++ b/api/services/mapping/converters.js @@ -23,6 +23,12 @@ const { getQualityBreakdown, } = require('../../utils/computeEntranceDataQuality'); +/** + * Derive a country code from an ISO 3166-2 region code ('FR-01' -> 'FR'). + * Returns null for anything that isn't a string containing a '-' past the + * first character, so a malformed or already-numeric id yields no country + * rather than a bogus prefix. + */ const extractCountryId = (regionId) => { if (!regionId || typeof regionId !== 'string') return null; const dash = regionId.indexOf('-'); @@ -977,15 +983,7 @@ const c = { source, (country) => country.id || country ), - regions: toList('regions', source, (region) => { - const id = region.id || region; - const name = region instanceof Object ? region.name : undefined; - return { - id, - name, - countryId: extractCountryId(typeof id === 'string' ? id : String(id)), - }; - }), + regions: toList('regions', source, (region) => region.id || region), massifs: toList('massifs', source, (massif) => massif instanceof Object ? c.toSimpleMassif(massif) : { id: massif } ), @@ -1002,6 +1000,30 @@ const c = { return result; }, + // Detail view for GET /api/v1/guidelines/:id. Unlike toSimpleGuideline (used + // by the list, by-entity and snapshot endpoints, which keep the leaner + // shape), this hydrates the geographic relations and the language with + // readable names so the front end can render a standalone guideline page + // without follow-up requests. Relies on + // GuidelineService.getGuidelineDetail having populated `language` and the + // massifs' nested `names`. + toGuideline: (source) => ({ + ...c.toSimpleGuideline(source), + countries: toList('countries', source, (country) => + country instanceof Object + ? { id: country.id, name: country.nativeName } + : { id: country } + ), + regions: toList('regions', source, (region) => { + const id = region instanceof Object ? region.id : region; + return { + id, + name: region instanceof Object ? region.name : undefined, + countryId: extractCountryId(id), + }; + }), + }), + // Transform the typesense response toSearchResult: (source, meta) => { // For each result of the search, convert the item and add it to the json to send diff --git a/assets/swaggerV1.yaml b/assets/swaggerV1.yaml index 8f9914f4b..a98b74b97 100644 --- a/assets/swaggerV1.yaml +++ b/assets/swaggerV1.yaml @@ -3544,7 +3544,10 @@ paths: get: tags: - guidelines - description: Get a single guideline by ID. Public endpoint — no authentication required. + description: >- + Get a single guideline by ID. Public endpoint — no authentication + required. Geographic relations and the language are hydrated with + readable names (see `GuidelineDetail`). parameters: - name: id in: path @@ -3558,7 +3561,7 @@ paths: content: application/json: schema: - $ref: '#/components/schemas/Guideline' + $ref: '#/components/schemas/GuidelineDetail' '404': description: Guideline not found or soft-deleted. content: @@ -3633,8 +3636,6 @@ paths: type: string '401': description: Unauthorized (missing or invalid JWT token). - '403': - description: Forbidden (user is not authorized to update this guideline). '404': description: Guideline not found or already deleted. content: @@ -3811,8 +3812,6 @@ paths: type: string '401': description: Unauthorized (missing or invalid JWT token). - '403': - description: Forbidden (user is not authorized to roll back this guideline). '404': description: Guideline or history snapshot not found. content: @@ -8782,6 +8781,83 @@ components: type: integer description: "Only present in history snapshots. The ID of the parent guideline this snapshot belongs to." + GuidelineDetail: + type: object + description: >- + Returned by `GET /guidelines/{id}` only. Same fields as `Guideline`, but + the geographic relations and the language are hydrated with readable + names so a guideline detail page can be rendered without follow-up + requests. The other guideline endpoints return the leaner `Guideline` + shape, where `countries`/`regions` are bare ID strings. + properties: + id: + type: integer + title: + type: string + description: + type: string + nullable: true + countries: + type: array + items: + type: object + properties: + id: + type: string + description: ISO 3166-1 alpha-2 country code. + name: + type: string + nullable: true + description: The country's native name. + regions: + type: array + items: + type: object + properties: + id: + type: string + description: ISO 3166-2 subdivision code. + name: + type: string + nullable: true + countryId: + type: string + nullable: true + description: >- + Country code derived from the ISO 3166-2 prefix + (e.g. `FR-01` yields `FR`). Null when the code has no prefix. + massifs: + type: array + items: + type: object + properties: + id: + type: integer + name: + type: string + nullable: true + language: + type: string + nullable: true + isDeleted: + type: boolean + dateInscription: + type: string + format: date-time + dateReviewed: + type: string + format: date-time + nullable: true + isDeleted: + type: boolean + author: + $ref: '#/components/schemas/Caver' + reviewer: + $ref: '#/components/schemas/Caver' + nullable: true + language: + $ref: '#/components/schemas/Language' + HealthStatus: type: object properties: diff --git a/test/integration/4_routes/Guidelines/find-for-entity.test.js b/test/integration/4_routes/Guidelines/find-for-entity.test.js index b36183ae3..df818e500 100644 --- a/test/integration/4_routes/Guidelines/find-for-entity.test.js +++ b/test/integration/4_routes/Guidelines/find-for-entity.test.js @@ -69,7 +69,7 @@ describe('Guideline find-for-entity', () => { if (err) return done(err); should(res.body).be.an.Array(); should(res.body.length).be.greaterThan(0); - should(res.body[0].regions.map((r) => r.id)).containEql('FR-01'); + should(res.body[0].regions).containEql('FR-01'); return done(); }); }); diff --git a/test/integration/4_routes/Guidelines/find.test.js b/test/integration/4_routes/Guidelines/find.test.js index 044142808..03581314b 100644 --- a/test/integration/4_routes/Guidelines/find.test.js +++ b/test/integration/4_routes/Guidelines/find.test.js @@ -3,71 +3,131 @@ const should = require('should'); // Requirements: 2.5, 2.6, 2.7, 2.8, 2.9 describe('Guideline find', () => { + // Guidelines dedicated to this file. Sibling test files in this same folder + // mutate the seeded rows — update.test.js clears guideline 1's geographic + // associations — and the whole 4_routes/Guidelines/ folder runs together in + // one shard, so asserting on a seeded guideline's associations would be + // order-dependent. These rows are created here and only read by this file + // (mirroring rollback.test.js). + let guidelineId; let deletedGuidelineId; before(async () => { - // Create a soft-deleted guideline for 404 testing (no fixture needed) const guideline = await TGuideline.create({ + title: 'Find Detail Guideline', + description: 'A guideline owned by find.test.js.', + author: 3, + reviewer: 2, + language: 'fra', + dateInscription: new Date(), + }).fetch(); + guidelineId = guideline.id; + await TGuideline.addToCollection(guidelineId, 'countries', ['FR']); + await TGuideline.addToCollection(guidelineId, 'regions', ['FR-01']); + await TGuideline.addToCollection(guidelineId, 'massifs', [1]); + + // A soft-deleted guideline for 404 testing (no fixture needed) + const deleted = await TGuideline.create({ title: 'Deleted Guideline For Find Test', author: 3, language: 'fra', dateInscription: new Date(), isDeleted: true, }).fetch(); - deletedGuidelineId = guideline.id; + deletedGuidelineId = deleted.id; }); describe('GET /api/v1/guidelines/:id', () => { // Req 2.5, 2.8: public endpoint — no auth required, returns 200 it('should return 200 without Authorization header (public endpoint)', (done) => { supertest(sails.hooks.http.app) - .get('/api/v1/guidelines/1') + .get(`/api/v1/guidelines/${guidelineId}`) .expect(200, done); }); // Req 2.5, 2.6: existing non-deleted guideline returns 200 with correct shape it('should return 200 with correct shape for an existing non-deleted guideline', async () => { const res = await supertest(sails.hooks.http.app) - .get('/api/v1/guidelines/1') + .get(`/api/v1/guidelines/${guidelineId}`) .expect(200); const g = res.body; - should(g).have.property('id', 1); + should(g).have.property('id', guidelineId); should(g).have.property('title').which.is.a.String(); should(g).have.property('description').which.is.a.String(); - should(g).have.property('language'); should(g).have.property('countries').which.is.an.Array(); should(g).have.property('regions').which.is.an.Array(); should(g).have.property('massifs').which.is.an.Array(); - should(g).have.property('author'); should(g).have.property('isDeleted', false); + should(g.author).have.property('id', 3); + should(g.reviewer).have.property('id', 2); }); - // Req 2.6: guideline 1 has country FR — verify countries array contains the ISO code - it('should return guideline 1 with country FR in countries array', async () => { + // Req 2.5: language is hydrated with its readable refName, not a bare code + it('should hydrate language with its id and readable refName', async () => { const res = await supertest(sails.hooks.http.app) - .get('/api/v1/guidelines/1') + .get(`/api/v1/guidelines/${guidelineId}`) .expect(200); - const g = res.body; - should(g.countries).have.length(1); - should(g.countries[0]).equal('FR'); + should(res.body.language).have.property('id', 'fra'); + should(res.body.language).have.property('refName', 'French'); }); - // Req 2.7, 2.9: guideline 3 has region FR-01 — verify regions array shape with countryId - it('should return guideline 3 with region FR-01 having correct shape including countryId', async () => { + // Req 2.6: countries carry their ISO id and a readable name + it('should return countries with their id and readable name', async () => { const res = await supertest(sails.hooks.http.app) - .get('/api/v1/guidelines/3') + .get(`/api/v1/guidelines/${guidelineId}`) .expect(200); - const g = res.body; - should(g.regions).have.length(1); - const region = g.regions[0]; + should(res.body.countries).have.length(1); + should(res.body.countries[0]).have.property('id', 'FR'); + should(res.body.countries[0]).have.property('name', 'France'); + }); + + // Req 2.7, 2.9: regions carry id, name and the countryId derived from the ISO prefix + it('should return regions with id, name and countryId', async () => { + const res = await supertest(sails.hooks.http.app) + .get(`/api/v1/guidelines/${guidelineId}`) + .expect(200); + + should(res.body.regions).have.length(1); + const region = res.body.regions[0]; should(region).have.property('id', 'FR-01'); - should(region).have.property('name').which.is.a.String(); + should(region).have.property('name', 'Ain'); should(region).have.property('countryId', 'FR'); }); + // Req 2.6: massif names live in t_name, so they must be hydrated too + it('should return massifs with their id and readable name', async () => { + const res = await supertest(sails.hooks.http.app) + .get(`/api/v1/guidelines/${guidelineId}`) + .expect(200); + + should(res.body.massifs).have.length(1); + should(res.body.massifs[0]).have.property('id', 1); + should(res.body.massifs[0]).have.property('name').which.is.a.String(); + should(res.body.massifs[0].name).not.be.empty(); + }); + + // A guideline with no geographic associations is valid (see issue #1775) + // and must still serialize as empty arrays rather than 404 or null. + it('should return empty arrays for a guideline with no geographic associations', async () => { + const bare = await TGuideline.create({ + title: 'Find Guideline Without Geo', + author: 3, + language: 'fra', + dateInscription: new Date(), + }).fetch(); + + const res = await supertest(sails.hooks.http.app) + .get(`/api/v1/guidelines/${bare.id}`) + .expect(200); + + should(res.body.countries).be.an.Array().and.be.empty(); + should(res.body.regions).be.an.Array().and.be.empty(); + should(res.body.massifs).be.an.Array().and.be.empty(); + }); + // Req 2.8: soft-deleted guideline returns 404 it('should return 404 for a soft-deleted guideline', (done) => { supertest(sails.hooks.http.app) @@ -82,4 +142,18 @@ describe('Guideline find', () => { .expect(404, done); }); }); + + // Req 3.9, 3.10: the leaner toSimpleGuideline shape is unchanged on the + // endpoints that share it — only the detail endpoint hydrates relations. + describe('shared converter is unaffected', () => { + it('should keep bare ISO strings for regions on the by-entity endpoint', async () => { + const res = await supertest(sails.hooks.http.app) + .get('/api/v1/guidelines/by-entity/region/FR-01') + .expect(200); + + should(res.body).be.an.Array(); + should(res.body.length).be.greaterThan(0); + should(res.body[0].regions).containEql('FR-01'); + }); + }); }); From 4c5766565fd855a82d3dd0c96d44da6d41559824 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Fri, 4 Sep 2026 11:07:36 -0600 Subject: [PATCH 5/9] fix(guideline): correct Language schema and guard relaxed rollback RBAC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on #1798. - Correct the shared Language schema: it declared `ref_name`, but the API serializes `refName` (`ref_name` is only the column name in TLanguage). Fixed on the shared schema rather than overridden locally because all 8 reference sites — /languages, /languages/{id}, Document.languages, Document.mainLanguage, Location.language, Guideline.language and GuidelineDetail.language — serialize through toLanguage or raw Waterline records, so every one of them was misdocumented. Also document `isPrefered`, which is returned but was absent from the schema. - Add a rollback success case for an authenticated non-author who is neither moderator nor administrator. The existing success case authenticates as user1 (id 3), who is also the guideline's author, so it still passed with the removed author-check restored; the new test fails in that state, which is what makes it a regression guard. It needs its own guideline: the existing case consumes its row's snapshot state, and the fixture uses distinct timestamps because Waterline cannot express h_guideline's composite (id, date_reviewed) primary key, so the migrate:drop-built test schema keys it on date_reviewed alone. - Move the validateMassifIds property tests to their own file. That validator predates this change set and is untouched by it. --- assets/swaggerV1.yaml | 8 +- .../GuidelineService.property.test.js | 71 ---------------- ...eServiceValidateMassifIds.property.test.js | 78 ++++++++++++++++++ .../4_routes/Guidelines/rollback.test.js | 81 +++++++++++++++++++ 4 files changed, 166 insertions(+), 72 deletions(-) create mode 100644 test/integration/1_services/GuidelineServiceValidateMassifIds.property.test.js diff --git a/assets/swaggerV1.yaml b/assets/swaggerV1.yaml index a98b74b97..4a50e4b23 100644 --- a/assets/swaggerV1.yaml +++ b/assets/swaggerV1.yaml @@ -8939,8 +8939,14 @@ components: type: string type: type: string - ref_name: + refName: type: string + description: >- + The language's readable name (e.g. `French`). Serialized as + `refName`; `ref_name` is the underlying column name, not the API + attribute. + isPrefered: + type: boolean comment: type: string diff --git a/test/integration/1_services/GuidelineService.property.test.js b/test/integration/1_services/GuidelineService.property.test.js index e7f3b8846..a6c0cc3fd 100644 --- a/test/integration/1_services/GuidelineService.property.test.js +++ b/test/integration/1_services/GuidelineService.property.test.js @@ -4,7 +4,6 @@ const fc = require('fast-check'); const { extractCountryId, } = require('../../../api/services/mapping/converters'); -const { validateMassifIds } = require('../../../api/services/GuidelineService'); /** * Property 1: extractCountryId returns the country prefix for any valid ISO 3166-2 code @@ -61,73 +60,3 @@ describe('extractCountryId - Property 2: hostile inputs return null', () => { ); }); }); - -/** - * Property 3: validateMassifIds returns true for arrays of positive finite numbers - * - * For any array whose every element is a positive finite number (> 0, not NaN, - * not Infinity), validateMassifIds must return true. - * - * Validates: Requirements 3.3 - */ -describe('validateMassifIds - Property 3: positive finite arrays return true', () => { - it('validateMassifIds returns true for arrays of positive finite numbers', function () { - this.timeout(30000); - fc.assert( - fc.property( - fc.array( - fc.float({ - min: Math.fround(0.001), - max: Math.fround(9999), - noNaN: true, - }) - ), - (arr) => { - const result = validateMassifIds(arr); - should(result).be.true(); - } - ), - { numRuns: 100 } - ); - }); -}); - -/** - * Property 4: validateMassifIds returns false when any element is non-positive or non-finite - * - * For any array that contains at least one invalid value (0, -1, NaN, - * Infinity, or -Infinity), validateMassifIds must return false. - * - * Validates: Requirements 3.3 - */ -describe('validateMassifIds - Property 4: array with any invalid element returns false', () => { - it('validateMassifIds returns false when any element is non-positive or non-finite', function () { - this.timeout(30000); - fc.assert( - fc.property( - fc - .tuple( - fc.array( - fc.float({ - min: Math.fround(0.001), - max: Math.fround(9999), - noNaN: true, - }) - ), - fc.oneof( - fc.constant(0), - fc.constant(-1), - fc.constant(NaN), - fc.constant(Infinity) - ) - ) - .map(([valid, invalid]) => [...valid, invalid]), - (arr) => { - const result = validateMassifIds(arr); - should(result).be.false(); - } - ), - { numRuns: 100 } - ); - }); -}); diff --git a/test/integration/1_services/GuidelineServiceValidateMassifIds.property.test.js b/test/integration/1_services/GuidelineServiceValidateMassifIds.property.test.js new file mode 100644 index 000000000..d2bdd66b4 --- /dev/null +++ b/test/integration/1_services/GuidelineServiceValidateMassifIds.property.test.js @@ -0,0 +1,78 @@ +/* eslint-disable func-names */ +const should = require('should'); +const fc = require('fast-check'); +const { validateMassifIds } = require('../../../api/services/GuidelineService'); + +/** + * Property-based coverage for GuidelineService.validateMassifIds. + * + * This validator predates the guideline geo/RBAC changes and its behavior is + * unchanged by them; it lives in its own file so it is not read as part of + * that change set. + */ + +/** + * Property 1: validateMassifIds returns true for arrays of positive finite numbers + * + * For any array whose every element is a positive finite number (> 0, not NaN, + * not Infinity), validateMassifIds must return true. + */ +describe('validateMassifIds - Property 1: positive finite arrays return true', () => { + it('validateMassifIds returns true for arrays of positive finite numbers', function () { + this.timeout(30000); + fc.assert( + fc.property( + fc.array( + fc.float({ + min: Math.fround(0.001), + max: Math.fround(9999), + noNaN: true, + }) + ), + (arr) => { + const result = validateMassifIds(arr); + should(result).be.true(); + } + ), + { numRuns: 100 } + ); + }); +}); + +/** + * Property 2: validateMassifIds returns false when any element is non-positive or non-finite + * + * For any array that contains at least one invalid value (0, -1, NaN, + * Infinity, or -Infinity), validateMassifIds must return false. + */ +describe('validateMassifIds - Property 2: array with any invalid element returns false', () => { + it('validateMassifIds returns false when any element is non-positive or non-finite', function () { + this.timeout(30000); + fc.assert( + fc.property( + fc + .tuple( + fc.array( + fc.float({ + min: Math.fround(0.001), + max: Math.fround(9999), + noNaN: true, + }) + ), + fc.oneof( + fc.constant(0), + fc.constant(-1), + fc.constant(NaN), + fc.constant(Infinity) + ) + ) + .map(([valid, invalid]) => [...valid, invalid]), + (arr) => { + const result = validateMassifIds(arr); + should(result).be.false(); + } + ), + { numRuns: 100 } + ); + }); +}); diff --git a/test/integration/4_routes/Guidelines/rollback.test.js b/test/integration/4_routes/Guidelines/rollback.test.js index e0d78f39a..a61d960d5 100644 --- a/test/integration/4_routes/Guidelines/rollback.test.js +++ b/test/integration/4_routes/Guidelines/rollback.test.js @@ -4,6 +4,7 @@ const AuthTokenService = require('../../AuthTokenService'); describe('Guideline rollback', () => { let userToken; + let leaderToken; // A guideline dedicated to the success case below. Sibling test files in // this same folder (e.g. update.test.js) mutate the seeded guideline 1, and @@ -16,8 +17,15 @@ describe('Guideline rollback', () => { const targetTitle = 'Rollback Target Title'; const targetDescription = 'Target state to roll back to.'; + // A second guideline for the non-author permission case. The success case + // below consumes its guideline's snapshot state, so that row cannot be + // rolled back twice. + let nonAuthorGuidelineId; + const nonAuthorTargetTitle = 'Rollback Target Title For Non Author'; + before(async () => { userToken = await AuthTokenService.getRawBearerUserToken(); + leaderToken = await AuthTokenService.getRawBearerLeaderToken(); // 1. Create the guideline. The AFTER INSERT trigger snapshots these // initial values at date_reviewed = '2024-01-01 10:00:00'. @@ -59,6 +67,47 @@ describe('Guideline rollback', () => { false, ] ); + + // Same three-step setup for the non-author case, authored by user 3 so the + // leader (user 7) is provably not the author. + // + // The timestamps below must differ from the ones used above: Waterline + // cannot express h_guideline's composite (id, date_reviewed) primary key, + // so the migrate:drop-built test schema keys it on date_reviewed alone. + // Two guidelines sharing a date_reviewed therefore collide in the history + // table even though the real SQL schema in sql/0_tables.sql allows it. + const nonAuthorGuideline = await TGuideline.create({ + title: 'Rollback Seed Title For Non Author', + description: 'Seed state.', + author: 3, + reviewer: 2, + language: 'fra', + dateInscription: '2024-02-01T10:00:00.000Z', + dateReviewed: '2024-02-01T10:00:00.000Z', + isDeleted: false, + }).fetch(); + nonAuthorGuidelineId = nonAuthorGuideline.id; + + await TGuideline.updateOne({ id: nonAuthorGuidelineId }).set({ + title: 'Rollback Current Title For Non Author', + description: 'Current state before rollback.', + }); + + await sails.sendNativeQuery( + `INSERT INTO h_guideline + (id, title, description, id_author, id_language, date_inscription, date_reviewed, is_deleted) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8)`, + [ + nonAuthorGuidelineId, + nonAuthorTargetTitle, + 'Target state to roll back to.', + 3, + 'fra', + '2024-07-01T09:00:00.000Z', + '2024-07-01T09:00:00.000Z', + false, + ] + ); }); describe('rollback', () => { @@ -125,5 +174,37 @@ describe('Guideline rollback', () => { ); should(preRollbackSnapshot).be.ok(); }); + + // Guards the RBAC relaxation from this change (issue #1775). The success + // case above authenticates as user1 (id 3), who is also the guideline's + // author, so it would still pass if the removed author-check came back. + // The leader (user 7) is authenticated but is neither the author, a + // moderator, nor an administrator — exactly the caller the old guard + // rejected with 403. + it('should allow an authenticated non-author, non-moderator, non-admin to roll back', async () => { + const getRes = await supertest(sails.hooks.http.app) + .get(`/api/v1/guidelines/${nonAuthorGuidelineId}/snapshots`) + .expect(200); + + const targetSnapshot = getRes.body.guidelines.find( + (s) => s.title === nonAuthorTargetTitle + ); + should(targetSnapshot).be.ok(); + + const res = await supertest(sails.hooks.http.app) + .post( + `/api/v1/guidelines/${nonAuthorGuidelineId}/rollback/${targetSnapshot.id}` + ) + .set('Authorization', leaderToken) + .expect(200); + + should(res.body.title).equal(nonAuthorTargetTitle); + + // Confirm the caller really was a non-author, so this test keeps its + // meaning if the fixtures change. + const guideline = await TGuideline.findOne(nonAuthorGuidelineId); + should(guideline.author).not.equal(7); + should(guideline.title).equal(nonAuthorTargetTitle); + }); }); }); From b68d652e4f572ea192b7aa48fdb0e10e4ecf24e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Fri, 4 Sep 2026 14:21:25 -0600 Subject: [PATCH 6/9] test(guideline): name the update cases for current behavior Addresses review feedback on #1798. The two cases were written before the fix, when they were expected to fail, and their names still claimed the endpoint "currently returns" 403/400. Both pass on this branch, so the output described behavior that no longer exists. Renamed to describe what they now assert and dropped the "WILL FAIL on unfixed code" commentary. The comments still record which removed guard each case covers, since that is what makes them regression tests rather than ordinary happy paths. No assertions changed. --- .../4_routes/Guidelines/update.test.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/test/integration/4_routes/Guidelines/update.test.js b/test/integration/4_routes/Guidelines/update.test.js index ae52395a9..879a5f548 100644 --- a/test/integration/4_routes/Guidelines/update.test.js +++ b/test/integration/4_routes/Guidelines/update.test.js @@ -99,13 +99,12 @@ describe('Guideline update', () => { should(snapshots.length).be.greaterThan(1); }); - // Bug condition exploration tests — EXPECTED TO FAIL on unfixed code - // Validates: Requirements 1.2, 1.3 + // Regression cases for the two guards removed in this change (issue #1775). - it('[BUG-2] any authenticated user (non-author, non-moderator, non-admin) should be able to update a guideline (currently returns 403)', (done) => { - // isBugCondition_2: leaderToken is user 7 — authenticated, not the author (user 3), not a moderator, not an admin - // Expected correct behavior: 200. Current (buggy) behavior: 403. - // This test WILL FAIL on unfixed code — that failure documents the bug exists. + it('should allow any authenticated user (non-author, non-moderator, non-admin) to update a guideline', (done) => { + // leaderToken is user 7 — authenticated, but not the author (user 3), + // not a moderator and not an admin: exactly the caller the removed + // author-check rejected with 403. supertest(sails.hooks.http.app) .patch('/api/v1/guidelines/1') .send({ title: 'Updated By Leader' }) @@ -114,11 +113,10 @@ describe('Guideline update', () => { .expect(200, done); }); - it('[BUG-1] PATCH with all geo associations set to empty arrays should return 200 (currently returns 400)', (done) => { - // isBugCondition_1: countries: [], regions: [], massifs: [] — all effective geo collections are empty - // Expected correct behavior: 200 with guideline saved with no geo associations. - // Current (buggy) behavior: 400 "At least one country, region, or massif must be specified." - // This test WILL FAIL on unfixed code — that failure documents the bug exists. + it('should allow PATCH with all geo associations set to empty arrays', (done) => { + // All effective geo collections empty. The removed guard rejected this + // with 400 "At least one country, region, or massif must be specified."; + // a guideline with no geographic scope is now valid and persists as such. supertest(sails.hooks.http.app) .patch('/api/v1/guidelines/1') .send({ countries: [], regions: [], massifs: [] }) From 4c4ab7fcefcd55a732c55d6e3081aba0e9856a68 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Sat, 5 Sep 2026 07:53:58 -0600 Subject: [PATCH 7/9] docs(permissions): record the relaxed guideline write rules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The RBAC documentation merged into develop describes guideline updates as author-or-moderator-or-administrator, which this branch no longer implements. - Update and rollback: no ownership or role check remains, so any authenticated user can edit or roll back any guideline. Drop the "own guidelines" wording, collapse the two matrix rows into one, and remove the claim from the Moderator and Administrator capability lists - Ownership: guidelines are no longer an ownership exception, which leaves comments as the only ownership-scoped update path and means ADMINISTRATOR now overrides ownership nowhere — correct both the Owner-Based Access Control and Ownership-Based Access sections, which named guidelines as its one case - Add the public GET /api/v1/guidelines/:id route to the Visitor list, the matrix, and the policy reference, noting it hides soft-deleted guidelines from every role - Note that PATCH accepts clearing every geographic association, unlike create - Record the resulting write/delete asymmetry as inconsistency 6, flagged as intended rather than tracked in #1796 Claims verified against the guideline route tests. --- PERMISSION_SYSTEM.md | 73 +++++++++++++++++++++++++------------------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/PERMISSION_SYSTEM.md b/PERMISSION_SYSTEM.md index 7ea8c9cc3..67dd3e8a8 100644 --- a/PERMISSION_SYSTEM.md +++ b/PERMISSION_SYSTEM.md @@ -13,7 +13,7 @@ Roles are **not hierarchical**; for instance, an Administrator does not automati - **Permissions**: - View public cave/entrance/document/organisation/massif/person data - Search through cave/entrance/document/organisation/massif/person/device data - - View legislation guidelines (list, by geographic entity, and snapshots) + - View legislation guidelines (list, single guideline by id, by geographic entity, and snapshots) - View the organizations responsible for a country/region/massif - View complete entity history - View statistics @@ -29,9 +29,9 @@ Roles are **not hierarchical**; for instance, an Administrator does not automati - Create legislation guidelines - Edit caves, entrances, documents, massifs, organisations, descriptions, locations, riggings, histories - Edit own comments - - Edit own legislation guidelines - - Rollback own legislation guidelines to a previous version (guidelines are the only entity exposing a rollback - route) + - Edit any legislation guideline, including other users' (no ownership or role check) + - Rollback any legislation guideline to a previous version, including other users' (guidelines are the only entity + exposing a rollback route) - Associate/dissociate responsible organizations for any country, region, or massif - Add/remove own explored entrances - Set main name for entities @@ -76,7 +76,6 @@ Roles are **not hierarchical**; for instance, an Administrator does not automati - Permanently delete author records (`type: AUTHOR`; deleting a `CAVER` account requires Administrator instead — see "Caver Deletion") - Update any user's comments - - Update/rollback any user's legislation guidelines - Unlink documents from caves, entrances, and massifs - Update documents that have modifications pending moderator approval - Validate documents @@ -106,9 +105,9 @@ Roles are **not hierarchical**; for instance, an Administrator does not automati - **System Operations**: - Import data from CSV files (documents and entrances) - System configuration and maintenance - - **Content Moderation** (guidelines only; other content moderation is Moderator-only): - - Update/rollback any user's legislation guidelines - - Delete/restore legislation guidelines + - **Content Moderation** (guideline deletion only; other content moderation is Moderator-only): + - Delete/restore legislation guidelines — the only guideline operation an Administrator gates, since updating and + rolling back them is open to any authenticated user - **Sensitive Data Management**: - View coordinates of sensitive entrances - Remove sensitive flag from entrances @@ -134,7 +133,7 @@ Roles are **not hierarchical**; for instance, an Administrator does not automati | Search content (including devices) | ✅ | ✅ | ✅ | ✅ | ✅ | | View statistics | ✅ | ✅ | ✅ | ✅ | ✅ | | View history/snapshots | ✅ | ✅ | ✅ | ✅ | ✅ | -| View guidelines (list/by-entity/snapshots) | ✅ | ✅ | ✅ | ✅ | ✅ | +| View guidelines (list/by-id/by-entity/snapshots) | ✅ | ✅ | ✅ | ✅ | ✅ | | View responsible organizations of country/region/massif | ✅ | ✅ | ✅ | ✅ | ✅ | | **Content Creation** | | Create caves/entrances/documents/organisations/massifs | ❌ | ✅ | ✅ | ✅ | ✅ | @@ -145,8 +144,7 @@ Roles are **not hierarchical**; for instance, an Administrator does not automati | Update descriptions/locations/riggings/histories | ❌ | ✅ | ✅ | ✅ | ✅ | | Update own comments | ❌ | ✅ | ✅ | ✅ | ✅ | | Update any comment | ❌ | ❌ | ❌ | ✅ | ❌ | -| Update/rollback own guidelines | ❌ | ✅ | ✅ | ✅ | ✅ | -| Update/rollback any guideline | ❌ | ❌ | ❌ | ✅ | ✅ | +| Update/rollback any guideline (ownership not checked) | ❌ | ✅ | ✅ | ✅ | ✅ | | Set main name for entities | ❌ | ✅ | ✅ | ✅ | ✅ | | Move entrance to another cave | ❌ | ✅ | ✅ | ✅ | ✅ | | Reorder descriptions/locations/riggings/histories/comments | ❌ | ✅ | ✅ | ✅ | ✅ | @@ -237,17 +235,22 @@ Roles are **not hierarchical**; for instance, an Administrator does not automati ### Legislation Guidelines Guidelines are legal/regulatory notes attached to one or more geographic entities (country, region, massif). -- **Read**: Fully public — list, by-entity lookup, and snapshots require no authentication +- **Read**: Fully public — list, single-guideline lookup by id, by-entity lookup, and snapshots require no + authentication. The by-id route is gated `['validateId']` only, so a malformed id yields `400` and a missing one `404`. + It returns `404` for soft-deleted guidelines **to every role**, including Moderators: unlike the core-content `find` + controllers, it has no `MODERATOR` branch revealing deleted records, so there is no authenticated way to read one back - **Create**: Any authenticated user; no role check. At least one country, region, or massif must be referenced -- **Update**: Author, Moderator, or Administrator only — unlike most content, a plain user cannot edit another user's - guideline -- **Rollback**: Same rule as update (rollback mutates title/description/language, so it is treated as an update). - Guidelines are currently the only entity exposing a rollback route -- **Soft delete/restore**: Moderator or Administrator only — the author alone cannot delete their own guideline +- **Update**: Any authenticated user; no ownership and no role check — `tokenAuth` is the only gate, so a plain user can + edit another user's guideline, as with most other content +- **Rollback**: Same rule as update — any authenticated user, no ownership or role check. Guidelines are currently the + only entity exposing a rollback route +- **Geographic scope on update**: unlike create, `PATCH` accepts clearing every association, so a guideline can end up + attached to no country, region, or massif. The at-least-one rule is enforced on create only +- **Soft delete/restore**: Moderator or Administrator only — neither the author nor a plain user can delete a guideline - **Permanent delete**: Administrator only, via `?isPermanent=1`. Performed as a two-phase delete that first clears the country/region/massif junction rows and history -- **Asymmetry to note**: authorship grants edit rights but not delete rights, so an author can amend their guideline but - must ask a Moderator or an Administrator to remove it +- **Asymmetry to note**: editing is open to every authenticated user while deletion requires a Moderator or an + Administrator, so any user can rewrite or roll back a guideline they must ask a moderator to remove ### Responsible Organization Associations Countries, regions, and massifs can be linked to the organizations in charge of managing them and their caves. @@ -264,14 +267,15 @@ Countries, regions, and massifs can be linked to the organizations in charge of associations are still returned by reads, flagged with `isDeleted` and a `redirectTo` pointer ### Owner-Based Access Control -- **Content Ownership**: Users can modify any content except other users' comments, other users' guidelines, and - documents that have modifications pending moderator approval (`modifiedDocJson` set) -- **Moderator Override**: Moderators can modify any content regardless of ownership. Administrators override ownership - only on guidelines — updating another user's comment, or a document with pending modifications, checks `MODERATOR` - alone and does not accept `ADMINISTRATOR` as an alternative -- **Comment Updates**: Users can update their own comments; Moderators can update any comments -- **Guideline Updates**: Users can update and roll back only their own guidelines; Moderators and Administrators can - update and roll back any guideline +- **Content Ownership**: Users can modify any content except other users' comments and documents that have modifications + pending moderator approval (`modifiedDocJson` set) +- **Moderator Override**: Moderators can modify any content regardless of ownership. There is no ownership check left for + `ADMINISTRATOR` to override — updating another user's comment, or a document with pending modifications, checks + `MODERATOR` alone and does not accept `ADMINISTRATOR` as an alternative +- **Comment Updates**: Users can update their own comments; Moderators can update any comments. Comments are now the only + entity whose update path is ownership-scoped +- **Guideline Updates**: Any authenticated user can update and roll back any guideline, their own or not — the update and + rollback controllers perform no ownership or role check ### Document Linking Linking and unlinking are **not** gated symmetrically for caves, entrances, and massifs: @@ -330,8 +334,8 @@ cannot detach it either. ### Ownership-Based Access - Users can modify their own content -- Which role overrides ownership is per-entity, not uniform: Moderator overrides it everywhere, while Administrator - overrides it only on guidelines. See "Owner-Based Access Control" for the exceptions +- Only comments and documents pending approval are ownership-scoped on update, and both accept `MODERATOR` alone as the + override — `ADMINISTRATOR` is not accepted anywhere as an ownership override. See "Owner-Based Access Control" - Organization members can manage their organization's explored caves ### Sensitive Data Protection @@ -411,7 +415,8 @@ combined with per-controller role checks that were not applied uniformly — so Items 1, 4 and 5 are tracked in [#1796](https://github.com/GrottoCenter/grottocenter-api/issues/1796). Items 2 and 3 are **accepted behaviour**: in -practice Administrators are granted every group, so they cumulate Moderator powers and are not blocked. +practice Administrators are granted every group, so they cumulate Moderator powers and are not blocked. Item 6 is +**intended behaviour** introduced deliberately, recorded here because it reads as an inconsistency. 1. **Moderators can permanently delete core content.** For caves, entrances, documents, comments, descriptions, locations, riggings, histories, organisations, and massifs, the `isPermanent` branch of the delete controller is @@ -431,6 +436,12 @@ practice Administrators are granted every group, so they cumulate Moderator powe the outlier, accepting either role. Tracked in #1796. 5. **Delete and restore disagree within the same entity.** `device`, `sensor-configuration` and `guideline` all accept either role to *delete*, but only `guideline` accepts either role to *restore*. Tracked in #1796. +6. **Guidelines are writable by anyone but deletable only by moderators.** `guideline/update` and `guideline/rollback` + perform no ownership or role check, so any authenticated user can rewrite or roll back any guideline, while + `guideline/delete` still requires Moderator or Administrator. Rollback is the sharper edge: it replaces the live + title, description, and language from an arbitrary snapshot, giving any user a one-request way to revert another + user's edits. Deliberate — it matches how caves, entrances, and documents are already open to any authenticated user — + but it leaves comments as the only ownership-scoped update path. If any of these are corrected in code, update the matrix rows and the "Soft Deletes" section together. @@ -443,7 +454,7 @@ If any of these are corrected in code, update the matrix rows and the "Soft Dele - `true` - Public access (no authentication required) - `'tokenAuth'` - Requires valid JWT token - `'mfaEnrollmentAuth'` - Requires valid MFA enrollment token -- `['validateId']` - Public access with ID validation +- `['validateId']` - Public access with ID validation (e.g. `v1/guideline/find`, the single-guideline read route) - `['validateId', 'tokenAuth']` - Authenticated with ID validation - `['tokenAuth', 'validateId']` - Same pair in the opposite order, used by the responsible-organization association routes. Policies run in sequence, so the order decides which rejection an unauthenticated request with a malformed ID From 04af044ff4593a67c142132a7e3148ab34dec8d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Mon, 7 Sep 2026 09:52:46 -0600 Subject: [PATCH 8/9] docs(permissions): correct the guideline read-policy description MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on the previous docs commit: two claims about the read path did not match the implementation. - validateId rejects through res.notFound, so a malformed id yields 404, not the 400 documented. A malformed id and a missing one are indistinguishable - "no authenticated way to read a soft-deleted guideline back" was too broad. It holds for the by-id route only: get-snapshots is public, queries h_guideline by t_id without consulting the live row or its isDeleted flag, and the update trigger snapshots the pre-delete title/description/language on soft-delete. Verified by probe — an unauthenticated GET of /guidelines/:id/snapshots returns the deleted guideline's text after the by-id route has started 404ing it. Narrowed to the live row and documented the snapshot exception: soft delete unpublishes, it does not redact - Inconsistency 6's heading said "deletable only by moderators" while its body and the delete controller both accept Administrators too Docs only; no behaviour change and no matrix rows touched. --- PERMISSION_SYSTEM.md | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/PERMISSION_SYSTEM.md b/PERMISSION_SYSTEM.md index 67dd3e8a8..d367f9378 100644 --- a/PERMISSION_SYSTEM.md +++ b/PERMISSION_SYSTEM.md @@ -236,9 +236,14 @@ Roles are **not hierarchical**; for instance, an Administrator does not automati Guidelines are legal/regulatory notes attached to one or more geographic entities (country, region, massif). - **Read**: Fully public — list, single-guideline lookup by id, by-entity lookup, and snapshots require no - authentication. The by-id route is gated `['validateId']` only, so a malformed id yields `400` and a missing one `404`. - It returns `404` for soft-deleted guidelines **to every role**, including Moderators: unlike the core-content `find` - controllers, it has no `MODERATOR` branch revealing deleted records, so there is no authenticated way to read one back + authentication. The by-id route is gated `['validateId']` only, and that policy rejects through `res.notFound`, so a + malformed id and a missing one are indistinguishable — both yield `404`. The route also returns `404` for soft-deleted + guidelines **to every role**, including Moderators: unlike the core-content `find` controllers it has no `MODERATOR` + branch revealing deleted records. That hides the *live row* only, and does not make a deleted guideline's content + unreachable — `get-snapshots` is public and queries `h_guideline` by `t_id` without consulting the live row or its + `isDeleted` flag, while the update trigger snapshots the pre-delete title, description and language on soft-delete. An + unauthenticated caller can therefore still read a soft-deleted guideline's text via + `GET /api/v1/guidelines/:id/snapshots`. Treat soft-deleting a guideline as unpublishing it, not as redacting it - **Create**: Any authenticated user; no role check. At least one country, region, or massif must be referenced - **Update**: Any authenticated user; no ownership and no role check — `tokenAuth` is the only gate, so a plain user can edit another user's guideline, as with most other content @@ -436,12 +441,12 @@ practice Administrators are granted every group, so they cumulate Moderator powe the outlier, accepting either role. Tracked in #1796. 5. **Delete and restore disagree within the same entity.** `device`, `sensor-configuration` and `guideline` all accept either role to *delete*, but only `guideline` accepts either role to *restore*. Tracked in #1796. -6. **Guidelines are writable by anyone but deletable only by moderators.** `guideline/update` and `guideline/rollback` - perform no ownership or role check, so any authenticated user can rewrite or roll back any guideline, while - `guideline/delete` still requires Moderator or Administrator. Rollback is the sharper edge: it replaces the live - title, description, and language from an arbitrary snapshot, giving any user a one-request way to revert another - user's edits. Deliberate — it matches how caves, entrances, and documents are already open to any authenticated user — - but it leaves comments as the only ownership-scoped update path. +6. **Guidelines are writable by anyone but deletable only by Moderators or Administrators.** `guideline/update` and + `guideline/rollback` perform no ownership or role check, so any authenticated user can rewrite or roll back any + guideline, while `guideline/delete` requires Moderator or Administrator. Rollback is the sharper edge: it replaces + the live title, description, and language from an arbitrary snapshot, giving any user a one-request way to revert + another user's edits. Deliberate — it matches how caves, entrances, and documents are already open to any + authenticated user — but it leaves comments as the only ownership-scoped update path. If any of these are corrected in code, update the matrix rows and the "Soft Deletes" section together. From 2887148e7478e62cdb7ddc01dde2cd2c2fe113df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cl=C3=A9ment=20Ronzon?= Date: Mon, 7 Sep 2026 17:32:58 -0600 Subject: [PATCH 9/9] fix(swagger): make nullable reviewer refs valid under OpenAPI 3.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback: GuidelineDetail.reviewer put `nullable: true` beside a $ref. OpenAPI 3.0 ignores every sibling of a Reference Object, so the schema resolved to a bare Caver and rejected `reviewer: null` — which is the normal response, since create never sets a reviewer and the converter passes null through untouched (convertIfObject returns non-objects as-is). Switched to the `nullable` + `allOf` wrapper this file already uses elsewhere (Document.editor, Document.library). Fixed all three occurrences, not just the reported one. A parse of the whole file for $ref-with-siblings found Guideline.reviewer (the schema I copied the pattern from) and SensorConfiguration.reviewer carrying the same bug; both predate this branch. That sweep now reports no remaining violations. Added a detail-endpoint case asserting `reviewer: null` to hold the contract. Verified the fix is load-bearing by validating the real response against both schema forms with OpenAPI nullable semantics applied: the old form rejects it ("must be object"), the new form accepts it. --- assets/swaggerV1.yaml | 9 ++++--- .../4_routes/Guidelines/find.test.js | 26 +++++++++++++++++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/assets/swaggerV1.yaml b/assets/swaggerV1.yaml index 4a50e4b23..753b53ab9 100644 --- a/assets/swaggerV1.yaml +++ b/assets/swaggerV1.yaml @@ -8773,8 +8773,9 @@ components: author: $ref: '#/components/schemas/Caver' reviewer: - $ref: '#/components/schemas/Caver' nullable: true + allOf: + - $ref: '#/components/schemas/Caver' language: $ref: '#/components/schemas/Language' t_id: @@ -8853,8 +8854,9 @@ components: author: $ref: '#/components/schemas/Caver' reviewer: - $ref: '#/components/schemas/Caver' nullable: true + allOf: + - $ref: '#/components/schemas/Caver' language: $ref: '#/components/schemas/Language' @@ -9904,8 +9906,9 @@ components: author: $ref: '#/components/schemas/SimpleCaver' reviewer: - $ref: '#/components/schemas/SimpleCaver' nullable: true + allOf: + - $ref: '#/components/schemas/SimpleCaver' SensorConfigurationCreate: type: object diff --git a/test/integration/4_routes/Guidelines/find.test.js b/test/integration/4_routes/Guidelines/find.test.js index 03581314b..1b426ab62 100644 --- a/test/integration/4_routes/Guidelines/find.test.js +++ b/test/integration/4_routes/Guidelines/find.test.js @@ -11,6 +11,7 @@ describe('Guideline find', () => { // (mirroring rollback.test.js). let guidelineId; let deletedGuidelineId; + let unreviewedGuidelineId; before(async () => { const guideline = await TGuideline.create({ @@ -35,6 +36,17 @@ describe('Guideline find', () => { isDeleted: true, }).fetch(); deletedGuidelineId = deleted.id; + + // No reviewer: this is the state every guideline is created in, since + // `create` never sets one. Kept separate from the row above so the + // `reviewer: 2` assertions there stay meaningful. + const unreviewed = await TGuideline.create({ + title: 'Find Guideline Without Reviewer', + author: 3, + language: 'fra', + dateInscription: new Date(), + }).fetch(); + unreviewedGuidelineId = unreviewed.id; }); describe('GET /api/v1/guidelines/:id', () => { @@ -128,6 +140,20 @@ describe('Guideline find', () => { should(res.body.massifs).be.an.Array().and.be.empty(); }); + // `reviewer` is null on every freshly created guideline (create never sets + // one) and the converter passes null straight through, so the detail + // response really does carry `reviewer: null`. This pins the OpenAPI + // contract: `GuidelineDetail.reviewer` must stay a nullable schema, which + // under OpenAPI 3.0 means `nullable` beside an `allOf` wrapper — a + // `nullable` sibling of `$ref` is ignored and would reject this response. + it('should return reviewer as null for a guideline that has no reviewer', async () => { + const res = await supertest(sails.hooks.http.app) + .get(`/api/v1/guidelines/${unreviewedGuidelineId}`) + .expect(200); + + should(res.body).have.property('reviewer', null); + }); + // Req 2.8: soft-deleted guideline returns 404 it('should return 404 for a soft-deleted guideline', (done) => { supertest(sails.hooks.http.app)