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.

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

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

Open
wants to merge 163 commits into
base: master
Choose a base branch
from

Conversation

erikmd
Copy link
Member

@erikmd erikmd commented Sep 16, 2020

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 Refactor token index #354, so these two PRs Refactor token index #354 and Add support for Moodle/LTI and email/password-based authentication #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 Fix static deployment #356
    (documentation for Fix static deployment #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

agrn and others added 30 commits June 12, 2020 16:09
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
For the association to an existing account to work, the user id given
by the LTI application should be forwarded to the page doing the work.

Let the csrf token be filled too, while we're at it.

The used-id field is not secured, an attacker could associate an
account that doesn't exist yet with a specific token.  An HMAC could
fix this issue.

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
As the front-end cannot access to POST parameters, one reliable way to
pass parameters from the back-end to the front-end is by using
cookies.  The page /launch and /launch/login both set the cookie
`token' if the token is correct, so instead of asking the user for its
token once again, try to retrieve it from the cookies before asking it
to the user.

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
This adds an HMAC to cryptographically check that the user_id has not
been tampered with, which would allow to register a token with an LTI
user that does not exist yet.  The HMAC is generated this way:

    hmac_sha256(secret_key, csrf ^ user_id)

The comparison is performed in constant-time with Eqaf.

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
…ticated

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
This adds a parameter to enable passwords and LTI.  Passwords must be
enabled to use LTI, otherwise `learn-ocaml' will fail.

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
When login in with LTI, the "token" cookie can stay and
re-authenticate the user when clicking on the "logout" button.  Remove
the cookie when disconnecting to avoid that.

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
This adds a new key in the local storage to store this information.
If the login happened by registering with an email/password pair, by
login in with an email/password pair, or with a cookie (indicating a
login through LTI), the underlying token cannot be used to login
directly, so the stored value will be false.  In this case, the token
will not be displayed when login out, and the button will be disabled.

If the user logged in by inputing the token, and that it succedeed,
the token can be displayed to the user.

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
When `use_moodle' is set to true, generate an OAuth token (if needed)
and print it to the standard output.

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
This adds a `is_directory' function to asynchronously check if a
directory exists or not.  `mkdir_p' is modified to use it.

Signed-off-by: Alban Gruin <alban.gruin@univ-tlse3.fr>
@erikmd erikmd force-pushed the oauth-moodle branch 2 times, most recently from 2508b10 to 9ec9f9f Compare May 6, 2021 07:52
@yurug
Copy link
Collaborator

yurug commented Jun 12, 2021

@erikmd What is the status of this PR?

@erikmd erikmd added the kind: feature New user-facing feature. label Jul 23, 2021

let add_token sync_dir token =
get_tokens sync_dir >>= fun tokens ->
if not (List.exists (fun found_token -> found_token = token) tokens) then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not (List.exists (fun found_token -> found_token = token) tokens) then
if not (List.mem token tokens) then

Please see my review at #354 (review) , I believe it still applies here.

@AltGr
Copy link
Collaborator

AltGr commented Jul 28, 2021

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 🤔 ; 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

let rec aux s acc =
Lwt.catch (fun () ->
Lwt_stream.get s >>= function
| Some ("." | ".." | "data") -> aux s acc
Copy link
Collaborator

Choose a reason for hiding this comment

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

A first optim could be to avoid scanning .git directories here, or even .*

erikmd added a commit to pfitaxel/learn-ocaml that referenced this pull request Aug 31, 2021
Regarding optional e-mails, for the sake of consistency, use the same
as that of PR ocaml-sf#362 (branch oauth-moodle-dev).

& Do some minor HTML cleanup as well.
@erikmd erikmd mentioned this pull request Sep 1, 2021
@yurug
Copy link
Collaborator

yurug commented Jan 3, 2022

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

@erikmd
Copy link
Member Author

erikmd commented Jan 7, 2022

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…)

"ssl" {= "0.5.5"}
"digestif" {>= "0.7.1"}
"dune" {= "2.0.1"}
"ezjsonm"
"lwt" {>= "4.0.0"}
"lwt_ssl"
"ocaml" {= "4.05.0"}
"ocamlnet" {> "4.1"}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm isn't ocamlnet a legacy library ? I am not sure it's much maintained anymore.
As far as I can see, it is only used for sending emails, did you consider alternatives ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes! It had been added just because the mirage counterpart of this library needed ocaml > 4.08, and we only had 4.05 support at this time. Thanks for reminding this need for an upgrade.

@AltGr
Copy link
Collaborator

AltGr commented Nov 2, 2023

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

@erikmd
Copy link
Member Author

erikmd commented Nov 3, 2023

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 #508.

Anyway at first sight, I'd say that 1.1 could integrate small features but technically important ones (e.g., #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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: feature New user-facing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants