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

Edit json in browser #1344

Merged
merged 102 commits into from
Jul 5, 2019
Merged

Edit json in browser #1344

merged 102 commits into from
Jul 5, 2019

Conversation

tbretl
Copy link
Member

@tbretl tbretl commented Oct 21, 2018

This PR adds an "Edit JSON" button to the course overview page (both for courses and course instances) that allows instructors to edit, push, and sync *.json files in the browser. Drafts are saved to S3 (or to a temporary directory if running locally).

In order to enable git push, I created a new team machine-write with root as a member (analogous to machine-read) in the PrairieLearn organization. This team would have to be given write access to all course repositories. Currently, this team only has access to the pl-ae353 repository, which I used for testing.

It is now easy to add an "Edit JSON" button that allows in-browser editing of all other course json files (e.g., question info.json files and assessment infoAssessment.json files) - this is something I plan to do before merging. (I may also go ahead and allow in-browser editing of non-json files.)

(Note the corresponding PR to the configuration of the production server - for in-browser editing to work in production, this change is also necessary.)

@nwalters512
Copy link
Contributor

Finally taking a final look at this 🎉 I'll be commenting as I go.

First thing: protecting against editing arbitrary paths. As an example, attempting to edit the PrairieLearn package.json file via a URL like this one: http://localhost:3000/pl/course_instance/4/instructor/question/305/edit?file=../PrairieLearn/package.json. Currently that fails with the following error message:

fatal: '../PrairieLearn/package.json' is outside repository

This is a byproduct of attempting to get the current commit hash before editing, which fails and prevents me from being able to see or edit files outside the repo. However, this feels possibly more like a happy accident than a deliberate prevention of editing unauthorized files. To be very explicit with this, we could do a [path.normalize] on the desired path and then check that said path begins with the course directory and fail if it doesn't. I'd also like to see explicit tests for this, as a regression here would be really bad.

@nwalters512
Copy link
Contributor

Next: we should pick a reasonable way for this to work in local dev. Currently, trying to edit a question info.json fails during the "save and sync" process during "Stage 6: Commit changes" with the following error:

*** Please tell me who you are.

Run

  git config --global user.email "you@example.com"
  git config --global user.name "Your Name"

to set your account's default identity.
Omit --global to set the identity only in this repository.

fatal: unable to auto-detect email address (got 'root@e548214d3135.(none)')

If we want this to work locally, we should fix it. If not, we should disable the buttons in dev mode. I'd prefer the former, but we should address this one way or another.

pages/partials/instructorAssessmentTabs.ejs Outdated Show resolved Hide resolved
server.js Outdated Show resolved Hide resolved
@mwest1066 mwest1066 merged commit 3cebe3d into master Jul 5, 2019
@mwest1066 mwest1066 deleted the edit_json_in_browser branch July 5, 2019 17:16
mwest1066 added a commit that referenced this pull request Sep 12, 2019
This fixes a bug introduced in PR #1344 (commit 3cebe3d), which
changed the admin URL routes but did not update the LTI callback URL to
match.
@mwest1066 mwest1066 mentioned this pull request Sep 12, 2019
mwest1066 added a commit that referenced this pull request Sep 12, 2019
This fixes a bug introduced in PR #1344 (commit 3cebe3d), which
changed the admin URL routes but did not update the LTI callback URL to
match.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants