Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improved password reset and session invalidation for "locked" users #11790

Merged
merged 8 commits into from
May 5, 2020
Merged

Improved password reset and session invalidation for "locked" users #11790

merged 8 commits into from
May 5, 2020

Conversation

naz
Copy link
Member

@naz naz commented May 4, 2020

  • Fixed error message when "locked user tries to login to show specific error instead of generic "Access Denied"
  • Changed copy for password reset message to: "As a security precaution, your password must be reset. Click "Forgot?" to receive an email with instructions."
  • Improved error returned by permission layer for non-active users. Allows Ghost-Admin to redirect them to signin screen instead of being stuck with "Resource Not Found" error

naz added 2 commits May 4, 2020 23:30
@naz
Fixed error message returned by session API
cbf7aa9 
- Instead of returning generic 'access' denied message when error
happens durin `User.check` we want to return more specific error thrown
iside of the method, e.g.: 'accountLocked' or 'accountSuspended'
- Fixed messaging for 'accountLocked' i18n, which not corresponds to the
actual UI available to the end user
@naz
Fixed session invalidation for "locked" user
f51b268 
- Currently Ghost API was returning 404 for users having status set to "locked". This lead the user to be stuck in Ghost-Admin with "Rousource Not Found" error message.
- By returning 401 for non-"active" users it allows for the Ghost-Admin to redirect the user to "signin" screen where they would be instructed to reset their password
@naz
Copy link
Member Author

naz commented May 4, 2020

For reference, old error returned by permission layer for "locked" users was 404 with following content:

{
  "errors": [
    {
      "message": "Resource not found error, cannot list tags.",
      "context": "User not found",
      "type": "NotFoundError",
      "details": null,
      "property": null,
      "help": null,
      "code": null,
      "id": "8b4a7890-8dfc-11ea-99a6-1bef62ddfd48"
    }
  ]
}

A new response is 401:

{
  "errors": [
    {
      "message": "Authorisation error, cannot list tags.",
      "context": "You are not authorised to make this request.",
      "type": "UnauthorizedError",
      "details": null,
      "property": null,
      "help": null,
      "code": null,
      "id": "474a61f0-8e01-11ea-a648-6ba7d586651b"
    }
  ]
}

naz added a commit to naz/Ghost-Admin that referenced this pull request May 5, 2020
@naz
Added special message for "locked" users when they fail signin
ffa42e8 
refs TryGhost/Ghost#11790

- After user fails to signin due to "locked" being locked a new view with automatic email message and more instructions will be shown to make the password reset process easier.
@naz naz mentioned this pull request May 5, 2020
Closed
naz added a commit to TryGhost/Admin that referenced this pull request May 5, 2020
@peterzimon @naz
Added special view for "locked" users when they fail signin (#1561)
d7597e4 
refs TryGhost/Ghost#11790

- After user fails to signin due to "locked" being locked a new view with automatic email message and more instructions will be shown to make the password reset process easier.
- Refined instructions screen
- Added icon + animation
- Refined copy

Co-authored-by: Nazar Gargol <nazargargol@gmail.com>
@kevinansfield
Copy link
Contributor

@naz I think the new error response should be using the same PasswordResetRequiredError type that we discussed previously so that clients can react accordingly

@naz naz requested a review from kevinansfield May 5, 2020 12:05
@naz
Copy link
Member Author

naz commented May 5, 2020

@kevinansfield pushed that already 😉

@naz
Copy link
Member Author

naz commented May 5, 2020

Both client and here. The whole PR is ready for review and merge I think

naz
naz commented May 5, 2020
View reviewed changes
core/server/services/permissions/providers.js
@@ -15,6 +15,10 @@ module.exports = {
}));
}

if (foundUser.get('status') !== 'active') {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensures redirect to /signin for the Ghost-Admin when the user becomes "locked" with an active session.

naz
naz commented May 5, 2020
View reviewed changes
package.json Outdated
@@ -83,7 +83,7 @@
"express-query-boolean": "2.0.0",
"express-session": "1.17.1",
"fs-extra": "9.0.0",
"ghost-ignition": "4.1.0",
"ghost-ignition": "4.2.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduces new UnauthorizedError error to be used with "locked" users

naz
naz commented May 5, 2020
View reviewed changes
core/server/api/canary/session.js
}

if (err.errorType === 'PasswordResetRequiredError') {
await api.authentication.generateResetToken({
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kevinansfield what is your opinion this function call type. It awaits for the response currently but was also thinking to make it "fire-and-forget" style of call. Wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only issue with "fire-and-forget" is that problems with mail config never surface, just appears like emails never arrived. That's typically only a problem for self-hosters though.

@kevinansfield kevinansfield merged commit c84866d into TryGhost:master May 5, 2020
kevinansfield added a commit that referenced this pull request May 6, 2020
@kevinansfield
Removed separate reset/forced-reset emails and updated email copy
0eec876 
refs #11790

- reduced complexity by sticking to one email for both normal reset and forced reset (locked staff accounts)
- exposed `siteTitle` for use in any email templates
- updated email copy to be suitable for both types of password reset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinansfield kevinansfield Awaiting requested review from kevinansfield

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@naz @kevinansfield