refs #10461
- the model layer (only post & user) fetches the model after update
- i assume it was added to ensure a response with all fields
- quick fixing it for now to ensure API layer can access ".wasChanged" to be able to decide if a request modified a resource or not
@NOTE: Bookshelf does not physically update a resource if nothing has changed.
no issue
- the event chain works like this:
- if a model registers an event, it get's triggered, because it's stronger than the base model
- but you have to call the base model to agree on a contract, because base model implements generic logic in event handlers
- this was inconsistently used
refs #10388
This updates the base model to retrieve column information, and explicitly set every property whose column is `nullable` and content is the empty string (`""`) to `null`
refs #9865
This updates all current permissible methods to use the new function
signature which includes the hasApiKeyPermissions parameter. It also
makes sure that the hasApiKeyPermissions argument is taken into account
whenever checking before returning a resolved promise.
To be continued. This is just a tiny part of the big picture. None of these changes are fully committed to stay as they are.
* Removed unused fields from v2 Content API
- We want to ship the v2 Content API as clean and lean as we can
- Many fields in the DB aren't actually used, we shouldn't return these values
- Other values aren't useful outside of Admin clients, and shouldn't be returned either
Fields removed:
- tags: created_at, updated_at, parent
- authors: locale, accessibility, tour
- posts: locale, author status, page
refs #10105
- `options.where` is an older deprecated logic
- before the filter language was invented, Ghost generates statements for knex
- if we want to replace GQL with NQL, we can't generate these statements
- they are not understood from NQL, because NQL uses mongo JSON
- go through usages and rewrite the statements
- invent `extraFilters` for now
- we need to keep the support for `status` or `staticPages` for now (API requirement)
- IMO both shortcuts in the extra filters should be removed in the future
This commit is required for https://github.com/TryGhost/Ghost/pull/10159!
* Added updateLastSeen method to user model
refs #10138
* Refactor codebase to use user.updateLastSeen
refs #10138
This is to ensure all updates go via the same method, meaning any
specific logic can be handled in one place, it also helps with grepping
the codebase to find where this occurs
* Created updateUserLastSeen middleware for v2 admin
refs #10138
This is intended to be used with the v2 admin api and _possibly_ the
content api, to give us an accruate report on thelast time a user access
a ghost instance.
* Wired updateUserLastSeen up to v2 Admin API
closes#10138
* Fixed broken test for v2 admin api
no-issue
This test was broken because it was incorrectly testing for a method to
be called exactly once - this was irrelevant to the functionality being
tested for.
* Updated user check method to set status to active
no-issue
* Debounced the updateUserLastSeen middlware an hour
no-issue
* Resolved some PR comments
refs #9866
- prep for v2
- you can better unit test the permissible function
- this avoids copying over the permission handling to v2 controller
- it was possible to move this logic into the model layer, because we now support `unsafeAttrs`
Moved URL attributes logic from the model into API layer
refs #9866
- Moved URL related attribute calculation for posts, users, and tags into API layer
- Added test coverage for url attributes in tags/authors/primary_tags/primary_authors
refs #9866
- Removed `toJSON` call in `findPage`
- Added JSON serialization on API layer
- Reason: model and api layer were coupled - all other model actions just returned the raw data and no specific format
- Corrected test suites to serialize fetched models to JSON
- Removed `absolute_urls` attribute from validOptions findPage methods as it's no longer needed in the data layer
- Changed 'include' test as this option is now tolerated and returns data
closes#9832
The API _should_ be returning absolute URLs for everything, 3rd party applications require absolute urls to read and display ghost data correctly. Currently they have to concat the blog url and the resource url, which is very uncomfortable.
Changing the public api like this would be considered a breaking change however so we've opted to put it behind a query parameter named `absolute_urls`.
no issue
- reported in the forum: https://forum.ghost.org/t/publishing-with-a-single-post-request-to-posts/1648
- the defaults are defined in two places
1. on the schema level (defaults for the database)
2. on the ORM (model layer)
- the defaults on the db layer are set correctly when inserting a new resource
- but if we don't apply all defaults on the model layer, it will happen that model events are emitted without the correct defaults
- see comment in code base
- it's caused by the fact that knex only returns the inserted resource id (probably caused by the fact knex has to support x databases)
- components/modules are listening on model events and expect:
1. a complete set of attributes
2. a complete set of defaults
3. sanitized values e.g. bool, date
- this commit fixes:
1. added missing defaults for user & post model
2. sanitize booleans (0|1 => false|true)
3. added tests to ensure this works as expected
4. clarfies the usage of `defaults`
Regarding https://forum.ghost.org/t/publishing-with-a-single-post-request-to-posts/1648:
- the post event was emitted with the following values {page: undefined, featured: undefined}
- the urlservice receives this event and won't match the resource against collection filters correctly
- NOTE: the post data in the db were correct
no issue
- if multiple queries run in a transaction, the model events are triggered before the txn finished
- if the txn rolls back, the events are anyway emitted
- the events are triggered too early
- solution:
- `emitChange` needs to detect that a transaction is happening
- it listens on a txn event to determine if events should be triggered
closes#9085
Fixes an issue, where the client sets image properties to `""` after deleting the image. This causes problems with the query filter (see https://github.com/TryGhost/GQL/issues/24), as they have to be `null`.
Added a check in the model layer saving method to set value to `null`, when the property is empty.
Affected models and properties:
- `posts`:
- `feature_image`
- `og_image`
- `twitter_image`
- `users`:
- `profile_image`
- `cover_image`
- `tags`:
- `feature_image`
refs https://github.com/TryGhost/Ghost/issues/3658
- the `validateSchema` helper was a bit broken
- if you add a user without email, you will receive a database error
- but the validation error should catch that email is passed with null
- it was broken, because:
- A: it called `toJSON` -> this can remove properties from the output (e.g. password)
- B: we only validated fields, which were part of the JSON data (model.hasOwnProperty)
- we now differentiate between schema validation for update and insert
- fixed one broken import test
- if you import a post without a status, it should not error
- it falls back to the default value
- removed user model `onValidate`
- the user model added a custom implementation of `onValidate`, because of a bug which we experienced (see https://github.com/TryGhost/Ghost/issues/3638)
- with the refactoring this is no longer required - we only validate fields which have changed when updating resources
- also, removed extra safe catch when logging in (no longer needed - unit tested)
- add lot's of unit tests to proof the code change
- always call the base class, except you have a good reason
no issue
- `isNew` does not work in Ghost, because Ghost does not use auto increment id's
- see https://github.com/bookshelf/bookshelf/issues/1265
- see https://github.com/bookshelf/bookshelf/blob/0.10.3/src/base/model.js#L211
- we only had one occurance, which was anyway redundant
- if you add a user, `hasChanged('password') is true
- if you edit a user and the password has changed, `hasChanged('password')` is true as well
NOTE #1:
1. We can't override `isNew` and throw an error, because bookshelf makes use of `isNew` as well, but it's a fallback if `options.method` is not set.
2. It's hard to re-implement `isNew` based on `options.method`, because then we need to ensure that this value is always set (requires a couple of changes)
NOTE #2:
If we need to differentiate if a model is new or edited, we should manually check for `options.method === insert`.
NOTE #3:
The unit tests are much faster compared to the model integration tests.
I did a comparision with the same test assertion:
- unit test takes 70ms
- integration test takes 190ms
no issue
- move password hashing and password comparison to lib/security/password
- added two unit test
- FYI: password hashing takes ~100ms
- we could probably mock password hashing in certain cases when unit testing
no issue
- this commit cleans up the usages of `include` and `withRelated`.
### API layer (`include`)
- as request parameter e.g. `?include=roles,tags`
- as theme API parameter e.g. `{{get .... include="author"}}`
- as internal API access e.g. `api.posts.browse({include: 'author,tags'})`
- the `include` notation is more readable than `withRelated`
- and it allows us to use a different easier format (comma separated list)
- the API utility transforms these more readable properties into model style (or into Ghost style)
### Model access (`withRelated`)
- e.g. `models.Post.findPage({withRelated: ['tags']})`
- driven by bookshelf
---
Commits explained.
* Reorder the usage of `convertOptions`
- 1. validation
- 2. options convertion
- 3. permissions
- the reason is simple, the permission layer access the model layer
- we have to prepare the options before talking to the model layer
- added `convertOptions` where it was missed (not required, but for consistency reasons)
* Use `withRelated` when accessing the model layer and use `include` when accessing the API layer
* Change `convertOptions` API utiliy
- API Usage
- ghost.api(..., {include: 'tags,authors'})
- `include` should only be used when calling the API (either via request or via manual usage)
- `include` is only for readability and easier format
- Ghost (Model Layer Usage)
- models.Post.findOne(..., {withRelated: ['tags', 'authors']})
- should only use `withRelated`
- model layer cannot read 'tags,authors`
- model layer has no idea what `include` means, speaks a different language
- `withRelated` is bookshelf
- internal usage
* include-count plugin: use `withRelated` instead of `include`
- imagine you outsource this plugin to git and publish it to npm
- `include` is an unknown option in bookshelf
* Updated `permittedOptions` in base model
- `include` is no longer a known option
* Remove all occurances of `include` in the model layer
* Extend `filterOptions` base function
- this function should be called as first action
- we clone the unfiltered options
- check if you are using `include` (this is a protection which could help us in the beginning)
- check for permitted and (later on default `withRelated`) options
- the usage is coming in next commit
* Ensure we call `filterOptions` as first action
- use `ghostBookshelf.Model.filterOptions` as first action
- consistent naming pattern for incoming options: `unfilteredOptions`
- re-added allowed options for `toJSON`
- one unsolved architecture problem:
- if you override a function e.g. `edit`
- then you should call `filterOptions` as first action
- the base implementation of e.g. `edit` will call it again
- future improvement
* Removed `findOne` from Invite model
- no longer needed, the base implementation is the same
refs #6103
- simplify `toJSON`
- `baseKey` was not used - have not find a single use case
- all the functionality of our `toJSON` is offered in bookshelf
- `omitPivot` does remove pivot elements from the JSON obj (bookshelf feature)
- `shallow` allows you to not return relations
- make use of `serialize`, see http://bookshelfjs.org/docs/src_base_model.js.html#line260
- fetching nested relations e.g. `users.roles` still works (unrelated to this refactoring)
> pick('shallow', 'baseKey', 'include', 'context')
We will re-add options validation in https://github.com/TryGhost/Ghost/pull/9427, but then with the official way: use `filterOptions`.
---
We return all fetched relations (pre-defined with `withRelated`) by default.
You can disable it with `shallow:true`.
closes#9314
* added fixtures for contributor role
* update post api tests to prevent contributor publishing post
* update permissible function in role/user model
* fix additional author code in invites
* update contributor role migration for knex-migrator v3
* fix paths in contrib migration
* ensure contributors can't edit or delete published posts, fix routing tests [ci skip]
* update db fixtures hash
* strip tags from post if contributor
* cleanup post permissible function
* excludedAttrs to ignore tag updates for now (might be removed later)
* ensure contributors can't edit another's post
* migration script for 1.21
refs #9127
- permission checks can happen everywhere in the code base
- we would like to create a context class
- global access to `options.context.is(...)`
- please read more about the access plugin in #9127 section "Model layer and the access plugin".
- removed the plugin and use direct context checks
refs #9178
- i am not super happy about `const imageLib = require('../lib/image')`
- i don't really like the name `imageLib`
- but i had no better idea 😃
- if we use the same name in the whole project, it's very easy to rename the folder or the variable
refs #9178
- continue with killing our global utils folder
- i haven't found any better naming for lib/promise
- so, require single files for now
- instead of doing `promiseLib = require('../lib/promise')`
- we can optimise the requires later
refs #9178
* Add eslint deps, remove old lint deps
* Add eslint config, remove old lint configs
* Config for server and tests are different
* Tweaked rules to suit us
* Fix linting in codebase - lots of indent changes.
* Fix a real broken test
refs #9150
- Moves the password length fn from `models/user` to `data/validation` where the other validator functions live.
- Added password validation rules. Password rules added:
- Disallow obviously bad passwords: '1234567890', 'qwertyuiop', 'asdfghjkl;' and 'asdfghjklm' for example
- Disallow passwords that contain the words 'password' or 'ghost'
- Disallow passwords that match the user's email address
- Disallow passwords that match the blog domain or blog title
- Disallow passwords that include 50% or more of the same characters: 'aaaaaaaaaa', '1111111111' and 'ababababab' for example.
- Password validation returns an `Object` now, that includes an `isValid` and `message` property to differentiate between the two error messages (password too short or password insecure).
- Use a catch predicate in `api/authentication` on `passwordReset`, so the correct `ValidationError` will be thrown during the password reset flow rather then an `UnauthorizedError`.
- When in setup flow, the blog title is not available yet from `settingsCache`. We therefore supply it from the received form data in the user model `setup` method to have it accessible for the validation.
refs #9150
- move data manipulation for importing users from `importers/data/users` to `model/user` for more consistency (see behaviour of post imports)
- changed importing logic in `onSaving` fn for user model:
- when importing, we set the password to a random uid and don't validate, just hash it and lock the user
- when importing with `importPersistUser` we check if the password is a bcrypt hash already and fall back to normal behaviour if not (set random password, lock user, and hash password)
- don't run validations when importing
no issue
* Comment current state of toJSON for user model
- currently the user model does not return the email if the context is app/external/public OR if there is no context object at all
- i am not 100% sure why if there is no context we should not return the email address
- i think no context means internal access
- maybe change this condition cc @ErisDS
* Extend our access rules plugin
- we already have a instance method to determine which context is used
- this relies on passing options into `.forge` - but we almost never pass the context into the forge call
- added @TODO
- provide another static method to determine the context based on the options object passed from outside
* Use the new static function for existing code
* Add comment where the external context is used
* Remove certain fields from a public request (User model only)
* Tests: support `checkResponse` for a public request
- start with an optional option pattern
- i would love to get rid of checkResponse('user', null, null, null)
- still support old style for now
- a resoure can define the default response fields and public response fields
* Tests: adapt public api test
* Tests: adapt api user test
- use new option pattern for `checkResponse`
- eww null, null, null, null....
* Revert the usage of the access rules plugin
no issue
- the logic here bypasses filtering options!
- that is wrong, because if we filter out certain options e.g. include
- the tests from the previous commit fail because of this
- if we don't fix this logic, the tests won't pass, because as said, you can bypass certain logic e.g. remove roles from include
- this has worked before, because we passed the wrong options via the API layer
- was introduced here 014e2c88dd, because of https://github.com/TryGhost/Ghost/pull/6122
- add proper tests to proof that these queries work!!
refs #8602
- Add the wiring to pass attributes around the permission system
- Allows us to get access to the important "unsafe" attributes that are changing
- E.g. status for posts
- This can then be used to determine whether a user has permission to perform an attribute-based action
- E.g. publish a post (change status)
no issue
- this bug fix affects all endpoints for the public user access
- we allowed fetching `roles` via the public api by accident
- see our docs: https://api.ghost.org/docs/users)
- we only allow `count.posts`
- returning roles via the public api exposes too many details
- this was never intentional
