zooniverse / theia

Building the next-generation Floating Forests pipeline
3 stars 2 forks source link

For OAuth to work, the container needs some additional environment variables. #120

Closed chelseatroy closed 3 years ago

chelseatroy commented 3 years ago

I have included them here in docker-compose file.

@zwolf if you need I can find a way to get the values for these env vars to ya!

Now, this doesn't fully fix oauth, because now when I go to authenticate I get "the redirect_uri is invalid." I've fixed this issue once before with @camallen, and the solution had to do with doorkeeper wanting a backslash. So I went into our oauth app for theia: https://panoptes.zooniverse.org/oauth/applications/128 and added the appropriate (I THOUGHT) redirect uri for our prod deployment, and I'm still getting the same error.

Does anything look wrong with that redirect_uri I added?

zwolf commented 3 years ago

The Doorkeeper app creds (PANOPTES_PROD_CLIENT_ID and PANOPTES_PROD_CLIENT_SECRET) are already being loaded from the kubernetes secret. Why does the app need both an app key/secret and a user login/password? What OAuth problem is this intended to solve?

Also I don't believe that the docker-compose file is being used outside of development. Production is spinning up the Dockerfile as a pod, so anything new should go in deploy template (except the Panoptes URL, which is already in there as PANOPTES_PROD_URL) or the secret (loaded by these lines )

zwolf commented 3 years ago

The var names are different than the ones in this PR, though, I must have taken them from the .env file:

https://github.com/zooniverse/theia/blob/5177020205a7f2d5e1cea6aa9c9f1cd302b648bc/.env.example#L4-L5

but the PanoptesUtils are using the shorter version: https://github.com/zooniverse/theia/blob/c5149bee26efdb71532bca2fd0291d43e8367721/theia/utils/panoptes_utils.py#L5-L12 I can change those keys in the secret if that's what they should be called.

chelseatroy commented 3 years ago

The var names are different than the ones in this PR, though, I must have taken them from the .env file:

https://github.com/zooniverse/theia/blob/5177020205a7f2d5e1cea6aa9c9f1cd302b648bc/.env.example#L4-L5

but the PanoptesUtils are using the shorter version: https://github.com/zooniverse/theia/blob/c5149bee26efdb71532bca2fd0291d43e8367721/theia/utils/panoptes_utils.py#L5-L12

I can change those keys in the secret if that's what they should be called.

@zwolf yes please!

chelseatroy commented 3 years ago

I also pushed an additional commit to update the env vars to the appropriate ones for our current version of the app!

zwolf commented 3 years ago

OK, the env var keys are fixed and the deployment was rolled, so the deployed version has the correctly named keys.

Couple things, though: why does this need user credentials? Also, I don't think this PR is going to change how the production app is building the OAuth URLs for the reason above.

chelseatroy commented 3 years ago

@zwolf —I'm not seeing the username or password used anywhere in the code bases. If we take them out, does the deployment break?

Change pushed to the k8s template!

zwolf commented 3 years ago

The lines in question are added by this PR, both in the docker-compose (used in dev) and the .env.example:

https://github.com/zooniverse/theia/blob/d0f62ae11aa2ff7719ce3dd2ac961b1bec6624dc/docker-compose.yml#L37-L38