This commit fixes a problem where the user will not be able to reset
their password when they only have security keys and backup codes
configured.
This commit also makes the following changes/fixes:
1. Splits password reset system tests to
`spec/system/forgot_password_spec.rb` instead of missing the system
tests in `spec/system/login_spec.rb` which is mainly used to test
the login flow.
2. Fixes a UX issue where the `Use backup codes` or `Use authenticator
app` text is shown on the reset password form when the user does
not have either backup codes or an authenticator app configured.
This makes it more obvious what's happening, and makes it much less likely that users will send repeated reset emails (and thereby hit the rate limit)
Followup to e97ef7e9af
This commit adds the ability for site administrators to mark users'
passwords as expired. Note that this commit does not add any client side
interface to mark a user's password as expired.
The following changes are introduced in this commit:
1. Adds a `user_passwords` table and `UserPassword` model. While the
`user_passwords` table is currently used to only store expired
passwords, it will be used in the future to store a user's current
password as well.
2. Adds a `UserPasswordExpirer.expire_user_password` method which can
be used from the Rails console to mark a user's password as expired.
3. Updates `SessionsController#create` to check that the user's current
password has not been marked as expired after confirming the
password. If the password is determined to be expired based on the
existence of a `UserPassword` record with the `password_expired_at`
column set, we will not log the user in and will display a password
expired notice. A forgot password email is automatically send out to
the user as well.
Previously, if the sso= payload was invalid Base64, but signed correctly, there would be no useful log or error. This commit improves things by:
- moving the base64 check before the signature checking so that it's properly surfaced
- split the ParseError exception into PayloadParseError and SignatureError
- add user-facing errors for both of those
- add/improve spec for both
In a handful of situations, we need to verify a user's 2fa credentials before `current_user` is assigned. For example: login, email_login and change-email confirmation. This commit adds an explicit `target_user:` parameter to the centralized 2fa system so that it can be used for those situations.
For safety and clarity, this new parameter only works for anon. If some user is logged in, and target_user is set to a different user, an exception will be raised.
Why this change?
When running system tests on our CI, we have been occasionally seeing
server errors like:
```
Error encountered while proccessing /stylesheets/desktop_e58cf7f686aab173f9b778797f241913c2833c39.css
NoMethodError: undefined method `+' for nil:NilClass
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/path/pattern.rb:139:in `[]'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:127:in `block (2 levels) in find_routes'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `each'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `each_with_index'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `block in find_routes'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:123:in `map!'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:123:in `find_routes'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:32:in `serve'
/__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:852:in `call'
```
While looking through various Rails issues related to the error above, I
came across https://github.com/rails/rails/pull/27647 which is a fix to
fully initialize routes before the first request is handled. However,
the routes are only fully initialize only if `config.eager_load` is set
to `true`. There is no reason why `config.eager_load` shouldn't be `true` in the
CI environment and this is what a new Rails 7.1 app is generated with.
What does this change do?
Enable `config.eager_load` when `env["CI"]` is present
This is part 2 (of 3) for passkeys support.
This adds a hidden site setting plus routes and controller actions.
1. registering passkeys
Passkeys are registered in a two-step process. First, `create_passkey`
returns details for the browser to create a passkey. This includes
- a challenge
- the relying party ID and Origin
- the user's secure identifier
- the supported algorithms
- the user's existing passkeys (if any)
Then the browser creates a key with this information, and submits it to
the server via `register_passkey`.
2. authenticating passkeys
A similar process happens here as well. First, a challenge is created
and sent to the browser. Then the browser makes a public key credential
and submits it to the server via `passkey_auth_perform`.
3. renaming/deleting passkeys
These routes allow changing the name of a key and deleting it.
4. checking if session is trusted for sensitive actions
Since a passkey is a password replacement, we want to make sure to confirm the user's identity before allowing adding/deleting passkeys. The u/trusted-session GET route returns success if user has confirmed their session (and failed if user hasn't). In the frontend (in the next PR), we're using these routes to show the password confirmation screen.
The `/u/confirm-session` route allows the user to confirm their session with a password. The latter route's functionality already existed in core, under the 2FA flow, but it has been abstracted into its own here so it can be used independently.
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
This commit adds support for an optional `prompt` parameter in the
payload of the /session/sso_provider endpoint. If an SSO Consumer
adds a `prompt=none` parameter to the encoded/signed `sso` payload,
then Discourse will avoid trying to login a not-logged-in user:
* If the user is already logged in, Discourse will immediately
redirect back to the Consumer with the user's credentials in a
signed payload, as usual.
* If the user is not logged in, Discourse will immediately redirect
back to the Consumer with a signed payload bearing the parameter
`failed=true`.
This allows the SSO Consumer to simply test whether or not a user is
logged in, without forcing the user to try to log in. This is useful
when the SSO Consumer allows both anonymous and authenticated access.
(E.g., users that are already logged-in to Discourse can be seamlessly
logged-in to the Consumer site, and anonymous users can remain
anonymous until they explicitly ask to log in.)
This feature is similar to the `prompt=none` functionality in an
OpenID Connect Authentication Request; see
https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
* FEATURE: add a setting to allowlist DiscourseConnect return path domains
This commit adds a site setting to allowlist DiscourseConnect return
path domains. The setting needs supports exact domain or wildcard
character (*) to allow for any domain as return path.
* Add more specs to clarify what is allowed in site setting
* Update setting description to explain what is allowed
This is a very subtle one. Setting the redirect URL is done by passing
a hash through a Discourse event. This is broken on Ruby 2 since the
support for keyword arguments in events was added.
In Ruby 2 the last argument is cast to keyword arguments if it is a
hash. The key point here is that creates a new copy of the hash, so
what the plugin is modifying is not the hash that was passed.
Previously, calling `sign_in` would cause the browser to be redirected to `/`, and would cause the Ember app to boot. We would then call `visit()`, causing the app to boot for a second time.
This commit adds a `redirect=false` option to the `/session/username/become` route. This avoids the unnecessary boot of the app, and leads to significantly faster system spec run times.
In local testing, this takes the full system-spec suite for chat from ~6min to ~4min.
This commit adds some protections in InviteRedeemer to ensure that email
can never be nil, which could cause issues with inviting the invited
person to private topics since there was an incorrect inner join.
If the email is nil and the invite is scoped to an email, we just use
that invite.email unconditionally. If a redeeming_user (an existing
user) is passed in when redeeming an email, we use their email to
override the passed in email. Otherwise we just use the passed in
email. We now raise an error after all this if the email is still nil.
This commit also adds some tests to catch the private topic fix, and
some general improvements and comments around the invite code.
This commit also includes a migration to delete TopicAllowedUser records
for users who were mistakenly added to topics as part of the invite
redemption process.
This security fix affects sites which have `SiteSetting.must_approve_users`
enabled. There are intentional and unintentional cases where invited
users can be auto approved and are deemed to have skipped the staff approval process.
Instead of trying to reason about when auto-approval should happen, we have decided that
enabling the `must_approve_users` setting going forward will just mean that all new users
must be explicitly approved by a staff user in the review queue. The only case where users are auto
approved is when the `auto_approve_email_domains` site setting is used.
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Previously we limited Discourse Connect provider to 1 secret per domain.
This made it pretty awkward to cycle secrets in environments where config
takes time to propagate
This change allows for the same domain to have multiple secrets
Also fixes internal implementation on DiscourseConnectProvider which was
not thread safe as it leaned on class variables to ferry data around
Co-authored-by: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Co-authored-by: David Taylor <david@taylorhq.com>
Discourse has the Discourse Connect Provider protocol that makes it possible to
use a Discourse instance as an identity provider for external sites. As a
natural extension to this protocol, this PR adds a new feature that makes it
possible to use Discourse as a 2FA provider as well as an identity provider.
The rationale for this change is that it's very difficult to implement 2FA
support in a website and if you have multiple websites that need to have 2FA,
it's unrealistic to build and maintain a separate 2FA implementation for each
one. But with this change, you can piggyback on Discourse to take care of all
the 2FA details for you for as many sites as you wish.
To use Discourse as a 2FA provider, you'll need to follow this guide:
https://meta.discourse.org/t/-/32974. It walks you through what you need to
implement on your end/site and how to configure your Discourse instance. Once
you're done, there is only one additional thing you need to do which is to
include `require_2fa=true` in the payload that you send to Discourse.
When Discourse sees `require_2fa=true`, it'll prompt the user to confirm their
2FA using whatever methods they've enabled (TOTP or security keys), and once
they confirm they'll be redirected back to the return URL you've configured and
the payload will contain `confirmed_2fa=true`. If the user has no 2FA methods
enabled however, the payload will not contain `confirmed_2fa`, but it will
contain `no_2fa_methods=true`.
You'll need to be careful to re-run all the security checks and ensure the user
can still access the resource on your site after they return from Discourse.
This is very important because there's nothing that guarantees the user that
will come back from Discourse after they confirm 2FA is the same user that
you've redirected to Discourse.
Internal ticket: t62183.
This PR adds an extra description to the 2FA page when granting a user admin access. It also introduces a general system for adding customized descriptions that can be used by future actions.
(Follow-up to dd6ec65061)
This allows text editors to use correct syntax coloring for the heredoc sections.
Heredoc tag names we use:
languages: SQL, JS, RUBY, LUA, HTML, CSS, SCSS, SH, HBS, XML, YAML/YML, MF, ICS
other: MD, TEXT/TXT, RAW, EMAIL
We validate the *format* of email addresses in many places with a match against
a regex, often with very slightly different syntax.
Adding a separate EmailAddressValidator simplifies the code in a few spots and
feels cleaner.
Deprecated the old location in case someone is using it in a plugin.
No functionality change is in this commit.
Note: the regex used at the moment does not support using address literals, e.g.:
* localpart@[192.168.0.1]
* localpart@[2001:db8::1]
2FA support in Discourse was added and grown gradually over the years: we first
added support for TOTP for logins, then we implemented backup codes, and last
but not least, security keys. 2FA usage was initially limited to logging in,
but it has been expanded and we now require 2FA for risky actions such as
adding a new admin to the site.
As a result of this gradual growth of the 2FA system, technical debt has
accumulated to the point where it has become difficult to require 2FA for more
actions. We now have 5 different 2FA UI implementations and each one has to
support all 3 2FA methods (TOTP, backup codes, and security keys) which makes
it difficult to maintain a consistent UX for these different implementations.
Moreover, there is a lot of repeated logic in the server-side code behind these
5 UI implementations which hinders maintainability even more.
This commit is the first step towards repaying the technical debt: it builds a
system that centralizes as much as possible of the 2FA server-side logic and
UI. The 2 main components of this system are:
1. A dedicated page for 2FA with support for all 3 methods.
2. A reusable server-side class that centralizes the 2FA logic (the
`SecondFactor::AuthManager` class).
From a top-level view, the 2FA flow in this new system looks like this:
1. User initiates an action that requires 2FA;
2. Server is aware that 2FA is required for this action, so it redirects the
user to the 2FA page if the user has a 2FA method, otherwise the action is
performed.
3. User submits the 2FA form on the page;
4. Server validates the 2FA and if it's successful, the action is performed and
the user is redirected to the previous page.
A more technically-detailed explanation/documentation of the new system is
available as a comment at the top of the `lib/second_factor/auth_manager.rb`
file. Please note that the details are not set in stone and will likely change
in the future, so please don't use the system in your plugins yet.
Since this is a new system that needs to be tested, we've decided to migrate
only the 2FA for adding a new admin to the new system at this time (in this
commit). Our plan is to gradually migrate the remaining 2FA implementations to
the new system.
For screenshots of the 2FA page, see PR #15377 on GitHub.
Job arguments go via JSON, and so symbols will appear as strings in the Job's `#execute` method. The latest version of Sidekiq has started warning about this to reduce developer confusion.
When staff visits the user profile of another user, the `email` field
in the model is empty. In this case, staff cannot send the reset email
password because nothing is passed in the `login` field.
This commit changes the behavior for staff users to allow resetting
password by username instead.
The UI used to request a password reset by username when the user was
logged in. This did not work when hide_email_already_taken site setting
was enabled, which disables the lookup-by-username functionality.
This commit also introduces a check to ensure that the parameter is an
email when hide_email_already_taken is enabled as the single allowed
type is email (no usernames are allowed).
* FEATURE: hide_email_address_taken forces use of email in forgot password form
This strengthens this site setting which is meant to be used to harden sites
that are experiencing abuse on forgot password routes.
Previously we would only deny letting people know if forgot password worked on not
New change also bans usage of username for forgot password when enabled
This commit adds token_hash and scopes columns to email_tokens table.
token_hash is a replacement for the token column to avoid storing email
tokens in plaintext as it can pose a security risk. The new scope column
ensures that email tokens cannot be used to perform a different action
than the one intended.
To sum up, this commit:
* Adds token_hash and scope to email_tokens
* Reuses code that schedules critical_user_email
* Refactors EmailToken.confirm and EmailToken.atomic_confirm methods
* Periodically cleans old, unconfirmed or expired email tokens
When the Forever option is selected for suspending a user, the user is suspended for 1000 years. Without customizing the site’s text, this time period is displayed to the user in the suspension email that is sent to the user, and if the user attempts to log back into the site. Telling someone that they have been suspended for 1000 years seems likely to come across as a bad attempt at humour.
This PR special case messages when a user suspended or silenced forever.
This PR allows invitations to be used when the DiscourseConnect SSO is enabled for a site (`enable_discourse_connect`) and local logins are disabled. Previously invites could not be accepted with SSO enabled simply because we did not have the code paths to handle that logic.
The invitation methods that are supported include:
* Inviting people to groups via email address
* Inviting people to topics via email address
* Using invitation links generated by the Invite Users UI in the /my/invited/pending route
The flow works like this:
1. User visits an invite URL
2. The normal invitation validations (redemptions/expiry) happen at that point
3. We store the invite key in a secure session
4. The user clicks "Accept Invitation and Continue" (see below)
5. The user is redirected to /session/sso then to the SSO provider URL then back to /session/sso_login
6. We retrieve the invite based on the invite key in secure session. We revalidate the invitation. We show an error to the user if it is not valid. An additional check here for invites with an email specified is to check the SSO email matches the invite email
7. If the invite is OK we create the user via the normal SSO methods
8. We redeem the invite and activate the user. We clear the invite key in secure session.
9. If the invite had a topic we redirect the user there, otherwise we redirect to /
Note that we decided for SSO-based invites the `must_approve_users` site setting is ignored, because the invite is a form of pre-approval, and because regular non-staff users cannot send out email invites or generally invite to the forum in this case.
Also deletes some group invite checks as per https://github.com/discourse/discourse/pull/12353
The 'Discourse SSO' protocol is being rebranded to DiscourseConnect. This should help to reduce confusion when 'SSO' is used in the generic sense.
This commit aims to:
- Rename `sso_` site settings. DiscourseConnect specific ones are prefixed `discourse_connect_`. Generic settings are prefixed `auth_`
- Add (server-side-only) backwards compatibility for the old setting names, with deprecation notices
- Copy `site_settings` database records to the new names
- Rename relevant translation keys
- Update relevant translations
This commit does **not** aim to:
- Rename any Ruby classes or methods. This might be done in a future commit
- Change any URLs. This would break existing integrations
- Make any changes to the protocol. This would break existing integrations
- Change any functionality. Further normalization across DiscourseConnect and other auth methods will be done separately
The risks are:
- There is no backwards compatibility for site settings on the client-side. Accessing auth-related site settings in Javascript is fairly rare, and an error on the client side would not be security-critical.
- If a plugin is monkey-patching parts of the auth process, changes to locale keys could cause broken error messages. This should also be unlikely. The old site setting names remain functional, so security-related overrides will remain working.
A follow-up commit will be made with a post-deploy migration to delete the old `site_settings` rows.
This moves all the rate limiting for user second factor (based on `params[:second_factor_token]` existing) to the one place, which rate limits by IP and also by username if a user is found.
25563357 moved the logout redirect logic from the client-side to the server-side. Unfortunately the login_required check was lost during the refactoring which meant that non-login-required sites would redirect to `/login` after redirect, and immediately restart the login process. Depending on the SSO implementation, that can make it impossible for users to log out cleanly.
This commit restores the login_required check, and prevents the potential redirect loop.
- Display reason for validation error when logging in via an authenticator
- Fix email validation handling for 'Discourse SSO', and add a spec
Previously, validation errors (e.g. blocked or already-taken emails) would raise a generic error with no useful information.
