diff --git a/PERMISSION_SYSTEM.md b/PERMISSION_SYSTEM.md index 7ea8c9cc3..d367f9378 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,27 @@ 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, 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**: 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 +272,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 +339,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 +420,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 +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 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. @@ -443,7 +459,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 diff --git a/api/controllers/v1/guideline/find.js b/api/controllers/v1/guideline/find.js new file mode 100644 index 000000000..e13a607a9 --- /dev/null +++ b/api/controllers/v1/guideline/find.js @@ -0,0 +1,21 @@ +const ControllerService = require('../../../services/ControllerService'); +const GuidelineService = require('../../../services/GuidelineService'); +const { toGuideline } = require('../../../services/mapping/converters'); + +module.exports = async (req, res) => { + const guidelineId = req.param('id'); + const guideline = await GuidelineService.getGuidelineDetail(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, + toGuideline + ); +}; 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/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 e56301ac6..7f3b84aaf 100644 --- a/api/services/mapping/converters.js +++ b/api/services/mapping/converters.js @@ -23,6 +23,18 @@ 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('-'); + return dash > 0 ? regionId.slice(0, dash) : null; +}; + const c = { toCave: (source, meta) => { const result = { @@ -988,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 @@ -1076,4 +1112,4 @@ const c = { }), }; -module.exports = c; +module.exports = { ...c, extractCountryId }; diff --git a/assets/swaggerV1.yaml b/assets/swaggerV1.yaml index 46d93d63c..753b53ab9 100644 --- a/assets/swaggerV1.yaml +++ b/assets/swaggerV1.yaml @@ -3541,10 +3541,40 @@ paths: type: string '/guidelines/{id}': + get: + tags: + - guidelines + 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 + required: true + schema: + type: integer + description: Guideline ID + responses: + '200': + description: Successful operation + content: + application/json: + schema: + $ref: '#/components/schemas/GuidelineDetail' + '404': + description: Guideline not found or soft-deleted. + content: + application/json: + schema: + type: object + properties: + message: + type: string 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: @@ -3606,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: @@ -3784,8 +3812,6 @@ paths: type: string '401': description: Unauthorized (missing or invalid JWT token). - '403': - description: Forbidden (user is not the author, a moderator, or an administrator). '404': description: Guideline or history snapshot not found. content: @@ -8747,14 +8773,93 @@ 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: 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: + nullable: true + allOf: + - $ref: '#/components/schemas/Caver' + language: + $ref: '#/components/schemas/Language' + HealthStatus: type: object properties: @@ -8836,8 +8941,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 @@ -9795,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/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/1_services/GuidelineService.property.test.js b/test/integration/1_services/GuidelineService.property.test.js new file mode 100644 index 000000000..a6c0cc3fd --- /dev/null +++ b/test/integration/1_services/GuidelineService.property.test.js @@ -0,0 +1,62 @@ +/* eslint-disable func-names */ +const should = require('should'); +const fc = require('fast-check'); +const { + extractCountryId, +} = require('../../../api/services/mapping/converters'); + +/** + * 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 } + ); + }); +}); 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/find.test.js b/test/integration/4_routes/Guidelines/find.test.js new file mode 100644 index 000000000..1b426ab62 --- /dev/null +++ b/test/integration/4_routes/Guidelines/find.test.js @@ -0,0 +1,185 @@ +const supertest = require('supertest'); +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; + let unreviewedGuidelineId; + + before(async () => { + 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 = 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', () => { + // 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/${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/${guidelineId}`) + .expect(200); + + const g = res.body; + 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('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('isDeleted', false); + should(g.author).have.property('id', 3); + should(g.reviewer).have.property('id', 2); + }); + + // 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/${guidelineId}`) + .expect(200); + + should(res.body.language).have.property('id', 'fra'); + should(res.body.language).have.property('refName', 'French'); + }); + + // 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/${guidelineId}`) + .expect(200); + + 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', '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(); + }); + + // `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) + .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); + }); + }); + + // 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'); + }); + }); +}); 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); + }); }); }); diff --git a/test/integration/4_routes/Guidelines/update.test.js b/test/integration/4_routes/Guidelines/update.test.js index 268072834..879a5f548 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,31 @@ describe('Guideline update', () => { const snapshots = await HGuideline.find({ t_id: 1 }); should(snapshots.length).be.greaterThan(1); }); + + // Regression cases for the two guards removed in this change (issue #1775). + + 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' }) + .set('Authorization', leaderToken) + .set('Content-type', 'application/json') + .expect(200, done); + }); + + 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: [] }) + .set('Authorization', moderatorToken) + .set('Content-type', 'application/json') + .expect(200, done); + }); }); });