This repository has been archived by the owner on May 22, 2020. It is now read-only.
RUN-3120 prevent API calls from unauthenticated WS connections #123
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
whyn07m3
requested review from
HarsimranSingh,
rdepena,
datamadic,
joneit,
wenjunche and
nicholasdgoodman
HarsimranSingh
suggested changes
Aug 7, 2017
|
|
||
| // Prevent all API calls from unauthenticated external connections, | ||
| // except for authentication APIs | ||
| if ( |
There was a problem hiding this comment.
this if statement looks kinda ugly. Maybe move the conditions out into variables with descriptive names.
There was a problem hiding this comment.
Done it this way initially to short-circuit checks, but agree with clarity over cleverness.
There was a problem hiding this comment.
@HarsimranSingh after taking a look at this again, after Ricardo pointed it out, one of the calls here is an expensive one, and we have to give up clarity here a bit
HarsimranSingh
previously approved these changes
Aug 7, 2017
rdepena
suggested changes
Aug 7, 2017
| }; | ||
|
|
||
| module.exports.registerMiddleware = function(requestHandler) { | ||
| requestHandler.addPreProcessor(isConnectionAuthenticated); |
There was a problem hiding this comment.
this code executes for every request, can we short circuit it early, before we get the connectionByUuid.
Something like:
if (isExternalConnection) {
next();
} else {
//check if is authenticated and either next or nack.
}
There was a problem hiding this comment.
brought short-circuiting back
|
✅ Test results: |
|
👍 |
rdepena
approved these changes
Aug 7, 2017
pbaize
pushed a commit
to pbaize/core
that referenced
this pull request
Oct 18, 2019
* RUN-3996 fixed a multi-runtime regression (HadoukenIO#109) * V bump.30.2 (HadoukenIO#110) (HadoukenIO#111) * RUN-3996 fixed a multi-runtime regression (HadoukenIO#109) * Version bump to 30.2 * RUN-3999 multi runtime tests config update (HadoukenIO#112) * V bump.30.2 (HadoukenIO#110) * RUN-3996 fixed a multi-runtime regression (HadoukenIO#109) * Version bump to 30.2 * RUN-3999 Update multi runtime tests config * Bug/run 3965 multi runtime cache clearing issue (HadoukenIO#114) * V bump.30.2 (HadoukenIO#110) * RUN-3996 fixed a multi-runtime regression (HadoukenIO#109) * Version bump to 30.2 * RUN-3965 Fix EPERM: operation not permitted * RUN-3965 Fix some cache folders not deleted issue * RUN-4017/services (HadoukenIO#113) * passing tests * made changes * RUN-4028 - Async add listener (HadoukenIO#115) * V bump.30.2 (HadoukenIO#110) * RUN-3996 fixed a multi-runtime regression (HadoukenIO#109) * Version bump to 30.2 * some test fixes * more timeouts in tests * altered tests to check for awaiting newlistener * undo unnecessary edit * fix types of registereventlistener and unregister * hold, fixed removeAllListeners but need to test * added some tests * added testing * cleanup * fix prependonce * bit more testing * revert test changes * changed class name, some cleanup * make tests match new best practices * name of test file * tests cleanup * back to arrows for tslint * bind rawsend (HadoukenIO#116) * RUN-4045 register window name early in creation (HadoukenIO#117) * RUN-4033 plugin.import accepts a string (HadoukenIO#118) * Feature/test sledghammer (HadoukenIO#119) * sledghammer approach to fixing tests. * taking a sledghammer to our Multi runtime tests. * Moving back to es6 untill we hit a major version. * A few items from the code review. * code review items. * code review items. * V bump.30.2 (HadoukenIO#110) (HadoukenIO#121) * RUN-3996 fixed a multi-runtime regression (HadoukenIO#109) * Version bump to 30.2 * RUN-3898 - External services tests (HadoukenIO#120) * hiold * hold * tests for external services re RUN-3898 * clean up * Test fixed, no longer leaving around runtime (HadoukenIO#122) * test fixed, no longer leaving around runtime * cleanup * Fixed issue with the webpack grunt task where it was not reporting the errors correctly (HadoukenIO#123) * Fixed issue with the webpack grunt task where it was not reporting the errors correctly * Passing error codes instead of arrays. * version bump. (HadoukenIO#124)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ℹ️ Adds auth middleware that prevents unauthenticated WebSocket connections making API calls.
➕ New Test
• WS API call w/o authentication
/api_handlers/authorization.jsfile looks crazy (because of indentation changes throughout the file), but the only changes to that file are:initfunction in place of previously having to instantiate classisConnectionAuthenticatedand its registration inregisterMiddleware✅ Test Results (ignore new multi-preload scripts test errors, they are invalid)
• Windows 7
• Windows 10
• Node-adapter all (84) tests passed
• Java-adapter all tests passed