Securely Handle Personal Access Tokens#53
Securely Handle Personal Access Tokens#53micchickenburger wants to merge 15 commits intooppia:developfrom
Conversation
…n in order to connect to github api
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @micchickenburger! I think it looks really nice code-wise (haven't actually tried out the UI itself, but I'm expecting that we may continue iterate on that as we finalize the other aspects of the UI). Had a few nits & other suggestions.
| @@ -0,0 +1,14 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Will this file get served to users once the PR is checked in? It seems like pages by default serves all files present--is there a way we can blacklist this file?
There was a problem hiding this comment.
Really good question. After reviewing GitHub Pages documentation, it does not look like there is a way to blacklist files. However, this is an open repository anyway, and since the file isn't linked to any resource a user would have to explicitly navigate to it. Furthermore, downloaded scripts do not have the executable bit enabled; that has to be set manually. Finally, even if a user did download and execute the script, all they'd get is a self-signed cert and a simple web server. This script doesn't change any certificate trust or store settings on the system, and I can't really see a way to use it for malicious intent. Do you have any particular concerns?
There was a problem hiding this comment.
I think you outlined everything. I think it's worth noting these details at a high level in the Python file (just so that future maintainers aren't surprised that it can be accessed via the web browser, and that that's okay).
| To start an HTTPS simple server with a generated self-signed certificate, execute: | ||
|
|
||
| ```shell | ||
| $ python3 start.py |
There was a problem hiding this comment.
Suggest mentioning somewhere in this section that only Python 3 is supported when accessing the project dashboard (just to be explicit since above we have both Python 2 & 3 instructions).
There was a problem hiding this comment.
I think since Python 2 has been deprecated for over a year it might be okay to remove python2 instructions. Thoughts?
There was a problem hiding this comment.
We're planning to keep the Python 2 instructions since the Oppia web backend still uses it.
change project dashboard heading Co-authored-by: Ben Henning <henning.benmax@gmail.com>
| }; | ||
|
|
||
| // TODO: Write getters and setters for datastore | ||
| // TODO(55): Write getters and setters for datastore |
There was a problem hiding this comment.
| // TODO(55): Write getters and setters for datastore | |
| // TODO(#55): Write getters and setters for datastore |
| * Methods for securely processing and storing GitHub | ||
| * Personal Access Tokens (PATs). | ||
| * | ||
| * @method |
This pull request includes changes that: