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

Buildpack experimentation #25

Merged
merged 2 commits into from
Jul 7, 2018
Merged

Buildpack experimentation #25

merged 2 commits into from
Jul 7, 2018

Conversation

CatChen
Copy link
Contributor

@CatChen CatChen commented Jul 5, 2018

This is a big change. Not a lot of files changed inside this repo, but more outside.

Here's what I want to achieve: Set OAuth app_id, client_id, client_secret as environment variables on the server and then nobody needs to set them separately. (Otherwise, nobody would really want to try out this app because nobody would apply for Hue OAuth by themselves.)

The biggest blocker is the OAuth client_secret needed for the request to /oauth2/token. I can't expose it in JavaScript. If I use typical REACT_APP_* environment variable, it will be compiled into JavaScript and expose. I have to keep it on the server side while use it when making the request to /oauth2/token.

The way I solved this is to ask Nginx to read the client_secret variable from the server directly and append it to the request header, so client_secret never leaves the server. It sounds simple but it's not supported in existing Heroku buildpack setup, so I had to change that.

First I added the header config in static.json, which is the source file that heroku-buildpack-static uses and compiles into Nginx config. This doesn't do anything, because heroku-buildpack-static doesn't recognize this new information yet.

Then I forked heroku-buildpack-static and changed that: CatChen/heroku-buildpack-static@a77851a. It can extract the header settings from static.json, replace environment variable with real value and put that into Nginx config.

Finally I forked create-react-app-buildpack, which is the actual buildpack used by this app. It depends on the official heroku-buildpack-static and I pointed that to my fork.

Now I can set the environment variables on Heroku and we don't have to set it inside the Settings dialog. At least, that's for http://hue-explorer.herokuapp.com. (It doesn't work for other Heroku instances of the same source code deployment, because environment variables don't share.)

@CatChen CatChen self-assigned this Jul 5, 2018
@CatChen CatChen requested a review from a team July 5, 2018 02:08
@CatChen CatChen temporarily deployed to hue-explorer-development-pr-25 July 5, 2018 02:08 Inactive
@codecov
Copy link

codecov bot commented Jul 5, 2018

Codecov Report

Merging #25 into master will decrease coverage by 0.05%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   35.61%   35.56%   -0.05%     
==========================================
  Files          34       34              
  Lines         702      703       +1     
  Branches      129      130       +1     
==========================================
  Hits          250      250              
  Misses        362      362              
- Partials       90       91       +1
Impacted Files Coverage Δ
src/ui/HueBridgeSelector.js 48.15% <ø> (ø) ⬆️
src/index.js 0% <0%> (ø) ⬆️

@shihangw
Copy link
Contributor

shihangw commented Jul 6, 2018

Not exactly sure what's the reason why heroku vars doesn't work..? https://devcenter.heroku.com/articles/config-vars

@shihangw
Copy link
Contributor

shihangw commented Jul 6, 2018

I think there should be some middleware for node.js to read vars and append secrets as oauth header

@CatChen
Copy link
Contributor Author

CatChen commented Jul 6, 2018

I’m using Heroku environment variable already. The thing is this app is set up as a static website — no server side code and serving only HTML, JS, CSS, etc.

The only exception is it supports proxy, but I don’t write any code for the proxy. It’s just config on the Nginx that serves the static content. Nginx config allows header setting in proxied requests, but I don’t control the Nginx config directly. The config file is generated from static.json by what Heroku calls a buildpack.

@CatChen
Copy link
Contributor Author

CatChen commented Jul 6, 2018

In short, I don’t have any Node.js code running in the project. All JS in this project runs in browser. Can I add Node.js support and create server side code? Yes, but I want to stay away from that for now.

@CatChen CatChen force-pushed the buildpack-experimentation branch from 7550fd7 to aab73b9 Compare July 6, 2018 06:09
@CatChen CatChen temporarily deployed to hue-explorer-development-pr-25 July 6, 2018 06:09 Inactive
Copy link
Contributor

@shihangw shihangw left a comment

Choose a reason for hiding this comment

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

gogogo

@shihangw shihangw merged commit c9dba01 into master Jul 7, 2018
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