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 CGI middleware for requests to People API #346

Merged
merged 6 commits into from
Apr 3, 2020
Merged

Conversation

ttpcodes
Copy link
Member

@ttpcodes ttpcodes commented Dec 13, 2019

This PR adds a new CGI handler called people.py that is responsible for making requests to the People API on behalf of any CourseRoad clients. Requests can be made to this endpoint with a user's kerb and the API will return either an error or the year of the user zero-indexed. This resolves an issue in which the credentials for interacting with the API was stored directly in the client-side code.

This PR also modifies the logic of the webpack-dev-server to allow for it to run its own CGI handler so that this logic can be tested during development as well. For this to work properly, each person will need to obtain a copy of credentials.ini to set up this script (which I currently have) since these credentials will not be committed to GitHub.

A few changes will need to be made once this is merged into master to make sure this works properly, including setting up any dependencies that this CGI middleware requires on Scripts. Once this is approved by someone, I'll start testing on dev before deploying to production.

@npfoss
Copy link
Collaborator

npfoss commented Feb 8, 2020

note: we should make sure there aren't CORS errors on either side, those are starting to show up again

@ttpcodes
Copy link
Member Author

The request from the Python script to the CourseRoad server doesn't have a CORS error. As long as CourseRoad is able to contact the Python script (considering they're on the same subdomain, I'd hope no CORS error appears), we should be fine.

Copy link
Collaborator

@npfoss npfoss left a comment

Choose a reason for hiding this comment

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

I like this setup a lot! Very simple and elegant, and importantly, the same between prod and local. A few comments:

There's no indication of a problem if credentials.ini is missing/wrong, it just doesn't get the year (and doesn't print that it didn't). Could you please add some indicator so we (and new devs) don't get tripped up by this? Not sure where the best place is. Even an error would be good I think, so we can tell it's happening.

Also, I deployed to dev and it didn't seem to work (no year printout, no cookie). Might be related to the ambiguous Error: Request failed with status code 404.

Can you also add the additional setup instructions to the README please? (like python packages, how to get credentials, etc)

build/webpack.config.dev.js Show resolved Hide resolved
@ttpcodes ttpcodes force-pushed the people-middleware branch from e5facf6 to 4535083 Compare March 4, 2020 00:22
Comment on lines 47 to 48
print(error + content)
print(dumps(output))
Copy link
Collaborator

Choose a reason for hiding this comment

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

these two lines appear in every case, I think. can't we just put them once at the end?

@npfoss npfoss merged commit c3e0932 into master Apr 3, 2020
@npfoss npfoss deleted the people-middleware branch April 3, 2020 20:30
miriam-rittenberg pushed a commit that referenced this pull request Oct 21, 2020
* Add CGI wrapper for People API requests

* Add CGI handler to dev server

* Point Auth to new People API endpoint

* Deploy cgi-bin

* Condense print statements in CGI script

Co-authored-by: Nate Foss <[email protected]>
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.

2 participants