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
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 attentional
no issue
- adds a ghost-backup client
- adds a client authenticated endpoint to export blog for ghost-backup client only
- allows some additional overrides during import
- allows for an import by file to override locking a user and double hashing the password
closes#8781
- when the ownership get's transferred, the id of the new owner is not '1' anymore
- we previously added a database rule, which signalises if the blog is setup or not, see 827aa15757 (diff-7a2fe80302d7d6bf67f97cdccef1f71fR542)
- this database rule is based on the owner id being '1', which is wrong when you transfer ownership
- we should keep in mind, that the owner id being '1' is only the default Ghost setup, but it can change
- blog is setup if the owner is locked
no issue
- if you upload a huge import file, parallel operations can throw errors e.g. lock wait exceeds
- this can happen if multiple transactions run in parallel
- there is no need to run:
1. the removal of active tokens on import, because imported users have no active session
2. rescheduling logic on timezone, because importing scheduled posts works out of the box via the model layer (if a published date is detected and it's in the future, the post get's scheduled)
closes#8645, closes#8710
- locked users were once part of the category "active users", but were moved to the inactive category
-> we have added a protection of not being able to edit yourself when you are either suspended or locked
- but they are not really active users, they are restricted, because they have no access to the admin panel
- support three categories: active, inactive, restricted
* - revert restricted states
- instead, update permission layer: fallback to `all` by default, because you are able to serve any user status
- add more tests
- ATTENTION: there is a behaviour change, that a blog owner's author page can be served before setting up the blog, see conversation on slack
-> LTS serves 404
-> 1.0 would serve 200
no issue
- if you destroy a user with an unknown user id, Ghost would crash
- because `userModel.hasRole` is undefined
- there is actually a bigger underlying architectual problem:
- the permission check should rely on an existing user
- so there should be a first api layer, which 1. validates (this code exists) and 2. ensures that requested database id's exist
- but this requires a bigger refactoring
refs #8620
Adds a new Ghost Author user, which is the author of the new welcome blog posts. The user is set to active, so the author slug works (otherwise it would render a 404, when user is suspended). Furthermore, there's one little fix in the user model, which was checking only for `active` user to decide the signup or setup process for the UI. Adding one more conditional to check if the found active user is also the owner, prevents to get redirected to sign in.
closes#8565
- isPasswordCorrect fn returns a specific error, which we simply forward
- no need to wrap a custom error into a new custom error
- the rule is always: if you are using a Ghost unit/function, you can expect that this unit returns a custom error
refs #5422
- we can support null titles after this PR if we want
- user model: fix getAuthorRole
- user model: support adding roles by name
- we support this for roles as well, this makes it easier when importing related user roles (because usually roles already exists in the database and the related id's are wrong e.g. roles_users)
- base model: support for null created_at or updated_at values
- post or tag slugs are always safe strings
- enable an import of a null slug, no need to crash or to cover this on import layer
- add new DataImporter logic
- uses a class inheritance mechanism to achieve an easier readability and maintenance
- schema validation (happens on model layer) was ignored
- allow to import unknown user id's (see https://github.com/TryGhost/Ghost/issues/8365)
- most of the duplication handling happens on model layer (we can use the power of unique fields and errors from the database)
- the import is splitted into three steps:
- beforeImport
--> prepares the data to import, sorts out relations (roles, tags), detects fields (for LTS)
- doImport
--> does the actual import
- afterImport
--> updates the data after successful import e.g. update all user reference fields e.g. published_by (compares the imported data with the current state of the database)
- import images: markdown can be null
- show error message when json handler can't parse file
- do not request gravatar if email is null
- return problems/warnings after successful import
- optimise warnings in importer
- do not return warnings for role duplications, no helpful information
- error handler: return context information of error
- we show the affected json entries as one line in the UI
- show warning for: detected duplicated tag
- schema validation: fix valueMustBeBoolean translation
- remove context property from json parse error
* 🙀 change database schema for images
- rename user/post/tag images
- contains all the required changes from the schema change
* Refactor helper/meta data
- rename cover to cover_image
- also rename default settings to match the pattern
- rename image to profile_image for user
- rename image to feature_image for tags/posts
* {{image}} >>> {{img_url}}
- rename
- change the functionality
- attr is required
- e.g. {{img_url feature_image}}
* gscan 1.0.0
- update yarn.lock
* Update casper reference: 1.0-changes
- see 5487b4da8d
refs #8258
* 🎨 change last_login to last_seen
- rename the column
- a change in Ghost-Admin is required as well
* test utils: revert export examples
* revert line breaks
refs #8111
- Ghost returns now all (active+none active) users by default
- protect login with suspended status
- test permissions and add extra protection for suspending myself
- if a user is suspended and tries to activate himself, he won't be able to proceed the login to get a new token
* 🎨 deny auto switch
no issue
- deny auth switch after the blog was setup
- setup completed depends on the status of the user right now, see comments
* Updates from comments
- re-use statuses in user model
- update error message
* ✨ ghost auth: sync email
refs #7452
- sync email changes in background (every hour right now)
- sync logged in user only!
- no sync if auth strategy password is used
- GET /users/me is triggered on every page refresh
- added TODO to support or add long polling for syncing data later
- no tests yet on purpose, as i would like to get a basic review first
* 🐩 use events
- remember sync per user
closes#7256
- original code changes made by @golya in https://github.com/TryGhost/Ghost/pull/7304
- refactored edit method in user model to validate an existing email address
- added test coverage for existing email update in user model spec
* 🛠 bookshelf tarball, bson-objectid
* 🎨 schema changes
- change increment type to string
- add a default fallback for string length 191 (to avoid adding this logic to every single column which uses an ID)
- remove uuid, because ID now represents a global resource identifier
- keep uuid for post, because we are using this as preview id
- keep uuid for clients for now - we are using this param for Ghost-Auth
* ✨ base model: generate ObjectId on creating event
- each new resource get's a auto generate ObjectId
- this logic won't work for attached models, this commit comes later
* 🎨 centralised attach method
When attaching models there are two things important two know
1. To be able to attach an ObjectId, we need to register the `onCreating` event the fetched model!This is caused by the Bookshelf design in general. On this target model we are attaching the new model.
2. We need to manually fetch the target model, because Bookshelf has a weird behaviour (which is known as a bug, see see https://github.com/tgriesser/bookshelf/issues/629). The most important property when attaching a model is `parentFk`, which is the foreign key. This can be null when fetching the model with the option `withRelated`. To ensure quality and consistency, the custom attach wrapper always fetches the target model manual. By fetching the target model (again) is a little performance decrease, but it also has advantages: we can register the event, and directly unregister the event again. So very clean code.
Important: please only use the custom attach wrapper in the future.
* 🎨 token model had overriden the onCreating function because of the created_at field
- we need to ensure that the base onCreating hook get's triggered for ALL models
- if not, they don't get an ObjectId assigned
- in this case: be smart and check if the target model has a created_at field
* 🎨 we don't have a uuid field anymore, remove the usages
- no default uuid creation in models
- i am pretty sure we have some more definitions in our tests (for example in the export json files), but that is too much work to delete them all
* 🎨 do not parse ID to Number
- we had various occurances of parsing all ID's to numbers
- we don't need this behaviour anymore
- ID is string
- i will adapt the ID validation in the next commit
* 🎨 change ID regex for validation
- we only allow: ID as ObjectId, ID as 1 and ID as me
- we need to keep ID 1, because our whole software relies on ID 1 (permissions etc)
* 🎨 owner fixture
- roles: [4] does not work anymore
- 4 means -> static id 4
- this worked in an auto increment system (not even in a system with distributed writes)
- with ObjectId we generate each ID automatically (for static and dynamic resources)
- it is possible to define all id's for static resources still, but that means we need to know which ID is already used and for consistency we have to define ObjectId's for these static resources
- so no static id's anymore, except of: id 1 for owner and id 0 for external usage (because this is required from our permission system)
- NOTE: please read through the comment in the user model
* 🎨 tests: DataGenerator and test utils
First of all: we need to ensure using ObjectId's in the tests. When don't, we can't ensure that ObjectId's work properly.
This commit brings lot's of dynamic into all the static defined id's.
In one of the next commits, i will adapt all the tests.
* 🚨 remove counter in Notification API
- no need to add a counter
- we simply generate ObjectId's (they are auto incremental as well)
- our id validator does only allow ObjectId as id,1 and me
* 🎨 extend contextUser in Base Model
- remove isNumber check, because id's are no longer numbers, except of id 0/1
- use existing isExternalUser
- support id 0/1 as string or number
* ✨ Ghost Owner has id 1
- ensure we define this id in the fixtures.json
- doesn't matter if number or string
* 🎨 functional tests adaptions
- use dynamic id's
* 🎨 fix unit tests
* 🎨 integration tests adaptions
* 🎨 change importer utils
- all our export examples (test/fixtures/exports) contain id's as numbers
- fact: but we ignore them anyway when inserting into the database, see https://github.com/TryGhost/Ghost/blob/master/core/server/data/import/utils.js#L249
- in 0e6ed957cd (diff-70f514a06347c048648be464819503c4L67) i removed parsing id's to integers
- i realised that this ^ check just existed, because the userIdToMap was an object key and object keys are always strings!
- i think this logic is a little bit complicated, but i don't want to refactor this now
- this commit ensures when trying to find the user, the id comparison works again
- i've added more documentation to understand this logic ;)
- plus i renamed an attribute to improve readability
* 🎨 Data-Generator: add more defaults to createUser
- if i use the function DataGenerator.forKnex.createUser i would like to get a full set of defaults
* 🎨 test utils: change/extend function set for functional tests
- functional tests work a bit different
- they boot Ghost and seed the database
- some functional tests have mis-used the test setup
- the test setup needs two sections: integration/unit and functional tests
- any functional test is allowed to either add more data or change data in the existing Ghost db
- but what it should not do is: add test fixtures like roles or users from our DataGenerator and cross fingers it will work
- this commit adds a clean method for functional tests to add extra users
* 🎨 functional tests adaptions
- use last commit to insert users for functional tests clean
- tidy up usage of testUtils.setup or testUtils.doAuth
* 🐛 test utils: reset database before init
- ensure we don't have any left data from other tests in the database when starting ghost
* 🐛 fix test (unrelated to this PR)
- fixes a random failure
- return statement was missing
* 🎨 make changes for invites
refs #7494, refs #7495
This PR is an extracted clean up feature of #7495.
We are using everywhere static id checks (userId === 0 or userId === 1).
This PR moves the static values into the Base model.
This makes it 1. way more readable and 2. we can change the id's in a central place.
I changed the most important occurrences - no tests are touched (yet!).
The background is: when changing from auto increment id (number) to ObjectId's (string) we still need to support id 1 and 0, because Ghost relies on these two static id's.
I would like to support using both: 0/1 as string and 0/1 as number.
1 === owner/internal
0 === external
Another important change:
User Model does not longer define the contextUser method, because i couldn't find a reason?
I looked in Git history, see 6e48275160
no issue
- removes count from user checks model
- uses brute express brute with brute-knex adaptor to store persisted data on spam prevention
- implement brute force protection for password/token exchange, password resets and private blogging
* 🔥 remove User model functions
- validateToken
- generateToken
- resetPassword
- all this logic will re-appear in a different way
Token logic:
- was already extracted as separate PR, see https://github.com/TryGhost/Ghost/pull/7554
- we will use this logic in the controller, you will see in the next commits
Reset Password:
Was just a wrapper for calling the token logic and change the password.
We can reconsider keeping the function to call: changePassword and activate the status of the user - but i think it's fine to trigger these two actions from the controlling unit.
* 🔥 remove password reset tests from User model
- we already have unit tests for change password and the token logic
- i will re-check at the end if any test case is missing - but for now i will just burn the tests
* ✨ add token logic to controlling unit
generateResetToken endpoint
- the only change here is instead of calling the User model to generate a token, we generate the token via utils
- we fetch the user by email, and generate a hash and return
resetPassword endpoint
- here we have changed a little bit more
- first of all: we have added the validation check if the new passwords match
- a new helper method to extract the token informations
- the brute force security check, which can be handled later from the new bruteforce middleware (see TODO)
- the actual reset function is doing the steps: load me the user, compare the token, change the password and activate the user
- we can think of wrapping these steps into a User model function
- i was not sure about it, because it is actually part of the controlling unit
[ci skip]
* 🎨 tidy up
- jscs
- jshint
- naming functions
- fixes
* ✨ add a test for resetting the password
- there was none
- added a test to reset the password
* 🎨 add more token tests
- ensure quality
- ensure logic we had
* 🔥 remove compare new password check from User Model
- this part of controlling unit
* ✨ compare new passwords for user endpoint
- we deleted the logic in User Model
- we are adding the logic to controlling unit
* 🐛 spam prevention forgotten can crash
- no validation happend before this middleware
- it just assumes that the root key is present
- when we work on our API, we need to ensure that
1. pre validation happens
2. we call middlewares
3. ...
* 🎨 token translation key
* 🎨 do not call generateSlug twice for User.setup
* 🎨 call generatePasswordHash onSaving only
- now we can add defaults to User Model
- it was not possible before because add User model did the following:
1. validate password length
2. hash password manually
3. call ghostBookshelf.Model.add and THEN bookshelf defaults fn gets triggered
- call generatePasswordHash in onSaving hook for all use case
- add more tests to user model, juhu
refs #7432
- all models implemented it's own initialize fn to register events
- we can register all events in the base model
- important: we only listen on the event, if the model has defined a hook for it
- this is just a small clean up PR
- register more bookshelf events
