There are two methods which the server uses to verify an invite is being redeemed with a matching email:
1) The email token, supplied via a `?t=` parameter
2) The validity of the email, as provided by the auth provider
Only one of these needs to be true for the invite to be redeemed successfully on the server. The frontend logic was previously only checking (2). This commit updates the frontend logic to match the server.
This commit does not affect the invite redemption logic. It only affects the 'show' endpoint, and the UI.
We now bundle Javascript for each theme/plugin separately, and only ship bundles for enabled plugins to the client. Therefore, these disabled_plugins checks are now redundant, and can be removed.
This allows plugins to store/modify things in the session (e.g. the destination_url). This change is backwards compatible with existing plugins. If they do not specify a third argument, they will just be passed the first two.
When testing theme components in development, it doesn't make sense to use the `test` environment. The `test` environment almost certainly has 0 themes installed.
This change still works fine when using the `themes:install_and_test` rake task, because that rake task explicitly specifies environment/database-config.
Running a development environment using Docker's qemu architecture emulation is currently not possible because `inotify` is not supported: https://github.com/docker/for-mac/issues/5321
- Add `d/ember-cli`, and publish port 4200
- Remove `d/sidekiq`. Sidekiq is now started with the rails server
- Move all `docker exec` logic into a single place, so we have one place to set environment variable pass-throughs
- Use `exec` for all bash scripts, so that return statuses are passed back correctly
- Avoid using `bin/bash -c` unnecessarily, because it makes escaping arguments difficult
Sometimes plugins directories will end up with other symlinks (e.g. inside node_modules folders). This logic does not work with deeply nested symlinks, and they are unlikely to be necessary for the plugin to work. Therefore we should only look for symlinks in the top-level of the `plugins` directory
- Task name is themes:qunit, not themes:unit
- Some shells try to expand the square brackets. The whole thing should be enclosed in quotes to avoid this
The auto restart logic was sending a USR2 to the parent process without checking what the parent process actually was. In some situations, it might not be the `bin/unicorn` supervisor.
This commit switches to use a global variable for the supervisor PID. This will be much less prone to unexpected behavior.
This commit adds a listener on (almost) all `.rb` files in the repository. When a change occurs, it checks whether Zeitwerk is responsible for autoloading it. If not, a warning will be printed to the console and the server will be automatically restarted. Optionally, you can pass the `AUTO_RESTART=0` environment variable to prevent auto-restart.
Using `bundle exec` will slow down server startup by at least 0.5s. `bin/unicorn` has built-in handling of bundler dependencies, so it is better to launch `bin/rails s` or `bin/unicorn` directly.
In Discourse, `rails s` ultimately launches the `bin/unicorn` script. However, the overhead of `rails` launching `bin/rails`, and then in turn `bin/unicorn` can be non-trivial. Therefore it is much better to run `bin/unicorn` directly.
This commit prints a warning message to STDERR when `rails s` is used. Functionality is unchanged.
This commit makes a few changes to improve boot time in development environments. It will have no effect on production boot times.
- Skip the SchemaCache warmup. In development mode, the SchemaCache is refreshed every time there is a code change, so warmup is of limited use.
- Skip warming up PrettyText. This adds ~2s to each web worker's boot time. The vast majority of requests do not use PrettyText, so it is more efficient to defer its warmup until it's needed
- Skip the intentional 1 second pause during Unicorn worker forking. The comment (which also exists in Unicorn's documentation) suggests this works around a Unix signal handling bug, but I haven't been able to locate any more information. Skipping it in dev will significantly speed up boot. If we start to see issues, we can revert this change.
On my machine, this improves `/bin/unicorn` boot time from >10s to ~4s
This updates the preview_theme_id preservation logic to use more recent, robust, browser APIs. It also adds support for preserving the `?pp=async-flamegraph` parameter which is proposed in https://github.com/MiniProfiler/rack-mini-profiler/pull/494
Sometimes, parts of the application pass in the locale as a string, not a symbol. This was causing the translate_accelerator to cache two versions of the locale separately: one cache for the symbol version, and one cache for the string version. For example, in a running production process:
```
irb(main):001:0> I18n.instance_variable_get(:@loaded_locales)
=> [:en, "en"]
```
This commit ensures the `locale` key is always converted to a symbol, and adds a spec to ensure the same locale cannot appear twice in `@loaded_locales`
In development we regularly restart/reload Rails, which wipes out the schema cache. This then has to be regenerated using DDL queries on the database.
Instead, we can make use of the `rake db:schema:cache:dump` command. This will dump the schema cache to a YAML file, and then load it when needed. This is significantly faster than rebuilding the cache from DDL queries every time.
On every request, Rails checks to see whether any ruby code has been changed on the filesystem. The default FileUpdateChecker does this by iterating over every file on the autoload_paths and comparing its modified-time.
In Discourse, our autoload path of `/app` includes the `/app/assets` directory, and therefore thousands of non-ruby files (e.g. node_modules). This makes the `Dir["/app"]` call very slow (>100ms in my case). On my machine, every Rails-handled request spends around 150-200ms in the FileUpdateChecker. This commit introduces a couple of changes to completely eliminate this wasted time:
- The `/app/assets` directory is excluded from the file watchers. For me, this cut the time spent in the file_watcher to around 50-100ms
- Switches our development config to use the `EventedFileUpdateChecker`, which makes use of the `listen` gem to subscribe to os-specific notifications of changes. This completely removes the `FileUpdateChecker` from the critical path
On my machine, topic_list requests now return in around 80ms (previously >200ms). Live code reload still works as it did before
Plugins always store their stylesheets under `/assets/stylesheets`, so we can make the glob pattern much more specific. In my local development environment, this increases the speed of `Stylesheet::Manager.max_file_mtime` from ~65ms to ~3ms (20x faster). This significantly improves stylesheet regeneration time, and the responsiveness of the theme admin UI.
Note that this will have negligible effect in production, because in production the value of `max_file_mtime` is aggressively cached.
When building the `scss_load_paths`, we were creating a full export of the theme (including uploads), and not cleaning it up. With many uploads, this can be extremely slow (because it downloads every upload from S3), and the lack of cleanup could cause a disk to fill up over time.
This commit updates the ZipExporter to provide a `with_export_dir` API, which takes care of cleanup. It also adds a kwarg which allows exporting only extra_scss fields. This should make things much faster for themes with many uploads.
These endpoints only return one `Theme` row, but the one-many relations were not being preloaded efficiently. This commit moves the `includes` statement to a scope, and makes use of it in `#index`, `#show`, and `#update`.
When navigating from a 'discovery' topic list to a 'tags' topic list, the scroll position from the 'discovery' list was being used by the tag list. That meant the user would be taken to a random point in the list, and not scrolled to the top.
Non-tag topic lists were working fine because we only apply the 'cached' logic (and by extension, the saved scroll location) when the user clicks 'back' in the browser. In the code, this is referred to as `isPoppedState`.
This commit takes the `isPoppedState` logic from the regular topic lists, and applies it to the tag topic lists.
It's important that we don't perform pg_dumps against databases
running behind pgbouncer.
We had an old monkey-patch to prevent this, but following some [recent
internal rails refactoring](https://github.com/rails/rails/commit/5488686851978c64346a0c03d2b6541cf2a94879),
the patch no longer works. Instead, we can use the official
`config.active_record.dump_schema_after_migration` option.
Setting this to false in production is recommended by Rails, and is the
default for newly generated Rails applications.
Previously it would pluck 6 categories which the user had posted in, **then** order them. To select the **top 6** categories, we need to perform the ordering in the SQL query before the LIMIT
Since cd24eff5d9, theme modules are now prefixed with the theme id. Therefore themes which defined raw-view classes will be broken
This commit updates the `raw` helper to use the resolver for looking up the view class. This automatically takes care of trying theme/plugin paths in addition to core paths.
Followup to 5deda5ef3e
The first argument to `Open3.capture3` can be an environment variable hash. In this case, we need to insert the `timeout` command after the env hash.
`GlobalSetting.relative_url_root` comes from the destination site. We
can't be sure whether it was the same on the original site. It's safer
to use a wildcard here, so we can backup/restore sites with different
relative_url_root values.
Previously, if the upload_id was present, but the upload was missing, the entire site would give a server error.
We have no foreign keys on this relation, so we have to be able to cope with the situation where the upload_id is present, but the actual upload has been deleted.
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
`isTesting` is a function, so `if(isTesting)` was only checking for the presence of the function. We need to actually evaluate it. Followup to 68a032a734
For 'local logins', the UX for staged users is designed to be identical to unregistered users. However, staged users logging in via external auth were being automatically unstaged, and skipping the registration/invite flow. In the past this made sense because the registration/invite flows didn't work perfectly with external auth. Now, both registration and invites work well with external auth, so it's best to leave the 'unstage' logic to those endpoints.
This problem was particularly noticeable when using the 'bulk invite' feature to invite users with pre-configured User Fields. In that situation, staged user accounts are used to preserve the user field data.
The logster initializer tries to adds RailsMultisite::Formatter to the STDOUT logger. In production, the lograge initializer then removes the RailsMultisite:Formatter because the JSON log will include the database.
e10a74694a used `Rails.application.reloader.to_prepare` to defer running the 100-logster initializer, which meant it ran **after** 101-lograge. This meant that we were writing JSON logs with a non-json text prefix.
The `to_prepare` was added because our freedom-patches are now deferred using `to_prepare`, and some initializers were relying on the freedom patches. However, following 1533cbb38b, we decided to load the RailsMultisite freedom patch without `to_prepare`. Therefore, `005-site_settings` and `100-logster` no longer need to use `to_prepare`. Removing it means that these initializers are back to running in sequential order, and the logging issue will be resolved.
The only remaining initializer which depends on freedom patches is `100-i18n`. I've added a comment to explain why.
The cluster name can be configured by setting the `DISCOURSE_CLUSTER_NAME` environment variable. If set, you can then call /srv/status with a `?cluster=` parameter. If the cluster does not match, an error will be returned. This is useful if you need a load balancer to be able to verify the identity, as well as the presence, of an application container.
This is not recommended. But if you have other protections in place for CSRF mitigation, you may wish to disable Discourse's implementation. This site setting is not visible in the UI, and must be changed via the console.
Onebox content may only be resolved during the process_post job. Onebox content could change the content of the excerpt, so we need to make sure the excerpt is updated accordingly.
The signup boolean was being passed in the third _autoLogin argument, when it should have been the fourth. The third parameter to _autoLogin was optional, which is confusing. This commit cleans things up so both optional arguments are supplied via keywords.
Followup to cbef2ba151
This allows auth provider plugins to behave differently for login / signup. Previously, there was no way for them to know which button had been used.
This change will be a no-op in the majority of cases. If auth plugins wish to make use of this new feature, they should check for ?signup=true in the URL. For example: https://github.com/discourse/discourse-oauth2-basic/pull/34
The Guardian object memoizes a list of allowed user fields. Normally this is fine because Guardian objects only persist for a single request. However, the WebHook class was memoizing a guardian at the class level. This meant that an app restart was required for changes to be reflected. Plus, the Guardian was being shared across all sites in a multisite instance.
Initializing a guardian is cheap, so we can manage without memoization here.
Previously, the `{{mobile-nav}}` component required a `currentRouteName` property, passed from the router service. It would observe changes in this property, and update the UI accordingly.
If we change between routes which have the same `currentRouteName` (e.g. two different group message inboxes), then the `currentRouteName` does not change and does not trigger the observer. Currently in core, we are relying on the fact that currentRouteName temporarily enters a `.loading` substate during a transition. This will change when we remove the loading substate in the near future.
This commit refactors `{{mobile-nav}}` to inject the router directly, and use the `routeDidChange` event instead of an observer. The change is backwards compatible, but plugins passing the old `currentPath` property will be shown a deprecation notice.
If this test ran before the new SiteSettingsController spec from 8cd7c9b259, it would fail the run. We need to `dup` the array before calling `.clear` on the original
Previously we were checking truthiness in some places, and `== true` in
others. That can lead to some inconsistent UX where the interface says
the email is valid, but account creation fails.
This commit ensures values are boolean when set, and raises an error for
other value types.
If this safety check is triggered, it means the specific auth provider
needs to be updated to pass booleans.
This `if` statement was backwards, such that it was a no-op. This hasn't
caused a problem because clicking an item triggers a page load, which
destroys and recreates the component.
However, we are soon planning to remove the intermediate loading screen,
which means the component will not be removed/recreated.
https://meta.discourse.org/t/177939/202
Still excludes GitHub avatars. Those were the original reason for adding
this broad exclusion. Context at https://meta.discourse.org/t/165713/4
If we find more oneboxes which are unsuitable for thumbnails, we can add
them to this selector.
If no email is provided, email_valid should be set false, so that
Discourse can prompt the user for an email and verify it.
This fixes signups via twitter for accounts with no email address.
Previously we would always take the first image in a post to use as the
thumbnail. On media-heavy sites, users may want to manually select a
specific image as the topic thumbnail. This commit allows this to be
done via a `|thumbnail` attribute in markdown.
For example, in this case, bbb would be chosen as the thumbnail:
```
```
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.
Previously it was using various willTransition and didTransition hooks
which can be quite fragile, especially when removing the
intermediate loading state.
We want to allow developers to customize their own vscode environment.
Including launch.json/tasks.json files in the repository makes this very
difficult.
These were originally added for GitHub codespaces. Once codespaces is
more widely available, we can look into automatically copying the
`.vscode-sample` directory to `.vscode` when the codespace boots.
After this, the only remaining issue preventing Discourse from booting on apple silicon is mini_racer/libv8. See upstream discussion at https://github.com/rubyjs/mini_racer/pull/186 for an experimental solution.
This cookie is only used during login. Having it persist after that can
cause some unusual behavior, especially for sites with short session
lengths.
We were already deleting the cookie following a new signup, but not for
existing users.
This commit moves the cookie deletion logic out of the erb template, and
adds logic and tests to ensure it is always deleted consistently.
Co-authored-by: Jarek Radosz <jradosz@gmail.com>
- Only initialize the S3Helper when needed
- Skip initializing the S3Helper for S3Store#cdn_url
- Allow cook_url to be passed a `local` hint to skip unnecessary checks
* DEV: Remove with_deleted workarounds for old Rails version
These workarounds using private APIs are no longer required in the latest version of Rails. The referenced issue (https://github.com/rails/rails/issues/4306) was closed in 2013. The acts_as_paranoid workaround which this was based on was removed for rails > 5.
Switching to using a scope also allows us to use it within a `belongs_to` relation (e.g. in the Poll model). This avoids issues which can be caused by unscoping all `where` clauses.
Predicates are not necessarily strings, so calling `.join(" AND ")` can sometimes cause weird errors. If we use `WhereClause#ast`, and then `.to_sql` we achieve the same thing with fully public APIs, and it will work successfully for all predicates.
If we clear the in-process cache first, it might get re-filled from the
DB before we clear the DB cache. This would be more likely on high-traffic
sites.
This allows plugins to call `register_demon_process` with a Class inheriting from Demon::Base. The unicorn master process will take care of spawning, monitoring and restarting the process. This API should be used with extreme caution, but it is significantly cleaner than spawning processes/threads in an `after_initialize` block.
This commit also cleans up the demon spawning logging so that it uses the same format as unicorn worker logging. It also switches to the block form of `fork` to ensure that Demons exit after running, rather than returning execution to where the fork took place.
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.
All the data we need for the `info` and `credentials` auth hash
are obtained via the user info API, not the JWT. Using and verifying
the JWT can fail due to clock skew, so let's skip it completely.
PR opened to fix the upstream issue at https://github.com/zquestz/omniauth-google-oauth2/pull/392
When jobs are enqueued inside a transaction, it's possible that they will be executed before the necessary data is available in the database. This commit ensures all jobs are enqueued in an ActiveRecord after_commit hook.
One potential downside here is if the job fails to enqueue, the transaction will no longer be aborted. However, the chance of that happening is reasonably low, and the impact is significantly lower than the current issue where jobs are scheduled before their data is ready.
Previously it matched the behavior of standard ActiveRecord after_commit callbacks. They do not work well within `joinable: false` nested transactions. Now `DB.after_commit` callbacks will only be run when the outermost transaction has been committed.
Tests always run inside transactions, so this also introduces some logic to run callbacks once the test-wrapping transaction is reached.
This ensures that users are only served cached content in their own language. This commit also refactors to make use of the `Discourse.cache` framework rather than direct redis access
User directory items are sorted by some activity metric. If those metrics have the same value, postgres does not guarantee the order in which they will be returned. This can cause issues in pagination - some users may appear twice, and some may be missed. To illustrate
```
pry(main)> query = DirectoryItem.where(period_type: DirectoryItem.period_types[:weekly]).order(:likes_received).limit(50);
pry(main)> page1 = query.offset(0).pluck(:id);
pry(main)> page2 = query.offset(50).pluck(:id);
pry(main)> (page1 & page2).count # users on both pages
=> 29
```
If we use the primary key to tie-break matching metrics, things are much more reliable
```
pry(main)> query = DirectoryItem.where(period_type: DirectoryItem.period_types[:weekly]).order(:likes_received, :id).limit(50);
pry(main)> page1 = query.offset(0).pluck(:id);
pry(main)> page2 = query.offset(50).pluck(:id);
pry(main)> (page1 & page2).count # users on both pages
=> 0
```
This most commonly effects new sites where all the directory metrics are zero.
The fact that the ordering is indeterminate makes it difficult to write a reliable test case for this.
- 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.
At the moment, when filtering by group, the directory will unconditionally return the current user at the top of the list. This is quite unexpected, given that the user is deliberately trying to filter the list. This commit makes sure the 'include current user' logic only triggers for unfiltered directories
Partial revert of b143412be4 (the refactoring is not reverted)
This broke a few things, so we need to investigate and make some changes before reinstating the error
This will only happen if the user_associated_accounts table is currently empty. It's useful for people that may be depending on primary key values in data explorer queries. This change will only have an effect on sites which have not already run this migration.
This reverts commit f31de2fa20.
This is causing `NoMethodError (undefined method `bytesize' for nil:NilClass)` in the anonymous cache. More investigation required
David Taylor
