Skip to content

Commit

Permalink
MB-2321 Allow revoking individual user sessions
Browse files Browse the repository at this point in the history
**Description**:
In order to obtain an ATO (Authority to Operate) for MilMove, we need
to provide a way to revoke individual user sessions. Currently,
session management is provided by JWTs per ADR 15, but JWTs aren't
designed to be revoked on an individual basis. Instead, we need to
store session data on the server.

In this PR, we have chosen Redis because it automatically deletes
expired sessions. With Postgres, we would need to run a routine
periodically to clean up stale sessions.

After researching various session management solutions, I chose
`scs` because it was the easiest to integrate, and it supports various
stores out of the box. It is the second most popular repo after
`gorilla/sessions`. I didn't pick `gorilla/sessions` because it suffers
from memory leak issues, and uses its own `context` instead of the
`request.Context()` provided by Golang. The maintainers are aware of
the issues and have opened a GitHub issue to propose improvements for
v2. However, it doesn't look like any progress has been made over the
past 2 years, while `scs` has implemented most of those improvements.

Note that this PR does not provide the ability to limit concurrent
sessions or revoke individual sessions via the admin interface. That
work is being done in a separate branch.

**Setup**:
`docker pull redis`

**Reviewer Notes**:
Things to test:

**milmovelocal auth**
1. Go to milmovelocal:3000
- [ ] Verify that a session cookie named "mil_session_token" is present
(Developer Tools -> Application tab -> Cookies (under Storage in the
left sidebar))
- [ ] Verify that the value in the `Expires/Max-Age` column is `Session`
- [ ] Verify that the HttpOnly column is checked
- [ ] Verify that the Path is `/`
- [ ] Verify that `SameSite` is `Lax`
2. In your Terminal, run `redis-dev`, then type `KEYS *`
- [ ] Verify there is an entry labeled `scs:session:token`, where
`token` is the `Value` of the `mil_session_token` cookie
3. Sign in
- [ ] Verify that after successful sign in, the `Value` of the
`mil_session_token` cookie changes
4. Run `KEYS *` again in the Redis console
- [ ] Verify that the previous entry is gone and that a new one
corresponding to the new session cookie is present
5. Sign out
- [ ] Verify that the previous entry in Redis is gone and that a new one
corresponding to the new session cookie is present
- [ ] Verify that the session cookie changed in the browser
6. In `serve.go`, on line 504, change the IdleTimeout from 15 minutes
to 1 minute
7. Sign in, then wait a little over a minute
8. Try to make a new request without refreshing the browser, for
example, filling out the moves form and clicking the Next button
- [ ] Verify that you are not able to make a request and that you see
an Unauthorized Error. Ideally, the user would be redirected to the
sign in page. I'm working on implementing that.

**devlocal auth**
- [ ] Verify you can sign in and out via devlocal auth flow:
http://milmovelocal:3000/devlocal-auth/login
- [ ] Verify you can create a New milmove User
- [ ] Verify you can create a New dps User

**Role based auth**
1. In your `.envrc.local`, add
`export FEATURE_FLAG_ROLE_BASED_AUTH=true`
2. Stop the server, run `direnv allow`
3. run `echo $FEATURE_FLAG_ROLE_BASED_AUTH` to make sure it's `true`
4. run `make server_run`
5. Go to milmovelocal:3000 and make sure you can sign in and out

**Logging into multiple apps at the same time**
1. Log in to http://milmovelocal:3000/
2. In a separate tab, log in to http://officelocal:3000
3. In a separate tab, log in to http://adminlocal:3000
4. Refresh each tab. Verify that you are still signed into each app.

**References**:
[gorilla sessions issues](gorilla/sessions#105)
[scs repo](https://github.com/alexedwards/scs)
  • Loading branch information
Moncef Belyamani committed Jun 16, 2020
1 parent eb2fea9 commit 035c2ba
Show file tree
Hide file tree
Showing 39 changed files with 993 additions and 607 deletions.
4 changes: 2 additions & 2 deletions .envrc
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ require MOVE_MIL_DOD_TLS_CERT "See 'chamber read app-devlocal move_mil_dod_tls_c
require MOVE_MIL_DOD_TLS_KEY "See 'chamber read app-devlocal move_mil_dod_tls_key'"
export MOVE_MIL_DOD_CA_CERT

# Prevent user sessions from timing out
export NO_SESSION_TIMEOUT=true
# Set a long inactivity timeout for local sessions
export SESSION_IDLE_TIMEOUT_IN_MINUTES=30

# Use UTC timezone
export TZ="UTC"
Expand Down
1 change: 1 addition & 0 deletions .spelling
Original file line number Diff line number Diff line change
Expand Up @@ -599,5 +599,6 @@ prac-frontend
VSCode
PascalCased
camelCased
redis-cli
# Put all custom terms BEFORE this comment, lest 'pre-commit' and 'make spellcheck' yield different errors.
- ./docs/backend/route-planner.md
56 changes: 46 additions & 10 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ DB_DOCKER_CONTAINER_TEST = milmove-db-test
# as possible.
# https://github.com/transcom/transcom-infrasec-com/blob/c32c45078f29ea6fd58b0c246f994dbea91be372/transcom-com-legacy/app-prod/main.tf#L62
DB_DOCKER_CONTAINER_IMAGE = postgres:12.2
REDIS_DOCKER_CONTAINER_IMAGE = redis:5.0.6
REDIS_DOCKER_CONTAINER = milmove-redis
TASKS_DOCKER_CONTAINER = tasks
export PGPASSWORD=mysecretpassword

Expand All @@ -34,6 +36,8 @@ DB_PORT_DEV=5432
DB_PORT_TEST=5433
DB_PORT_DEPLOYED_MIGRATIONS=5434
DB_PORT_DOCKER=5432
REDIS_PORT=6379
REDIS_PORT_DOCKER=6379
ifdef CIRCLECI
DB_PORT_DEV=5432
DB_PORT_TEST=5432
Expand Down Expand Up @@ -118,7 +122,7 @@ check_docker_size: ## Check the amount of disk space used by docker
scripts/check-docker-size

.PHONY: deps
deps: prereqs ensure_pre_commit client_deps bin/rds-ca-2019-root.pem ## Run all checks and install all depdendencies
deps: prereqs ensure_pre_commit client_deps redis_pull bin/rds-ca-2019-root.pem ## Run all checks and install all depdendencies

.PHONY: test
test: client_test server_test e2e_test ## Run all tests
Expand Down Expand Up @@ -302,7 +306,7 @@ server_build: bin/milmove ## Build the server

# This command is for running the server by itself, it will serve the compiled frontend on its own
# Note: Don't double wrap with aws-vault because the pkg/cli/vault.go will handle it
server_run_standalone: check_log_dir server_build client_build db_dev_run
server_run_standalone: check_log_dir server_build client_build db_dev_run redis_run
DEBUG_LOGGING=true ./bin/milmove serve 2>&1 | tee -a log/dev.log

# This command will rebuild the swagger go code and rerun server on any changes
Expand All @@ -311,7 +315,7 @@ server_run:
# This command runs the server behind gin, a hot-reload server
# Note: Gin is not being used as a proxy so assigning odd port and laddr to keep in IPv4 space.
# Note: The INTERFACE envar is set to configure the gin build, milmove_gin, local IP4 space with default port GIN_PORT.
server_run_default: .check_hosts.stamp .check_go_version.stamp .check_gopath.stamp .check_node_version.stamp check_log_dir bin/gin build/index.html server_generate db_dev_run
server_run_default: .check_hosts.stamp .check_go_version.stamp .check_gopath.stamp .check_node_version.stamp check_log_dir bin/gin build/index.html server_generate db_dev_run redis_run
INTERFACE=localhost DEBUG_LOGGING=true \
$(AWS_VAULT) ./bin/gin \
--build ./cmd/milmove \
Expand All @@ -324,7 +328,7 @@ server_run_default: .check_hosts.stamp .check_go_version.stamp .check_gopath.sta
2>&1 | tee -a log/dev.log

.PHONY: server_run_debug
server_run_debug: .check_hosts.stamp .check_go_version.stamp .check_gopath.stamp .check_node_version.stamp check_log_dir build/index.html server_generate db_dev_run ## Debug the server
server_run_debug: .check_hosts.stamp .check_go_version.stamp .check_gopath.stamp .check_node_version.stamp check_log_dir build/index.html server_generate db_dev_run redis_run ## Debug the server
scripts/kill-process-on-port 8080
scripts/kill-process-on-port 9443
$(AWS_VAULT) dlv debug cmd/milmove/*.go -- serve 2>&1 | tee -a log/dev.log
Expand Down Expand Up @@ -396,7 +400,7 @@ mocks_generate: bin/mockery ## Generate mockery mocks for tests
go generate $$(go list ./... | grep -v \\/pkg\\/gen\\/ | grep -v \\/cmd\\/)

.PHONY: server_test
server_test: db_test_reset db_test_migrate server_test_standalone ## Run server unit tests
server_test: db_test_reset db_test_migrate redis_reset server_test_standalone ## Run server unit tests

.PHONY: server_test_standalone
server_test_standalone: ## Run server unit tests with no deps
Expand All @@ -407,20 +411,20 @@ server_test_build:
NO_DB=1 DRY_RUN=1 scripts/run-server-test

.PHONY: server_test_all
server_test_all: db_dev_reset db_dev_migrate ## Run all server unit tests
server_test_all: db_dev_reset db_dev_migrate redis_reset ## Run all server unit tests
# Like server_test but runs extended tests that may hit external services.
LONG_TEST=1 scripts/run-server-test

.PHONY: server_test_coverage_generate
server_test_coverage_generate: db_test_reset db_test_migrate server_test_coverage_generate_standalone ## Run server unit test coverage
server_test_coverage_generate: db_test_reset db_test_migrate redis_reset server_test_coverage_generate_standalone ## Run server unit test coverage

.PHONY: server_test_coverage_generate_standalone
server_test_coverage_generate_standalone: ## Run server unit tests with coverage and no deps
# Add coverage tracker via go cover
NO_DB=1 COVERAGE=1 scripts/run-server-test

.PHONY: server_test_coverage
server_test_coverage: db_test_reset db_test_migrate server_test_coverage_generate ## Run server unit test coverage with html output
server_test_coverage: db_test_reset db_test_migrate redis_reset server_test_coverage_generate ## Run server unit test coverage with html output
DB_PORT=$(DB_PORT_TEST) go tool cover -html=coverage.out

.PHONY: server_test_docker
Expand All @@ -435,6 +439,38 @@ server_test_docker_down:
# ----- END SERVER TARGETS -----
#

#
# ----- START REDIS TARGETS -----
#

.PHONY: redis_pull
redis_pull: ## Pull redis image
docker pull $(REDIS_DOCKER_CONTAINER_IMAGE)

.PHONY: redis_destroy
redis_destroy: ## Destroy Redis
@echo "Destroying the ${REDIS_DOCKER_CONTAINER} docker redis container..."
docker rm -f $(REDIS_DOCKER_CONTAINER) || echo "No Redis container"

.PHONY: redis_run
redis_run: ## Run Redis
ifndef CIRCLECI
@echo "Stopping the Redis brew service in case it's running..."
brew services stop redis 2> /dev/null || true
endif
@echo "Starting the ${REDIS_DOCKER_CONTAINER} docker redis container..."
docker start $(REDIS_DOCKER_CONTAINER) || \
docker run -d --name $(REDIS_DOCKER_CONTAINER) \
-p $(REDIS_PORT):$(REDIS_PORT_DOCKER) \
$(REDIS_DOCKER_CONTAINER_IMAGE)

.PHONY: redis_reset
redis_reset: redis_destroy redis_run ## Reset Redis

#
# ----- END REDIS TARGETS -----
#

#
# ----- START DB_DEV TARGETS -----
#
Expand Down Expand Up @@ -667,7 +703,7 @@ db_e2e_up: bin/generate-test-data ## Truncate Test DB and Generate e2e (end-to-e
DB_PORT=$(DB_PORT_TEST) bin/generate-test-data --named-scenario="e2e_basic" --db-env="test"

.PHONY: db_e2e_init
db_e2e_init: db_test_reset db_test_migrate db_e2e_up ## Initialize e2e (end-to-end) DB (reset, migrate, up)
db_e2e_init: db_test_reset db_test_migrate redis_reset db_e2e_up ## Initialize e2e (end-to-end) DB (reset, migrate, up)

.PHONY: db_dev_e2e_populate
db_dev_e2e_populate: db_dev_reset db_dev_migrate ## Populate Dev DB with generated e2e (end-to-end) data
Expand All @@ -677,7 +713,7 @@ db_dev_e2e_populate: db_dev_reset db_dev_migrate ## Populate Dev DB with generat
go run github.com/transcom/mymove/cmd/generate-test-data --named-scenario="e2e_basic" --db-env="development"

.PHONY: db_test_e2e_populate
db_test_e2e_populate: db_test_reset db_test_migrate build_tools db_e2e_up ## Populate Test DB with generated e2e (end-to-end) data
db_test_e2e_populate: db_test_reset db_test_migrate redis_reset build_tools db_e2e_up ## Populate Test DB with generated e2e (end-to-end) data

.PHONY: db_test_e2e_backup
db_test_e2e_backup: ## Backup Test DB as 'e2e_test'
Expand Down
Loading

0 comments on commit 035c2ba

Please sign in to comment.