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

Update docker-compose.yml to use localhost as app domain #13792

Merged
merged 1 commit into from May 25, 2021

Conversation

djuber
Copy link
Contributor

@djuber djuber commented May 18, 2021

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

Problem: Users are unable to login when using docker in development.
Suspected cause: HTTP Origin header doesn't match the (docker internal) name specified in the APP_DOMAIN.

There was an issue seen with the HTTP Origin header not matching (the error unhelpfully reported "localhost:3000" did not match
"localhost:3000").

Change the APP_DOMAIN internally to localhost:3000 (which is what the user will in fact use, not rails), and since that breaks the http requests from the db seed and sidekiq containers, allow them to only check for a tcp connection instead of expecting a 200
response (sending requests to the disallowed host rails triggered a 403).

Related Tickets & Documents

Related to discussion in #4955

#13696 looks related as well

https://forem.dev/manuel/forem-blank-page-after-login-using-docker-221d seems to describe the same issue

QA Instructions, Screenshots, Recordings

You may want to test this branch in a clean tree (i.e. re-clone the forem repo into a new directory) since the docker containers will leave root-owned files in places you might not want them (possibly including locking your manifest in a way that will break local development later, slowly, and in an aggravating way).

If you're doing thi s in a fresh clone, do copy .env_sample to .env before starting any processes.

If you're also running postgres and/or redis for local development, either disable them during container testing, or change the ports: sections to expose: (this prevents trying to bind to locahost:5432 and conflicting with the running service, but also requires you to access redis or the db from the rails container since the bindings are now only valid within the docker network for this system).

-    ports:
-      - "5432:5432"
+    expose:
+      - "5432"
     volumes:
       - db_data:/var/lib/postgresql/data:delegated
       
   redis:
     image: redis:6.0.9-alpine
     container_name: forem_redis
-    ports:
-      - "6379:6379"
+    expose:
+      - "6379"

If you're running the app locally on port 3000, stop it before proceeding - this has to be a ports: section since we'll be connecting to it from our browsers.

docker-compose up

Wait for webpack, and the db seed, to complete - sidekiq will keep itself busy for a while as it processes podcast episodes.

Connect to http://localhost:3000/, and login as the admin user.

On main I don't think this works (the post will respond with an empty 200 and the logs will show the origin conflict)

On this branch things should be okay.

Failure on main

visit http://localhost:3000/enter
login as admin@forem.local using default password

forem_rails  | Started POST "/users/sign_in" for 172.28.0.1 at 2021-05-18 17:32:01 +0000
forem_rails  |    (0.5ms)            SELECT ff.key AS feature_key, fg.key, fg.value           FROM flipper_features ff           LEFT JOIN flipper_gates fg ON ff.key = fg.feature_key 
forem_rails  |   ↳ app/middlewares/set_cookie_domain.rb:13:in `call'
forem_rails  | Processing by Devise::SessionsController#create as HTML
forem_rails  |   Parameters: {"utf8"=>"✓", "authenticity_token"=>"[FILTERED]", "user"=>{"email"=>"admin@forem.local", "password"=>"[FILTERED]", "remember_me"=>"1"}, "commit"=>"Continue"}
forem_rails  | HTTP Origin header (http://localhost:3000) didn't match request.base_url (http://localhost:3000)        
forem_rails  | Completed 200 OK in 4ms (ActiveRecord: 0.0ms | Allocations: 2926)

The blank 200 response leaves a white screen in the browser (user is not redirected to /)

Success on this branch

visit http://localhost:3000/enter
login as admin@forem.local using default password

forem_rails  | Started POST "/users/sign_in" for 172.28.0.1 at 2021-05-18 17:35:17 +0000
forem_rails  |    (0.5ms)            SELECT ff.key AS feature_key, fg.key, fg.value           FROM flipper_features ff           LEFT JOIN flipper_gates fg ON ff.key = fg.feature_key 
forem_rails  |   ↳ app/middlewares/set_cookie_domain.rb:13:in `call'
forem_rails  | Processing by Devise::SessionsController#create as HTML
forem_rails  |   Parameters: {"utf8"=>"✓", "authenticity_token"=>"[FILTERED]", "user"=>{"email"=>"admin@forem.local", "password"=>"[FILTERED]", "remember_me"=>"1"}, "commit"=>"Continue"}
forem_rails  |   User Load (1.4ms)  SELECT "users"."id",  (and many more columns) FROM "users" WHERE "users"."email" = $1 ORDER BY "users"."id" ASC LIMIT $2  [["email", "admin@forem.local"], ["LIMIT", 1]]
forem_rails  |   ↳ app/controllers/concerns/edge_cache_safety_check.rb:12:in `current_user'
forem_rails  |   Ahoy::Visit Load (0.5ms)  SELECT "ahoy_visits".* FROM "ahoy_visits" WHERE "ahoy_visits"."visit_token" = $1 ORDER BY "ahoy_visits"."id" ASC LIMIT $2  [["visit_token", "7bb49ec1-626f-5fdf-874c-b8c5073477db"], ["LIMIT", 1]]                                
forem_rails  |   ↳ app/controllers/concerns/edge_cache_safety_check.rb:12:in `current_user'
forem_rails  |   User Load (1.4ms)  SELECT "users"."id",  (and many more columns) FROM "users" WHERE "users"."remember_token" = $1 ORDER BY "users"."id" ASC LIMIT $2  [["remember_token", "adXeyt978-Vpy4t99FHX"], ["LIMIT", 1]]                                             
forem_rails  |   ↳ app/controllers/concerns/edge_cache_safety_check.rb:12:in `current_user'
forem_rails  |   TRANSACTION (0.3ms)  BEGIN
forem_rails  |   ↳ app/controllers/concerns/edge_cache_safety_check.rb:12:in `current_user'
forem_rails  |   User Update (2.3ms)  UPDATE "users" SET "updated_at" = $1, "remember_created_at" = $2, "remember_token" = $3 WHERE "users"."id" = $4  [["updated_at", "2021-05-18 17:35:17.845718"], ["remember_created_at", "2021-05-18 17:35:17.844616"], ["remember_token", "adXeyt978-Vpy4t99FHX"], ["id", 11]]
forem_rails  |   ↳ app/controllers/concerns/edge_cache_safety_check.rb:12:in `current_user'
forem_rails  |   Profile Load (0.6ms)  SELECT "profiles".* FROM "profiles" WHERE "profiles"."user_id" = $1 LIMIT $2  [["user_id", 11], ["LIMIT", 1]]
forem_rails  |   ↳ app/models/user.rb:302:in `block in <class:User>'
forem_rails  |   TRANSACTION (0.8ms)  COMMIT
forem_rails  |   ↳ app/controllers/concerns/edge_cache_safety_check.rb:12:in `current_user'
forem_sidekiq | 2021-05-18T17:35:17.897Z pid=19 tid=1q23 class=Users::BustCacheWorker jid=e46e000e4737b44f9d250d79 INFO: start
forem_rails  | Redirected to http://localhost:3000/?signin=true
forem_rails  | Completed 302 Found in 409ms (ActiveRecord: 18.0ms | Allocations: 170097)

I'm logged in and redirected to home /, where I can see "Write a post", possibly after going through the new user onboarding dialogs to accept code of conduct and follow tags.

UI accessibility concerns?

None, backend developer setup only

Added tests?

  • Yes
  • No, and this is why: no tests currently around "developer can use docker" scenarios
  • I need help with writing tests

[Forem core team only] How will this change be communicated?

  • I'm not sure how best to communicate this change and need help

I'll comment on the forem.dev report for this and the open issue letting the impacted users know.

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

stock image of frustrated user smashing laptop with a hammer

There was an issue seen with the HTTP Origin header not matching (the
error unhelpfully reported localhost:3000 did not match
localhost:3000).

Change the APP_DOMAIN internally to localhost:3000 (which is what the
user will in fact use, not rails), and since that breaks the http
requests from the db seed and sidekiq containers, allow them to only
check for a tcp connection instead of expecting a 200
response (sending requests to the disallowed host `rails` triggered a
403).

Related to discussion in #4955
@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label May 18, 2021
@djuber djuber changed the title Update docker-compose.yml file to permit login Update docker-compose.yml to use localhost as app domain May 18, 2021
@djuber

This comment has been minimized.

@djuber djuber marked this pull request as ready for review May 18, 2021 17:39
@djuber djuber requested review from a team and phannon716 and removed request for a team May 18, 2021 17:39
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels May 18, 2021
@djuber djuber requested review from a team, Ridhwana and jacobherrington and removed request for a team May 18, 2021 17:39
@djuber
Copy link
Contributor Author

djuber commented May 18, 2021

@forem/systems note: I did not update container-compose.yml (which is not a symlink but a distinct file) as I'm unable to confirm if this is needed, or if the change performs as expected. That should limit the blast radius for hosted forems but I invite your opinion there.

@@ -89,7 +89,7 @@ services:
DATABASE_URL: postgresql://forem:forem@db:5432/PracticalDeveloper_development
volumes:
- .:/opt/apps/forem:delegated
entrypoint: ["dockerize", "-wait", "tcp://db:5432", "-wait", "tcp://redis:6379", "-wait", "http://rails:3000", "-timeout", "2700s", "-wait-retry-interval", "20s"]
entrypoint: ["dockerize", "-wait", "tcp://db:5432", "-wait", "tcp://redis:6379", "-wait", "tcp://rails:3000", "-timeout", "2700s", "-wait-retry-interval", "20s"]
Copy link
Contributor Author

@djuber djuber May 18, 2021

Choose a reason for hiding this comment

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

tcp://host:port only checks that a service is listening, not that it's functioning. I think that's okay in this case (we really are waiting on the webserver to confirm all of the things it waits for, like bundle and db:prepare, have completed, rather than needing the webserver live in most cases to start the seed or background jobs.)

I played around a little with the -wait-http-header argument to see if I could change "Host" but didn't get the expected response (this still gives a 403 and may be ignoring the host header or have already sent one).

Digging through how dockerize handles http schema, I think this test case disproves being able to modify the host from the url:

package main

import (
	"net/http"
)

func main() {
	client := &http.Client{}
	req, _ := http.NewRequest("GET", "http://localhost:4000", nil)
	req.Header.Add("host", "hostheader")
	req.Header.Add("nonhost", "thenonhostheader")
	client.Do(req)
}

Now wait for it to execute by starting a tcp service - I do not get back the requested host header, suggesting Go doesn't support overriding the url host by passing a host header (the added header is just dropped):

$ nc -l 4000
GET / HTTP/1.1
Host: localhost:4000
User-Agent: Go-http-client/1.1
Nonhost: thenonhostheader
Accept-Encoding: gzip

Copy link
Contributor

@citizen428 citizen428 left a comment

Choose a reason for hiding this comment

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

I'm approving this with the caveat that I don't even have Docker installed anymore but the changes and rationale in this PR seem reasonable.

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels May 19, 2021
@djuber djuber merged commit a1c27cc into main May 25, 2021
@djuber djuber deleted the djuber/docker-compose-fix-login branch May 25, 2021 01:03
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants