nzoi / nztrain Goto Github PK

NZTrain has a library (as in lending books) feature.

I think this feature lies too far away from NZTrain's original purpose as an automated judge. Most of its functionality can be expressed in a spreadsheet. It also brings in some significant dependencies, in particular wkhtmltopdf and xvfb for the QR code generator (!), which make it harder to set up.

Let's remove it.

NZTrain depends on a geocoder library.

What does it do? Do we still need it?

Background: Our app requires a specific Ruby version. Currently the version is hard-coded in the install scripts (scripts/install/*) and in the obsolete .travis-ci.yml. One issue with this approach is that Dependabot is not aware of that version, so it suggests incompatible updates. Another issue is that it doesn't integrate with the GitHub Action ruby/setup-ruby (#150). I'd like to switch to a better method for specifying the Ruby version.

Here are the methods I am aware of and their tooling support (trickiest constraints in bold):

Comments:

• rbenv and RVM are used in our install scripts and it would be nice to have good integration with them for local development. Currently we don't have good integration (cd into the project doesn't automatically switch to the right Ruby version).
• setup-ruby was proposed in #150 and seems like the best tool for running on GitHub Actions natively, however I think it is more likely that we will use Docker (at least to start with) because GitHub Actions doesn't let us choose the Ubuntu version.
• asdf is a general-purpose language version manager that supports Ruby, Node.js etc. The reason I included it is that .tool-versions originates from asdf, and #150 proposed to use .tool-versions.

Here are the options I can think of:

A. Use Gemfile only.
We won't have rbenv integration, but we've managed so far without it.
We'll get RVM integration but with a warning¹. I think the only way to disable this warning is to modify ~/.rvm/user/rvmrc_ignored, which I'd rather not do.
If we want to use setup-ruby in CI, we can add a step that extracts the version from the Gemfile and writes it to .ruby-version.

B. Use both .ruby-version and Gemfile.
This means the Ruby version will appear in two places and they'll have to be kept in sync. We can add a CI check to detect inconsistencies. Correction: Bundler automatically checks the version specifier in the Gemfile is satisfied, so no need for a custom check.

C. Make .ruby-version the single source of truth, and have the Gemfile automatically look up the version using the line

ruby "#{File.read(File.expand_path(".ruby-version", File.dirname(__FILE__)))}"

I think this option is too clever, and it means Dependabot won't automatically refresh its PRs when we update .ruby-version.

Thoughts?

¹ RVM produces the following warning when you cd into a project and the version is detected using the Gemfile:

RVM used your Gemfile for selecting Ruby, it is all fine - Heroku does that too,
you can ignore these warnings with 'rvm rvmrc warning ignore /.../nztrain/Gemfile'.
To ignore the warning for all files run 'rvm rvmrc warning ignore allGemfiles'."

The server is currently running Linux kernel 4.4.0, which is the default on Ubuntu 16.04.6 server. For #64 (Add C# .NET Core), we need to update to 4.14 or later.

Linux 4.15.0 is available on Ubuntu 16.04.6 through the LTS Enablement (HWE) stack. To install it, we would need to run

sudo apt-get install linux-generic-hwe-16.04

then reboot the server.

If all goes well the server should only be offline for a couple of minutes, but we should be prepared to access the boot menu in case there are problems with the new kernel.

The forums have a message saying they're unused, but folks are obviously still trying to post (5 months ago last attempt) so it might be best to remove the menu item reference to the community, and replace the link (/forum) with a static page indicating where other NZOI communities hang, etc.

Removes a huge number of gems and dependencies from the application, that were never really used aren't aren't being used now

The issue was originally reported by @Techno-coder with a proposed fix in PR #80/#83, but the fix caused a problem and was reverted. We should try to find another fix.

Summary: CI is down. I suggest we switch to GitHub Actions (I'll try to finally review #82). In the meantime, can someone please disable the CI requirement (or give me admin access so I can do it myself)?

Travis CI has shut down travis-ci.org and requires projects to migrate to travis-ci.com (which was originally the paid version for private repositories). I believe the free tier for open source projects (public repositories) has been replaced with a free trial that includes one-off credits for 1000 build minutes, and additional credits need to be manually requested from the Travis CI support team, subject to requirements. (This is despite the migration guide saying that "Travis CI will continue to offer a free tier for public or open-source repositories on travis-ci.com and will not be affected by the migration.")

We would probably meet those requirements, but it sounds like it would be an ongoing hassle. I think we'd be better off switching to another service, for example GitHub Actions. There is already a great open PR (#82) for that; I started reviewing it but never finished, I'll try to finally get that done.

In the meantime, the CI outage together with our current branch protection settings prevent merges into branch 'master'. How about we disable the CI requirement for now?

When a program errors on a test case, the feedback we get is minimal, 2 letters and a number in fact. It would be a lot nicer if we got some words instead.

I checked the evaluator code, and it just runs the evaluator script.

This means that anybody who can add an evaluator might add a malicious script which could do anything.

We should sandbox the evaluator as well. Found something which might be useful.

http://ftp.netbsd.org/pub/NetBSD/packages/pkgsrc/mk/bulk/mksandbox

These should probably be investigated at some point. Accessible to admins under admin>qless

• Use caching for bundle install (https://docs.travis-ci.com/user/caching/#bundler)
• Skip installing languages which are not needed in testing

This is most likely due to swap being enabled – see isolate's manual page. It can be fixed by disabling swap but that seems dangerous as the server is currently running on only 4 GB of physical memory.

The more viable option is to enable the swap cgroup controller. This can be done by setting GRUB_CMDLINE_LINUX_DEFAULT="cgroup_enable=memory swapaccount=1" in /etc/default/grub, then running update-grub and rebooting the server.

Currently the Ruby process runs as user "deploy" (I can't find where we set that user, so I assume it's due to auto detection).
But while deploying recent changes I noticed that APP_USER was originally set to "nztrain" (and also isolock's group was "nztrain" turns out that was recent and unintentional).
The "nztrain" user exists, has a near-empty home dir, and doesn't own any other files.

Does anyone have an opinion on whether we should keep using the "deploy" user or switch to the "nztrain" user?

Personally I think that there might be a small security benefit in switching to a user with read-only access to the app's code, but I don't think it's worth the effort (need to give the "nztrain" user write access to the uploads directory and probably more; it already has database access).

@logan12358 can you please fill out this issue with (a) rationale for the change and (b) progress so far? Thanks!

Right now, email addresses are displayed all over the training site, which would make it an ideal target for spammers.

I propose hiding the email addresses of users and letting them choose usernames instead. Having custom usernames would also open up the possibility of changing one's email address without losing all their progress.

Contest timer always displays NaN on Firefox (and possibly also on Safari). The timer works fine on Chromium.

Software versions:

Firefox 36.0.1
Chromium 41.0.2272.89
Arch Linux 64-bit

Should not require javascript. Basically the right hand side is blank without javascript. Only navigation pane on left is visible. (this might be due to some jquery animation thing)

Note: other things....

Menu needs some nesting anyways. So this may be fixed as part of menu changes. Many more links can be made than would fit on the side pane. However, when it is expanded, I think it would be weird having submenus popup from there, because the side pane is too wide.

So I am thinking that the menu should be moved to the top of the page, as a thin bar across the top.

We can put some rankings/high scores of brownie points of problems solved on the left instead of the menu.

The current selection of languages is getting outdated. It would be useful to support:

• Javascript
• C#
• Newer Java
• Newer GHC (#27)
• Probably more...

To support a new language, we need to:

• Update the scripts to install the language to the chroot environment
• Update the language database
• Update documentation (e.g. the list of NZIC languages)
• Check that things work?

For now, we're blocked on deployment, but it would be good to prioritise the list of languages to add and consider any issues that may arise (e.g. input and output support/guides in Javascript).

Seems counter intuitive to me, especially when super admins can add themselves as admins. Discovered while reviewing PR #125.

Requires changes to app/policies/school_policy.rb perhaps.

Time taken always displays as 0, and placing always displays as 1 when using SQLite.

This is not a problem for the production site because it uses postgreSQL. If anyone developing on SQLite is annoyed by this, they can try fix this.

However, do we want to continue long term support of SQLite as well as postgreSQL?

Site needs unit tests and feature tests.

Security issue from those with access to editing of problem statements. <script> tags work, therefore, vulnerable to cross-site scripting attacks.

Need to sanitize html with loofah

I can't see where it's used and it doesn't appear to be actually working anyway.

remove from config/routes.rb:

  get 'nzic/info/*name/edit', to: 'nzic/info#edit', as: :edit_nzic_info
  namespace :nzic do
    resources :infos, controller: :info, param: :name, only: [:index, :new, :create]
    get 'info/*name', to: 'info#show', as: :info
    patch 'info/*name', to: 'info#update'
    delete 'info/*name', to: 'info#destroy'

    resources :menu_items
  end

drop app/controllers/nzic/
drop app/views/nzic/

remove nzic-models from gemfile
remove config/initialisers/nzic.rb
remove removes to nzic from config/*.yaml

then potentially even remove the repo from the org?

Leaving this here as a note. Feel free to ignore.

I think /users, /users/online, /users/newest (can still be accessed via direct link) should be limited to admin only.

1. Privacy - I think it's a bit strange that any user can see all 2.5k+ users of the website, even if it's mostly harmless info.
2. This might be the most resource-intensive request across the whole site, taking up to 5 seconds to respond (and will increase in future). Might be vulnerable to DDOS attacks?
3. Not much point in having it anyways

To make configuration easier (especially in Docker), I think we should:
1. Allow ERB in all the config/*.yml files (started in #180)
2. Add a dependency on dotenv-rails
3a. Create a skeleton .env file with all the supported environment variables
3b. Replace the *.yml.template files with *.yml files that use ERB to look up the appropriate environment variables, with fallback to sensible defaults
3c. Remove (most of) script/install/config.bash and script/install/nztrain.bash

The visibility of filelinks can be set to "protected", which allows users to view the file if they are not currently competing (see app/models/filelink.rb:17, app/policies/filelink_policy.rb:26).
I don't understand what this is for.

The problem series importer sets the visibility of problem statement attachments to "protected" (see app/workers/problem_series/import_worker.rb:254), but I think the intention there is for the statement to be visible only if the user is currently competing. (Fortunately, this doesn't expose the problem statements before the competition because the controller also checks that the containing problem is accessible, see app/controllers/filelinks/roots_controller.rb:51.)

I thought it might be to do with closed-book exams, but that doesn't make sense either because the policy doesn't check if the contest is closed book or open book.

Does anyone have any insights as to what "protected" is meant for?
If not, I propose that we remove "protected" (and change the problem series importer to use "public").

As suggested by @Holmes98 , to help students who are new to nztrain, we should automatically show input, output, and expected output for failing sample cases on the submission page.

Look to add an auto-generated CRUD admin interface. There are a couple I found with a quick google search:

1. RailsAdmin
2. ActiveAdmin
3. ActiveScafold

and a round up here

With the new interface, there's no blank lines separating the test case results, which makes them a pain to read.

As of this writing, NZTrain uses Rails 4.

C++11 submissions are not being rejudged. Qless has no errors.

eg
https://train.nzoi.org.nz/submissions/47648

The HWE kernel was installed in #73. Now upgrading the system installs both the HWE kernel (4.15.0) and the general availability kernel (4.4.0):

$ apt-get dist-upgrade -s
...
The following NEW packages will be installed:
  linux-headers-4.15.0-88 linux-headers-4.15.0-88-generic linux-headers-4.4.0-174 linux-headers-4.4.0-174-generic linux-image-4.15.0-88-generic linux-image-4.4.0-174-generic linux-modules-4.15.0-88-generic
  linux-modules-4.4.0-174-generic linux-modules-extra-4.15.0-88-generic linux-modules-extra-4.4.0-174-generic
The following packages will be upgraded:
  linux-generic linux-generic-hwe-16.04 linux-headers-generic linux-headers-generic-hwe-16.04 linux-image-generic linux-image-generic-hwe-16.04
6 upgraded, 10 newly installed, 0 to remove and 0 not upgraded.

I propose we remove the general availability kernel (source):

sudo apt-get remove linux-generic linux-headers-generic linux-image-generic

It appears as if readline() is giving a result that is not of type string for the https://train.nzoi.org.nz/problems/24 problem, when running with the new V8 8.1 interpreter.

Reproduction code is listed below, in Python and ECMAScript.

const nums = readline()
  .split(' ')
  .map(Number);

const min = Math.min(...nums);
const max = Math.max(...nums);

const avg = Math.floor(nums.reduce((a, b) => a + b) / 100);

console.log(min, max, avg);

from math import floor
from functools import reduce

nums = list(map(int, input().split()))

minNum = min(nums)
maxNum = max(nums)
avgNum = floor(reduce((lambda a, b: a + b), nums) / 100)

print("{} {} {}".format(minNum, maxNum, avgNum))

The given problem input was used, and both cases work when tested from a command line.

$ python sample.py < input
1 100 52
$ ../d8 sample.js < input
1 100 52

However, when the javascript sample is uploaded .split(' ') fails as the result from readline() is undefined, causing a runtime error.

NZTrain has a forum, but it's largely empty, and the library that provides it has been unmaintained for years.

Even if we need a forum in the future, it'll have little code in common with what's here now.

Let's remove it.

A user has reported that sign up form no longer works (it produces a generic error message).

The following extract from log/production.log shows it is due to an SMTP authentication failure when sending the confirmation email from our @gmail.com account:

Net::SMTPAuthenticationError (535-5.7.8 Username and Password not accepted. Learn more at
):
  app/controllers/accounts/registrations_controller.rb:6:in `create'
  lib/rack/response_timer.rb:12:in `_call'
  lib/rack/response_timer.rb:7:in `call'

(The SMTP response is cut off, but apparently just links to https://support.google.com/mail/?p=BadCredentials which isn't very helpful.)

The problem is that Google recently stopped allowing username-and-password authentication for third-party apps (the "Allow less secure apps" option):

To help keep your account secure, from May 30, 2022, Google no longer supports the use of third-party apps or devices which ask you to sign in to your Google Account using only your username and password.
—https://support.google.com/accounts/answer/6010255

I implemented and deployed a temporary workaround in the branch 'hotfix-email' (commits). The workaround is to write the emails to a file and ask users to contact us so we can manually send them the confirmation link.

To fix this properly we plan to enable 2-step verification for the app's Gmail account, which makes it possible to generate an app-specific password (https://support.google.com/accounts/answer/185833). Authentication using an app-specific password is still supported. This process is currently delayed because the account's recovery details need to be updated, and this can take a week.

Another approach is to switch to OAuth for authentication, but that's complicated.

The server currently runs Ruby 2.2.9, which has issues (see commit f7bfe8b, and a recent pain point in commit 7dce8bf). The 'master' branch uses Ruby 2.3.8 (commit 36814a0), and I suggest we upgrade to that. It has already reached end-of-life, but we can't upgrade to an even newer Ruby version yet because yajl-ruby 1.1.0 breaks on Ruby >= 2.4.0 and would need to be updated first.

NOTE: If no version is specified when installing Bundler it default to Bundler 2, which is a major change and currently breaks in the 'deployed' branch. The github shorthand changed from git:// to https:// in Bundler 2, so the entries in Gemfile.lock that refer to git:// are ignored resulting in version conflicts. We can either wait until 'master' is merged into 'deployed' (for commits 7673380 / dccb9a4), or change git:// to https:// in Gemfile.lock.

# backup current version in case things go wrong
(cd ~ && tar cz .rbenv > rbenv-backup-ruby-2.2.9.tar.gz)
# update rbenv
(cd ~/.rbenv && git pull)
(cd ~/.rbenv/plugins/ruby-build && git pull)
# install new Ruby
rbenv install 2.3.8
# install bundler
RBENV_VERSION=2.3.8 gem install bundler -v $VERSION
# optional: pre-download the gems without touching the real app
mkdir /tmp/fake-app
cp $APP/{Gemfile,Gemfile.lock} /tmp/fake-app
(cd /tmp/fake-app && RBENV_VERSION=2.3.8 bundle install)
rm -r /tmp/fake-app
# install gems
cd $APP
RBENV_VERSION=2.3.8 bundle install
# change the default ruby version
rbenv global 2.3.8
# restart the app
bundle exec passenger-config restart-app /
sudo systemctl restart qless.service
# optional: uninstall old Ruby
rbenv uninstall 2.2.9

As far as I can tell by looking at the code, setting the contest observation to "private" actually has the same effect as "protected".

The field was added in commit fb341af with the comment

for observers: public (everyone), protected (groups it is added to even if not competing), private (only if competing)

The logic was added in commit b298d94, and only checks if the field is set to "public"; otherwise the behaviour matches the description of "protected".

I don't understand how "private" as defined in the original comment is useful. The main impact is that it hides the scoreboard at the end of the contest, which doesn't seem useful.

It almost helps with an issue discussed in chat:

users that start later have a slight advantage in terms of information, since they are able to see which problems/subtasks have been solved by more users

However, to solve this issue "private" would need to only show the scoreboard after the user finished competing (as opposed to during), and I don't think it makes sense to use the observation field for that; we should use a separate field as prototyped in #103.

I suggest we either

1. Combine "private" and "protected" into a single option called "private", with the current behaviour of non-public contests; or
2. Fix the code to match the documentation (i.e. if observation set to private, only show the scoreboard to users who are currently competing in that contest).

Related: Filelinks also have public/protected/private visibility, where "protected" means the user can view the file if not competing. See issue #106 for discussion.

The schema of the production database (on the server) matches the 'deployed' branch, and has a "limit: 128" constraint on the encrypted_password column of the users table. The schema in the 'master' branch (which was regenerated in commit fe025a8 by running the migrations on a clean install) does not have this constraint.

The constraint was added to the "devise" gem in commit heartcombo/devise@27c4280, then later removed in commit heartcombo/devise@ac0105d (with the explanation "No need to limit password"). I suggest we remove this legacy constraint from the production database (and make the schema match the 'master' branch) by running

psql nztrain -c 'ALTER TABLE users ALTER COLUMN encrypted_password TYPE varchar(255)'

(255 is the default length)
I've tried it on a copy of the production database and it worked, but please double check my plan.

Currently partially complete - we need an automatic backup onto a separate server.

I will be working on this.

Based on the comments in the Gemfile, Psych was added because the Ruby version in use was too old and didn't support YAML safe_load.

After #28 is merged, we should investigate whether this is still the case.

Following #125 and as mentioned in #128. All schools now in the database should be NZ schools. However, statements such as School.where(country_code: "NZ") will not work if this field is set to nil or removed from the database.

Suggested tasks:

• Remove country code field from registration form.
• Remove country code from database and create migration.
• Ensure that all mentions of School.where(country_code: "NZ") are replaced across the site.

• update sandbox to isolate
• hstore judging output
• Choice of operators for test set scoring
• Partial scoring

i think if you do a .includes(user: :school) you'll skip an N+1 accessing schools

--@bagedevimo in #142 (comment) (edited)

Need to confirm that we are making extra DB queries (in production mode) on the schools table when displaying / exporting the scoreboard, and that the proposal fixes that.

To repeat. Submit a C++11 or C++14 solution.

The compilation step doesn't appear to be completing and the submission pends indefinitely.

Seems to have been an issue since the 20th.

Sentry has a pretty generous free tier for open source projects so could be worth a first look

Errors

During installation, the geoipupdate command fails with one of the following error messages (depending on its version):

Received an unexpected HTTP status code of 401 from https://updates.maxmind.com/app/update_secure?db_md5=00000000000000000000000000000000&challenge_md5=3a5d812fa6d536c0e18a6271023f5669&user_id=999999&edition_id=GeoLite2-City

error retrieving updates: unexpected HTTP status code: 401 Unauthorized: An account ID and license key are required to use this service.

The failure causes the registration page (/accounts/sign_up) to crash with the following error:

IOError - GeoIP2 - Error opening the specified MaxMind DB file: /usr/share/GeoIP/GeoLite2-City.mmdb:
  app/views/accounts/registrations/new.html.erb:22:in `block in _app_views_accounts_registrations_new_html_erb__...'
  app/views/accounts/registrations/new.html.erb:3:in `_app_views_accounts_registrations_new_html_erb__...'
  lib/rack/response_timer.rb:12:in `_call'
  lib/rack/response_timer.rb:7:in `call'

Cause

The MaxMind GeoLite2 database requires a (free) license key since 30 December 2019 due to issues related to privacy regulations. See https://blog.maxmind.com/2019/12/18/significant-changes-to-accessing-and-using-geolite2-databases/ for details.

Workaround

The sign up page can be fixed by changing

  <%= f.country_select(:country_code, selected: @user.country_code, priority_countries: ["NZ", "AU"]) %></p>

Proposed Solution

I propose removing the GeoIP database altogether. As far as I can tell, its only purpose is to set the default country on the registration page. To me it's not worth the hassle of obtaining a license key (and maintaining the install script, and downloading the packages).

Since almost all our users are from New Zealand, I suggest we simply set the default to NZ. If we don't want non-NZ users to accidentally leave the country as NZ, we can set the default to a placeholder and force users to explicitly select a country (the cost is that the majority of the users will then need two additional clicks to select NZ).

Alternative Solution
Replace with a different database

Related issues
#37 - Investigate geocoder dependency