Skip to content

Commit

Permalink
Enforce naming rules on indexes and collections (#1531)
Browse files Browse the repository at this point in the history
# Description

Enforce strict naming rules on indexes and collections. This has the following effects:

* index and collection creation now return dedicated error messages/ids (`index:create` and `collection:create`)
* it's no longer possible to restrict a profile to an index or a collection that cannot possibly exist ([opened issue: KZL-1392](https://jira.kaliop.net/browse/KZL-1392))

The enforced rules are a stricter version of the [Elasticsearch naming rules](https://www.elastic.co/guide/en/elasticsearch/reference/7.4/indices-create-index.html), as Kuzzle adds a few restrictions of its own.

The documentation has also been updated with these naming rules.

# Architecture

Naming rules have been implemented directly in the ES service, and propagated through the ClientAdapter class. I've made that choice because, to me, naming rules depend on the implemented storage service and therefore might change depending on it.

That way, if we use another storage service, it simply needs to implement the new `isIndexNameValid` and `isCollectionNameValid`.

This choice also explains why we have 1 dedicated function per object type (index & collection) even if this is useless with ES since we emulate collections with indexes. This paves the way for other storage engines allowing different structures and maybe different naming rules for indexes and collections.
  • Loading branch information
scottinet authored Nov 28, 2019
1 parent 6c25a4c commit b6443aa
Show file tree
Hide file tree
Showing 13 changed files with 317 additions and 162 deletions.
12 changes: 8 additions & 4 deletions doc/2/api/controllers/collection/create/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,24 @@ title: create

# create

Creates a new [collection](/core/2/guides/essentials/store-access-data), in the provided `index`.
<SinceBadge version="1.0.0" />

<SinceBadge version="1.3.0" />
Creates a new [collection](/core/2/guides/essentials/store-access-data), in the provided `index`.

You can also provide an optional body with a [collection mapping](/core/2/guides/essentials/database-mappings) allowing you to exploit the full capabilities of our persistent data storage layer.

This method will only update the mapping when the collection already exists.

<SinceBadge version="1.7.1" />

You can define the collection [dynamic mapping policy](/core/2/guides/essentials/database-mappings#dynamic-mapping-policy) by setting the `dynamic` field to the desired value.

You can define [collection additional metadata](/core/2/guides/essentials/database-mappings#collection-metadata) within the `_meta` root field.

Collection names must meet the following criteria:

* Lowercase only
* Cannot include one of the following characters: `\\`, `/`, `*`, `?`, `"`, `<`, `>`, `|`, ` ` (space character), `,`, `#`, `:`, `%`, `&`, `.`
* Cannot be longer than 126 bytes (note it is bytes, so multi-byte characters will count towards the 126 limit faster)

---

## Query Syntax
Expand Down
8 changes: 7 additions & 1 deletion doc/2/api/controllers/index/create/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,13 @@ title: create
# create


Creates a new [index](/core/2/guides/essentials/store-access-data) in Kuzzle.
Creates a new [index](/core/2/guides/essentials/store-access-data) in Kuzzle.

Index names must meet the following criteria:

* Lowercase only
* Cannot include one of the following characters: `\\`, `/`, `*`, `?`, `"`, `<`, `>`, `|`, ` ` (space character), `,`, `#`, `:`, `%`, `&`, `.`
* Cannot be longer than 126 bytes (note it is bytes, so multi-byte characters will count towards the 126 limit faster)

---

Expand Down
3 changes: 2 additions & 1 deletion doc/2/api/essentials/errors/codes/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,9 @@ order: 500
| services.storage.unexpected_error<br/><pre>0x01010023</pre> | [ExternalServiceError](/core/2/api/essentials/errors/handling#externalserviceerror) <pre>(500)</pre> | Embeds an unexpected error from Elasticsearch |
| services.storage.no_mapping_found<br/><pre>0x01010025</pre> | [NotFoundError](/core/2/api/essentials/errors/handling#notfounderror) <pre>(404)</pre> | Attempted to read a non-existent mapping |
| services.storage.index_already_exists<br/><pre>0x01010026</pre> | [PreconditionError](/core/2/api/essentials/errors/handling#preconditionerror) <pre>(412)</pre> | Attempted to create an already existing index |
| services.storage.forbidden_character<br/><pre>0x01010027</pre> | [BadRequestError](/core/2/api/essentials/errors/handling#badrequesterror) <pre>(400)</pre> | An index or a collection name contains a forbidden character |
| services.storage.invalid_search_query<br/><pre>0x01010028</pre> | [BadRequestError](/core/2/api/essentials/errors/handling#badrequesterror) <pre>(400)</pre> | A forbidden argument has been provided in the search query |
| services.storage.invalid_index_name<br/><pre>0x01010029</pre> | [BadRequestError](/core/2/api/essentials/errors/handling#badrequesterror) <pre>(400)</pre> | A provided index name is invalid |
| services.storage.invalid_collection_name<br/><pre>0x0101002a</pre> | [BadRequestError](/core/2/api/essentials/errors/handling#badrequesterror) <pre>(400)</pre> | A provided collection name is invalid |

---

Expand Down
9 changes: 4 additions & 5 deletions lib/api/controllers/collection.js
Original file line number Diff line number Diff line change
Expand Up @@ -347,15 +347,14 @@ class CollectionController extends NativeController {
* @param {Request} request
* @returns {Promise.<Object>}
*/
create (request) {
async create (request) {
const
mappings = this.getBody(request, {}),
{ index, collection } = this.getIndexAndCollection(request);

return this.publicStorage.createCollection(index, collection, mappings)
.then(() => ({
acknowledged: true
}));
await this.publicStorage.createCollection(index, collection, mappings);

return { acknowledged: true };
}

/**
Expand Down
15 changes: 14 additions & 1 deletion lib/config/error-codes/services.json
Original file line number Diff line number Diff line change
Expand Up @@ -218,13 +218,26 @@
"description": "An index or a collection name contains a forbidden character",
"code": 39,
"message": "An index or a collection name cannot contain the character \"%s\"",
"class": "BadRequestError"
"class": "BadRequestError",
"deprecated": true
},
"invalid_search_query": {
"description": "A forbidden argument has been provided in the search query",
"code": 40,
"message": "The argument \"%s\" is not allowed at this level of a search query.",
"class": "BadRequestError"
},
"invalid_index_name": {
"description": "A provided index name is invalid",
"code": 41,
"message": "The index name \"%s\" is invalid",
"class": "BadRequestError"
},
"invalid_collection_name": {
"description": "A provided collection name is invalid",
"code": 42,
"message": "The collection name \"%s\" is invalid",
"class": "BadRequestError"
}
}
},
Expand Down
53 changes: 30 additions & 23 deletions lib/core/models/repositories/profileRepository.js
Original file line number Diff line number Diff line change
Expand Up @@ -239,32 +239,39 @@ class ProfileRepository extends Repository {
* @param {object} [options] - The persistence options
* @returns {Promise<Profile>}
**/
validateAndSaveProfile (profile, options) {
async validateAndSaveProfile (profile, options) {
if (!profile._id) {
return assertionError.reject('missing_argument', 'profileId');
assertionError.throw('missing_argument', 'profileId');
}

const policiesRoles = profile.policies.map(p => p.roleId);
return this.kuzzle.repositories.role.loadRoles(policiesRoles)
.catch(() => errorsManager.throw(
'security',
'profile',
'cannot_hydrate',
profile._id))
.then(() => profile.validateDefinition())
.then(() => {
if ( profile._id === 'anonymous'
&& policiesRoles.indexOf('anonymous') === -1
) {
errorsManager.throw('security', 'profile', 'missing_anonymous_role');
}
this.kuzzle.emit('core:profileRepository:save', {_id: profile._id, policies: profile.policies});
return this.persistToDatabase(profile, options);
})
.then(() => this.loadOneFromDatabase(profile._id))
.then(updatedProfile => {
this.profiles.set(profile._id, updatedProfile);
return updatedProfile;
});

try {
await this.kuzzle.repositories.role.loadRoles(policiesRoles);
}
catch (nil) {
errorsManager.throw('security', 'profile', 'cannot_hydrate', profile._id);
}

await profile.validateDefinition();

if ( profile._id === 'anonymous'
&& policiesRoles.indexOf('anonymous') === -1
) {
errorsManager.throw('security', 'profile', 'missing_anonymous_role');
}

this.kuzzle.emit('core:profileRepository:save', {
_id: profile._id,
policies: profile.policies
});

await this.persistToDatabase(profile, options);

const updatedProfile = await this.loadOneFromDatabase(profile._id);

this.profiles.set(profile._id, updatedProfile);
return updatedProfile;
}

/**
Expand Down
41 changes: 28 additions & 13 deletions lib/core/models/security/profile.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ const
_ = require('lodash'),
Rights = require('./rights'),
Bluebird = require('bluebird'),
errorsManager = require('../../../util/errors');
errorsManager = require('../../../util/errors'),
{ isPlainObject } = require('../../../util/safeObject');

const
_kuzzle = Symbol.for('_kuzzle'),
Expand Down Expand Up @@ -120,7 +121,7 @@ class Profile {

let j = 0;
for (const restriction of policy.restrictedTo) {
if (restriction === null || typeof restriction !== 'object' || Array.isArray(restriction)) {
if (!isPlainObject(restriction)) {
return assertionError.reject(
'invalid_type',
`policies[${i}].restrictedTo[${restriction}]`,
Expand All @@ -133,22 +134,36 @@ class Profile {
`policies[${i}].restrictedTo[${j}].index`);
}

if (typeof restriction.index !== 'string' || restriction.index.length === 0) {
return assertionError.reject(
'invalid_type',
`policies[${i}].restrictedTo[${j}].index`,
'string');
if (!this[_kuzzle].storageEngine.internal.isIndexNameValid(restriction.index)) {
return errorsManager.reject(
'services',
'storage',
'invalid_index_name',
restriction.index);
}

if ( restriction.collections !== undefined
&& restriction.collections !== null
&& ( !Array.isArray(restriction.collections)
|| restriction.collections.some(c => typeof c !== 'string' || c.length === 0))
) {
return assertionError.reject(
'invalid_type',
`policies[${i}].restrictedTo[${j}].collections`,
'string[]');
if (!Array.isArray(restriction.collections)) {
return assertionError.reject(
'invalid_type',
`policies[${i}].restrictedTo[${j}].collections`,
'string[]');
}

const invalid = restriction.collections.find(c => {
return !this[_kuzzle].storageEngine.internal
.isCollectionNameValid(c);
});

if (invalid) {
return errorsManager.reject(
'services',
'storage',
'invalid_collection_name',
invalid);
}
}

for (const member of Object.keys(restriction)) {
Expand Down
6 changes: 4 additions & 2 deletions lib/core/storage/clientAdapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,15 @@ class ClientAdapter {
'indexExists',
'info',
'init',
'isCollectionNameValid',
'isIndexNameValid',
'listAliases',
'listIndexes',
'scroll'
];

// Methods that need to assert index/collection existence and add/remove item
// from the indexCache
// Methods that need to assert index/collection existence and add/remove
// item from the indexCache
this._cacheChangeMethods = [
'createIndex',
'createCollection',
Expand Down
Loading

0 comments on commit b6443aa

Please sign in to comment.