learn-ocaml icon indicating copy to clipboard operation
learn-ocaml copied to clipboard

Add support for Moodle/LTI and email/password-based authentication

Open erikmd opened this issue 5 years ago • 9 comments
trafficstars

Description

This development is a joint work with @Aleridia and @agrn that completed their internship in Toulouse this summer.

This PR adds the following features to learn-ocaml:

  • the ability to send e-mails for account confirmation & password change/reset (assuming the new option use_passwd is enabled)
  • the ability to authenticate from a Moodle server (hosted apart, e.g. in one's university) using the so-called LTI protocol, allowing one to sign-up and sign-in very easily (assuming both options use_moodle and use_passwd have been enabled)
  • the ability to "upgrade" a legacy token-based account to a password-based and/or LTI-based account, while keeping the same underlying /sync architecture based on tokens, which are still "unique IDs" identifying the accounts, albeit login for new accounts and upgraded accounts is specifically performed with an e-mail address (or by a LTI request).

Important notes

  • This PR depends on #354, so these two PRs #354 and #362 should be merged in a row.
  • As usual, the *.opam* files (and dune-project) should be bumped (e.g. to 0.13) before the upcoming release, but I did not include this commit in this PR because it would make the PR unmergeable otherwise (due to a conflict with this already-integrated commit).
  • The server API has been modified/extended since 0.12 as needed by these features, but the /sync folders created with 0.12 are fully compatible, given all the new metadata is stored in /sync/data/, and great care has been put into making the migration smooth from 0.12 to (upcoming) 0.13, whatever is the chosen config:
    • use_passwd=false, use_moodle=false (backward-compatible choice)
    • use_passwd=true, use_moodle=false (email/pass auth)
    • use_passwd=true, use_moodle=true (email/pass auth + Moodle/LTI support)

How to test it?

Deploying a complete dev environment to test this PR is very easy using docker-compose. First, create a file demo-repository/server_config.json containing:

{
    "use_passwd": true,
    "use_moodle": true
}

then you should just need to run:

cd .../learn-ocaml
git pr 362  # using this git alias
docker-compose up --build
xdg-open http://localhost:8080  # learn-ocaml application
xdg-open http://localhost:1080  # web-based, ephemeral mail user agent
xdg-open http://localhost:9090  # dockerized Moodle server (login=user, passwd=bitnami)

Anyway, note that if you don't want to rely on these features:

  • The usual ocaml-based deployment is still possible: make && make opaminstall && learn-ocaml --repo=demo-repository
  • likewise for the standard, single-container Docker deployment;
  • and static deployment (e.g. using GitHub Pages, without a backend) is still possible since #356
    (documentation for #356 should be added though)

Status of the PR and checklist

All points that had been raised in the video meeting / review with @yurug, and subsequently in later tests have been addressed and the current version of the code has been successfully deployed (I hope it'll be further tested thanks to ~180 students in the upcoming days :)

Note that the docker-compose.yml file gathered in this PR only brings a "dev / test" environment.

Regarding the "prod" environment, a configuration template (without Moodle, but with a dockerized mail transfer agent) is available in the following repo: https://github.com/pfitaxel/docker-learn-ocaml

A couple of tasks remain though:

  • [ ] Rename/Make environment variables FROM_DOMAIN SMTPSERVER, EMAIL documented in Cmdliner
  • [ ] Add more documentation in the docs folder [mandatory]
  • [ ] Bump cohttp versions to relax a bit the "upward" (<) constraints [optional, advised by opam maintainers]
  • [ ] Investigate the perms warning that appeared here;
  • [ ] Check/fix/update compatibility with the emacs front-end: https://github.com/pfitaxel/learn-ocaml.el

Ideally, I guess that the first task should be performed as part of this PR (I'll have a look ASAP) and the three others could be done likewise, or possibly after the PR merge? just before the 0.13 release anyway… that might occur in October, maybe? Cc @yurug FYI

erikmd avatar Sep 16 '20 00:09 erikmd

Thanks for this PR! I will review it in a couple of days.

yurug avatar Sep 16 '20 06:09 yurug

FYI @yurug, we deployed the oauth-moodle branch this week, but we experienced one major issue: when many people connect (via Moodle or a password), the server seems to become unresponsive…

The issue certainly comes from token_index.ml or from learnocaml_server.ml and we'll investigate further anyway, but unfortunately this is not straightforward to reproduce - as everything works very fine when there is only one person (the tester!) authenticating at the same time.

So for the moment, we've rolled-back to learn-ocaml 0.12 in prod for our students :-/

erikmd avatar Sep 24 '20 15:09 erikmd

Autre remarque sur la résolution de conflit: il y a cette erreur dans le log du CI :

# File "src/server/learnocaml_server.ml", line 1169, characters 48-56:
# Error: Unbound value root_url

erikmd avatar May 05 '21 09:05 erikmd

@erikmd What is the status of this PR?

yurug avatar Jun 12 '21 18:06 yurug

FYI @yurug, we deployed the oauth-moodle branch this week, but we experienced one major issue: when many people connect (via Moodle or a password), the server seems to become unresponsive…

corroborates what I wrote in the review :thinking: ; if the scan indeed took minutes, you probably had all the requests trigger concurrent scans before the first one could finish, which would explain the blowup

AltGr avatar Jul 28 '21 12:07 AltGr

@erikmd What is the status of this PR? Do you need more feedback?

yurug avatar Jan 03 '22 06:01 yurug

Thanks @yurug ! so FYI:

  • the PR based on this branch is not the latest version of the feature (see the branch oauth-moodle-dev)
  • it had become slightly stalled because there was an important evolution that did require OCaml > 4.08,
  • but now that we are compatible with OCaml 4.12, I'll be able to work on it soon 👍
  • so I'll obviously let you know when I need more feedback, so that we can discuss acceptance tests (and I'll refactor the shape of the PR to make it easier to review it, as well as w.r.t. our release-please conventions…)

erikmd avatar Jan 07 '22 02:01 erikmd

I see this has no milestone: I think it's among the top most-wanted features and should be prioritised :) Maybe 1.1 ?

AltGr avatar Nov 02 '23 09:11 AltGr

Hi @AltGr, yes, thanks, this almost-done PR certainly is among the most important ones for teachers' UX.

Regarding the exact milestone, I'm not sure because we should probably discuss the roadmap at the next telco and prioritize w.r.t. the meta-issue https://github.com/ocaml-sf/learn-ocaml/issues/508.

Anyway at first sight, I'd say that 1.1 could integrate small features but technically important ones (e.g., https://github.com/ocaml-sf/learn-ocaml/issues/541 as well as fixes after the 1.0.0 release), and 1.2 could be dedicated to #362.

But we should certainly discuss more

erikmd avatar Nov 03 '23 19:11 erikmd