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

Assigns session data based on app name [delivers #163469960] #1951

Merged
merged 3 commits into from Apr 2, 2019

Conversation

4 participants
@stangah
Copy link
Contributor

stangah commented Apr 2, 2019

Description

Currently we populate the session with OfficeUserID and TSPUserID regardless of which app they are accessing. Because our model authorization checks are mutually exclusive (if isTSPUser do something, else if isOfficeUser do something else), users were potentially being restricted with TSP auth checks when on the office app or vice versa.

This PR modifies that code so that OfficeUserID is only populated when accessing the office app, and similarly for the TSP data and app.

References

@stangah stangah force-pushed the ps-auth-role-fix branch from 5d134ad to b56741c Apr 2, 2019

@stangah stangah changed the title Assigns session data based on app name Assigns session data based on app name [delivers #163469960] Apr 2, 2019

@stangah stangah marked this pull request as ready for review Apr 2, 2019

@stangah stangah requested a review from chrisgilmerproj Apr 2, 2019

@stangah stangah assigned kahlouie and unassigned kahlouie Apr 2, 2019

@stangah stangah requested a review from kahlouie Apr 2, 2019

@@ -271,53 +271,57 @@ func authorizeKnownUser(userIdentity *models.UserIdentity, h CallbackHandler, se
session.DpsUserID = *(userIdentity.DpsUserID)
}

if userIdentity.OfficeUserID != nil {
session.OfficeUserID = *(userIdentity.OfficeUserID)

This comment has been minimized.

Copy link
@stangah

stangah Apr 2, 2019

Author Contributor

This line (and the TSP equivalent) is the main culprit, it was being run regardless of session.IsOfficeApp()

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 2, 2019

Codecov Report

Merging #1951 into master will decrease coverage by 0.11%.
The diff coverage is 13.64%.

@@            Coverage Diff             @@
##           master    #1951      +/-   ##
==========================================
- Coverage    60.8%   60.69%   -0.11%     
==========================================
  Files         192      193       +1     
  Lines       12232    12280      +48     
==========================================
+ Hits         7437     7453      +16     
- Misses       3920     3953      +33     
+ Partials      875      874       -1
@kahlouie
Copy link
Contributor

kahlouie left a comment

From a cursory glance, this looks good. Thanks for adding tests!

TspUserID: &tspUserID,
}

req := httptest.NewRequest("GET", fmt.Sprintf("http://%s/auth/authorize", "tsp.example.com"), nil)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 2, 2019

Contributor

Use TspTestHost for this test.

TspUserID: &tspUserID,
}

req := httptest.NewRequest("GET", fmt.Sprintf("http://%s/auth/authorize", "office.example.com"), nil)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Apr 2, 2019

Contributor

Use OfficeTestHost for this test.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

This is great work! I'm approving but there are two minor fixes for the test code I'd like addressed.

@chrisrcoles chrisrcoles merged commit be00c80 into master Apr 2, 2019

17 of 19 checks passed

codecov/patch 13.64% of diff hit (target 60.8%)
Details
codecov/project/go 60.52% (-0.11%) compared to 4386f9e
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
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_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
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.