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

Use new admin users table to authentication and authorization. #2377

Merged
merged 12 commits into from Jul 22, 2019

Conversation

4 participants
@garrettqmartin8
Copy link
Contributor

commented Jul 11, 2019

Description

We currently can't log into the admin app on dev, experimental, or staging. We're also not properly authorizing users for hitting admin app endpoints. The changes in this PR take advantage of the new admin_users table to make use of a strategy very similar to what we use to authenticate office users, both unknown (logging in for the first time) and known.

Reviewer Notes

  • How do we feel about the AdminAuthMiddleware?
  • I've created a secure migration for bootstrapping us with a few system admin users:
    • Risa (the new product person on this feature)
    • Alexi (the original product person on this feature)
    • Me
    • Shall I go ahead and add more? The purpose of this is so we can begin verifying admin stories in staging.

Setup

  • In local_migrations/20190710174257_add_system_admin_users.sql, edit one of the insert statements to reflect the email address you use with login.gov.
  • make db_dev_e2e_migrate
  • make server_run
  • make client_run

Code Review Verification Steps

  • Visit adminlocal:3000
  • Log in via login.gov with the email address you entered in the local migration
  • Ensure you don't see an unauthorized page
  • Log out
  • Login in as an office user, either devlocal or via login.gov
  • Attempt to visit officelocal:3000/admin/v1/office_users and ensure you hit an Unauthorized page

References

@chrisgilmerproj
Copy link
Contributor

left a comment

This looks really good. I've got some concerns about the migrations that I expect won't be hard to address. The middleware is an interesting approach which I think works because its limited to just the admin app. I'm curious how that might play out for other routes.

@mikena-truss
Copy link
Contributor

left a comment

Overall looks solid! Left some code style comments.

Show resolved Hide resolved pkg/auth/authentication/auth.go Outdated

// IsSystemAdmin checks whether the authenticated admin user is a system admin
func (s *Session) IsSystemAdmin() bool {
role := "SYSTEM_ADMIN"

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jul 11, 2019

Contributor

We might want to use a similar to pattern to how we do move states here.

In the model:

type AdminRole

const (
	SystemAdminRole AdminRole = "SYSTEM_ADMIN"
	ProgramAdminRole AdminRole = "PROGRAM_ADMIN"
)

Then here you get models.SystemAdminRole rather than redefining the constants.

This comment has been minimized.

Copy link
@mikena-truss

mikena-truss Jul 11, 2019

Contributor

Right now it looks like we use a ValidRoles function, but I think that can work hand in hand we these const.

Show resolved Hide resolved pkg/models/admin_user.go Outdated
Show resolved Hide resolved pkg/models/admin_user.go Outdated
@codecov

This comment has been minimized.

Copy link

commented Jul 11, 2019

Codecov Report

Merging #2377 into master will decrease coverage by 0.1%.
The diff coverage is 45.9%.

@@           Coverage Diff            @@
##           master   #2377     +/-   ##
========================================
- Coverage    59.6%   59.5%   -<.1%     
========================================
  Files         268     267      -1     
  Lines       15141   15238     +97     
========================================
+ Hits         9012    9055     +43     
- Misses       5066    5118     +52     
- Partials     1063    1065      +2
Impacted Files Coverage Δ
pkg/auth/session.go 50% <0%> (-10.7%) ⬇️
pkg/auth/authentication/auth.go 27.8% <28.8%> (+0.1%) ⬆️
pkg/models/admin_user.go 61.8% <72.3%> (-0.1%) ⬇️
pkg/auth/authentication/devlocal.go 37% <77%> (+1%) ⬆️
pkg/models/user.go 80.9% <77.3%> (+3.6%) ⬆️
pkg/services/accesscode/fetch_access_code.go
@garrettqmartin8

This comment has been minimized.

Copy link
Contributor Author

commented Jul 12, 2019

@mikena-truss I've made some changes based on your feedback.

@garrettqmartin8

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

@chrisgilmerproj would you mind taking another look? Still one outstanding questions about point of contact.

@garrettqmartin8 garrettqmartin8 force-pushed the gm-admin-users-auth branch from c5719d4 to 31143f9 Jul 15, 2019

@Ryan-Koch
Copy link
Contributor

left a comment

Added in a question about a return code. Otherwise with the changes you've put in this looks really good. That said, I'd suggest also getting an approval from either @chrisgilmerproj or @mikena-truss to confirm.

@garrettqmartin8 garrettqmartin8 force-pushed the gm-admin-users-auth branch from 31143f9 to be995d6 Jul 16, 2019

@mikena-truss

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2019

Left a couple minor comments. Other than that looks good.

@mikena-truss
Copy link
Contributor

left a comment

lgtm. @chrisgilmerproj still has a block though

@chrisgilmerproj
Copy link
Contributor

left a comment

I'm looking at the secure migrations and a couple things come to mind.

  1. You're adding the new organization Truss in this PR. Can you update all existing truss users to also have the organization set?

  2. You're dropping is_superuser from the users table. However, won't there will be code running in production looking at this column when its dropped? I expect that to cause errors. I'd rather we remove the code from production that relies on it first and then come back around and do a separate migration to drop that column.

  1. This is a nit-pick, but I would love to see us use lower-cased UUIDs in the migrations if possible.

I think we're almost there and I'll keep an eye out for changes so I can approve quickly.

@donaldthai donaldthai removed their request for review Jul 18, 2019

@garrettqmartin8 garrettqmartin8 force-pushed the gm-admin-users-auth branch from be995d6 to 9bf0763 Jul 19, 2019

@garrettqmartin8

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2019

@chrisgilmerproj I've addressed 1 & 3 and in an updated secure migration.

Regarding number 2: I've replaced as many references to superuser as I've found and have been rebasing master frequently to make sure I'm not letting any slip through the cracks. Are there other cases that I'm not thinking of?

garrettqmartin8 and others added some commits Jul 8, 2019

Use correct middleware name in logs
Co-Authored-By: Chris Gilmer <chris@truss.works>

@garrettqmartin8 garrettqmartin8 force-pushed the gm-admin-users-auth branch from e36d9e4 to e6a53b9 Jul 22, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

🚀 - Thanks for the work on this!

@garrettqmartin8 garrettqmartin8 merged commit 999adf5 into master Jul 22, 2019

18 of 19 checks passed

codecov/patch/go 45.9% of diff hit (target 59.4%)
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/project/go 59.3% (-<.1%) compared to c2d5cc9
Details

@garrettqmartin8 garrettqmartin8 deleted the gm-admin-users-auth branch Jul 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.