firstdraft / grade_runner

A Ruby client for grades.firstdraft.com
MIT License
1 stars 0 forks source link

Read the grade token from the Environment Variables #41

Closed jelaniwoods closed 4 years ago

jelaniwoods commented 4 years ago

Resolves #39

Proposed changes:

Test by:

As a part of this change, grades will need to be updated to display the Gitpod link with the token embedded.

raghubetina commented 4 years ago

Good start! A few thoughts:

jelaniwoods commented 4 years ago

@raghubetina

Part of the objective is to make it slightly less easy for, let's say, precocious students (like my nephew) who wonder "what if I paste an access token for a project that I already have a 100% in across all my homeworks?" to do so.

While I do agree that people shouldn't be able to fool grade_runner with another project's token— should grade_runner be in charge of "knowing" whether a token has been tampered with? I remember discussing something like this before and we talked about how grades could identify if the spec/ folder had been modified and reject the grade or something, which would cover mismatch tokens.

We might need to add another rake task to fix a broken access token, in the rare case that something goes wrong with one, I suppose; e.g., if a student creates a Gitpod workspace manually and then pastes a token incorrectly.

I agree, we would need another rake task to reset the token in those circumstances. This makes me think that removing grades.yml doesn't actually solve the issue of people using wrong tokens since they would still be able to reset the token whenever they want. Albeit, in a perfect world, they wouldn't need to ever reset a token or know about the command to do so.

How about renaming GRADES_ACCESS_TOKEN to GITPOD_LTICI_APITOKEN to make it blend in?

Sure. What does the CI in LTICI stand for?

jelaniwoods commented 4 years ago

@raghubetina I'm not sure it's possible to permanently reset the Environment variable from a script, you can only set an Environment variable for the current process (Terminal). Any new Terminals would not have the Environment variable. To more permanently set one, you'd need to set the .bashrc or .bash_profile files, but that is not possible once a Gitpod workspace has been created. Gitpod won't save changes made outside the repo folder, so editing the .bashrc will be undone the next time the workspace starts up.

jelaniwoods commented 4 years ago

I guess the script could, once you enter your token, make a temporary branch and git commit, then push to GitHub, get the URL of the temporary branch, generate a URL for a new workspace and set the environment variable in the URL, and finally remove the temporary branch.

But that seems not ideal.

raghubetina commented 4 years ago

While I do agree that people shouldn't be able to fool grade_runner with another project's token— should grade_runner be in charge of "knowing" whether a token has been tampered with? I remember discussing something like this before and we talked about how grades could identify if the spec/ folder had been modified and reject the grade or something, which would cover mismatch tokens.

Yes, this is a larger and separate issue, and not necessarily grade_runner's responsibility. Nor is it something I want to deal with right now; probably something for Grades 2.0.

I agree, we would need another rake task to reset the token in those circumstances. This makes me think that removing grades.yml doesn't actually solve the issue of people using wrong tokens since they would still be able to reset the token whenever they want. Albeit, in a perfect world, they wouldn't need to ever reset a token or know about the command to do so.

Yep, I just want to add a tiny bit of friction / make it less blindingly obvious and easy to cheat. Right now it's so obvious that we rely wholly on the honor code, pretty much.

Sure. What does the CI in LTICI stand for?

I was thinking Continuous Integration. I got to it by inspecting existing env vars in a Gitpod workspace and massaging an existing one (I think CLI?). But I iterated on it to get there, the CI part is vestigial; we could go with GITPOD_LTI_APITOKEN if that feels better.

raghubetina commented 4 years ago

I'm not sure it's possible to permanently reset the Environment variable from a script, you can only set an Environment variable for the current process (Terminal).

Interesting; TIL. In that case, rather than a Ruby program to fix it, perhaps Grades can provide guidance to export the environment variable instead, then? Rather than just giving only providing the token itself.

I.e., we can have a muted message that looks something like:


Do you see a message "Invalid access token" when you run rails grade? Then click here.

At a Terminal prompt, run the following command:

export GITPOD_LTICI_APITOKEN="abc123"                         Click here to copy

Then, run rails grade again.

jelaniwoods commented 4 years ago

Do you see a message "Invalid access token" when you run rails grade? Then click here.

  export GITPOD_LTICI_APITOKEN="abc123"                         Click here to copy

@raghubetina the issue with setting the environment variable after workspace creation is that it's not permanent in Gitpod. export GITPOD_LTICI_APITOKEN="abc123" would only set GITPOD_LTICI_APITOKEN for the current Terminal (and any Terminals created from that Terminal). If a student were to open a new Terminal, or restart their workspace, the previously set GITPOD_LTICI_APITOKEN would revert back to its original value.

So we can't actually reset the token fully.

raghubetina commented 4 years ago

@jelaniwoods Ah, I understand now. In that case, perhaps we just name grades.yml something less obvious, bury it in some subfolder, and call it a day?

For example, we can call it .theia/.ltici_apitoken.yml?

jelaniwoods commented 4 years ago
  • How about renaming GRADES_ACCESS_TOKEN to GITPOD_LTICI_APITOKEN to make it blend in?

@raghubetina 🤔 it looks like I'm unable to name an environment variable this. I assume Gitpod prevents us from setting workspace ids and the internal environment variables that they use. LTICI_GITPOD_APITOKEN will work though.

raghubetina commented 4 years ago
  • How about renaming GRADES_ACCESS_TOKEN to GITPOD_LTICI_APITOKEN to make it blend in?

@raghubetina 🤔 it looks like I'm unable to name an environment variable this. I assume Gitpod prevents us from setting workspace ids and the internal environment variables that they use. LTICI_GITPOD_APITOKEN will work though.

LTICI_GITPOD_APITOKEN sounds good to me.

jelaniwoods commented 4 years ago

@raghubetina I've made the file changes— could I have some more review?

raghubetina commented 4 years ago

LGTM, nice work!

raghubetina commented 4 years ago

@jelaniwoods Oh actually, question: I think .gitignore needs to be updated? This might be an issue for appdev_template.