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

GPII-3551: Improvements and bug fix for /ready and /health endpoint #713

Merged
merged 9 commits into from Dec 12, 2018

Conversation

Projects
None yet
4 participants
@klown
Copy link
Contributor

commented Nov 29, 2018

JIRA: https://issues.gpii.net/browse/GPII-3551

@cindyli @mrtyler The new code for the /health and /ready endpoints is nearly there. What's missing:

  • when I run all of the tests (npm run test), I get a failure in some test of the /access_token endpoint. I'm still tracking that down.
  • I've modified the flowmanager mock test for /ready; however, there is no similar test for /health. There never was. I am in the process of adding one.

Still, there are enough changes to begin a review.

klown added some commits Nov 28, 2018

GPII-3551: Improvements and bug fix for /ready and /health endpoints.
Fixed the bug in the /ready endpoint where the "_design/views" databse
path was improperly encoded as "_design%2Fviews".  Improved error
messages for flowmanager's and preferences server's "ready get"
handlers.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Nov 29, 2018

GPII-3551: Fixed usage of node's URL module.
Fixed to work with node tests and browser tests.
@cindyli

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

@klown, here are tests for /health endpoint: https://github.com/GPII/universal/blob/master/tests/platform/cloud/CloudStatusTests.js#L65-L83. Let me know if they are not what you meant for.

* @param {Component} dataSource - An instance of gpii.dbOperation.dbDataSource.
* @return {String} A sanatized version of the url.
*/
gpii.dbOperation.dbDataStore.sanitizeUrl = function (dataSource) {

This comment has been minimized.

Copy link
@cindyli

cindyli Nov 30, 2018

Contributor

When this kettle pull request fluid-project/kettle#49 is merged, the censoring of the sensitive information will be handled at the lower level in kettle.

It should be safe to directly return messages from kettle.

@amb26, can you confirm if this comment makes sense? Thanks.

This comment has been minimized.

Copy link
@amb26

amb26 Dec 1, 2018

Member

@cindyli - yes, that is correct - although the improvements in Kettle will only affect its own logging of the URLs. Given we independently log the URL in error messages here, it is reasonable to perform our own sanitisation.

This comment has been minimized.

Copy link
@klown

klown Dec 3, 2018

Author Contributor

The need for removing sensitive information before reporting URLs has come up before (e.g., deleting and loading the snapsets). It sounds like there is a need for a general function somewhere. Is Infusion too low level a place to put this (fluid.censorUrlCredentials())? If that's too low, maybe a function in the gpii namespace.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Nov 30, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2018

@klown, here are tests for /health endpoint: https://github.com/GPII/universal/blob/master/tests/platform/cloud/CloudStatusTests.js#L65-L83. Let me know if they are not what you meant for.

Thanks @cindyli, for two reasons.

First, at least one of those CloudStatusTests fails due to the changes for this JIRA, A fix is in the works.

Secondly, with respect to a test for the /health endpoint, they may be enough. What I noticed when modifying a /ready test inside the flowmanager's test suite, that there was no test for /health. In fact, that /ready test is for the flowmanager's prefsServerDataSource. I just wondered if there should be similar tests for the CBFM's /health and /ready endpoints in the flowmanager test folder.

@cindyli

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2018

Make sense. Please go ahead to add tests for /health endpoint in the flow manger test folder.

GPII-3551: Fixed cloud based flowmanager node test.
Updated test based on changes to what the /ready endpoint returns
on failure.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Nov 30, 2018

return promiseTogo;
};

/**
* Return a version of the database URL without showing senstive information,

This comment has been minimized.

Copy link
@amb26

amb26 Dec 1, 2018

Member

Typos senstive, sanatized

This comment has been minimized.

Copy link
@klown

klown Dec 3, 2018

Author Contributor

Thanks, @amb26.

@@ -93,7 +93,7 @@ gpii.tests.cloudStatus.testCases = {
name: "Test the liveness of the cloud based flow manager: all local with only the cloud based flow manager running",
url: "/ready",
config: gpii.tests.cloudStatus.configs.allLocalWithoutPrefsServer,
expectedStatusCode: 404
expectedStatusCode: 503

This comment has been minimized.

Copy link
@cindyli

cindyli Dec 3, 2018

Contributor

Please test response messages for all success and failed test cases. Thanks.

This comment has been minimized.

Copy link
@klown

klown Dec 4, 2018

Author Contributor

Done, but I'm not satisfied with the one error condition test. The CBFM isLive() invoker (in ReadyGetHandler.js) implies there are more error cases to check -- working on it.

GPII-3551: Updated cloud status tests
- Modified to check both status code and payloads.
- Fixed some typos.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Dec 4, 2018

klown added some commits Dec 6, 2018

GPII-3551: Modified /ready logic and updated tests accordingly
Database:
- if query successfully retrieves record, return it,
- if query succeeds but returns empty record, return undefined,
- if query fails, report failure.

Preferences service:
- if database query returns record, report isReady: true,
- if database query returns undefined, report isReady: false.
- if database query fails, report isReady: false.

Preferences server:
- if query reports isReady: true, report isReady: true,
- if query reports isReady: false, report error (503, problem with database)
- if query fails, report error (503, problem with database)

Flowmanager:
- if query reports isReady: true, report isReady: true,
- if query reports isReady: false, report error (503, problem with preferences server),
- if flowmanager and preferences server hosts are the same, report error (503, problem with preferences server),
- if query fails, report error (503, problem with preferences server)
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Dec 6, 2018

@klown

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

@cindyli Ready for review.

@@ -28,8 +28,8 @@ fluid.defaults("gpii.flowManager.cloudBased.ready.handler", {
gpii.flowManager.cloudBased.ready.handler.reportError = function (that, prefsServerURL) {
that.events.onError.fire({
isError: true,
message: "Cannot connect to Preferences Server at " + prefsServerURL,
statusCode: 404
message: "Error connecting to Preferences Server at " + prefsServerURL,

This comment has been minimized.

Copy link
@cindyli

cindyli Dec 11, 2018

Contributor

The preferences server is intended for internal use by CBFM and PPT etc. It won't be accessed by the public. I'm thinking for security reasons, it's probably better not to include prefs server URL in the response message, instead, before firing this event at line 29, use fluid.log() to log a detailed error, with the prefs server url, in the flow manager log file for developers and system admins.

This comment applies across the pull request for not exposing internal urls to the public, such as urls of prefs server and couchdb, but log them for debugging using fluid.log().

This comment has been minimized.

Copy link
@klown

klown Dec 11, 2018

Author Contributor

Okay, but that means we've had a security issue with the response error message -- I changed the status code only.

I will audit the code in this pull and make sure that couch and prefs urls are not exposed in responses. What about flow manager urls? Are they safe in the responses? I believe they are, but I'll check.

This comment has been minimized.

Copy link
@cindyli

cindyli Dec 11, 2018

Contributor

Yes, it was me who wrote this code that included the prefs server url in this response message. Sorry and thanks for fixing it.

The Cloud Based Flow manager URL should be OK to expose.

This comment has been minimized.

Copy link
@klown

klown Dec 12, 2018

Author Contributor

@cindyli, I found another case that exposes the URL. When running the code in the pull request for production configuration tests, the following error message is added to the payload when trying to update a snapset:

Cannot update: GPII key "testUser1" is a snapset while executing HTTP PUT on url http://preferences:9081/preferences/%gpiiKey?merge=%merge

I found the first part of that error message in preferencesService.js, namely this part: Cannot update: GPII key "testUser1" is a snapset, but I can't find where the URL is added.

Any ideas?

This comment has been minimized.

Copy link
@cindyli

cindyli Dec 12, 2018

Contributor

The second part is likely from this line in kettle.

This comment has been minimized.

Copy link
@klown

klown Dec 12, 2018

Author Contributor

Thanks! I used grep on the universal code, but didn't think to look inside the top-level 'node_modules' folder.

Changing something in kettle is beyond the scope of this JIRA/PR, IMO. Furthermore, the very next line in that kettle code removes the sensitive parts of the URL. I think we are fine with exposing the URL in this case.

I've made the other two changes, and haven't found any other sensitive URL exposure. What's left?

@@ -41,6 +41,7 @@ gpii.flowManager.cloudBased.ready.handler.isSameHost = function (flowManagerHost
// Remove the trailing slash if any
prefsServerHost = prefsServerHost.replace(/\/$/, "");
flowManagerHost = flowManagerHost.replace(/\/$/, "");
fluid.log("Preferences server: '", prefsServerHost, "', Flow manager: '", flowManagerHost, "'");

This comment has been minimized.

Copy link
@cindyli

cindyli Dec 11, 2018

Contributor

Is this line for debugging? If so, please remove it. Thanks.

This comment has been minimized.

Copy link
@klown

klown Dec 11, 2018

Author Contributor

Oops. Yes, for debugging -- thanks for catching that.

GPII-3551: Moved reporting of sensitive information into logs
Removed sensitive information from error messages in payloads and
put them into a fluid.log() message instead.
@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2018

@cindyli

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

Please update this pull request by merging in the master. Thanks.

@gpii-bot

This comment has been minimized.

Copy link
Collaborator

commented Dec 12, 2018

@cindyli cindyli merged commit 72d6487 into GPII:master Dec 12, 2018

1 check passed

default Build finished.
Details
@cindyli

This comment has been minimized.

Copy link
Contributor

commented Dec 12, 2018

Merged at 33e314e

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.