backend: Stop using environment variables for Canvas API Tokens?

[frozenfrank]

Overview

It's my understanding that we had stopped using environment variables in the autograder for security reasons.

I noticed that the Canvas Integration test because I do not have a Canvas API environment variable established. I figure this could have slipped by other's radars because might have the variable set in their run configuration, and therefore have forgotten to adjust this instance in the code.

If in fact, we do intend to be reading the API token from the environment variable, please close this issue. An explanation should be great too: I would love to better understand why we want it to work the way it does.

Screenshots

[pawlh]

Yes, this was intentional, although I'm sure there could be other alternatives.

Environment variables are not usually a security concern, except given the nature of the project (we're running unvetted code in the same environment), program arguments seemed to be the safer option for secrets.

The tests are a little different in that the only code that will be running will be our own, and thus not vulnerable to unvetted code. In fact, integration tests should essentially only ever run on GitHub Actions (as opposed to locally), so developers of the autograder should not need to modify their environment variables.

I'll hold off from closing this though as I feel less inclined to dictate the direction of this project at this point 😉

(a little further explanation: environment variables are a little easier to work with when it comes to Maven. Program arguments can be provided to JUnit through Maven, but the process is less intuitive and harder to understand. Environment variables provide an easier and more familiar experience, ideal for test maintenance)

[pawlh]

@19mdavenport

[frozenfrank]

Thanks for providing that additional information. Everything you've shared makes sense; I just have a counter-point to present.

In fact, integration tests should essentially only ever run on GitHub Actions (as opposed to locally), so developers of the autograder should not need to modify their environment variables.

The argument here appears to be that developers won't see these tests running, and thus those developers don't need to worry about them. However, they do still fail when running "All Tests in Tests" which makes this issue pretty obvious. Where a developer would expect to see a green checkmark, this approach now nearly guarantees that they will see red X's representing that some tests are failing. While this is an issue in itself, it also requires additional information to understand that some of the tests are expected to fail on local machines (but not all of them, obviously).

I didn't know this, and it would be difficult for a new person on the project to figure out.

Is there a way we can interpret the ENV variables more intelligently to skip these tests when the configuration is not present, or by some other mechanism or combination of mechanisms? This way local developers can see that all tests are passing then evaluating features and also allow us to configure the GitHub Actions to process additional tests.