Create an endpoint to change the default port for new access keys #461
Conversation
|
Responded to review comments, PTAL |
…ohenjon-port-hostname
…ne-server into cohenjon-port-hostname
|
Ping on this ^_^ waiting for some answers before doing another round of review response. |
|
Friendly ping |
| } catch (error) { | ||
| logging.error(error); | ||
| if (error instanceof errors.InvalidPortNumber) { | ||
| return next(invalidPortArgument(error.message)); |
There was a problem hiding this comment.
Could you use resitfy.BadRequestError here instead? It makes it easier to see what HTTP error is being returned.
There was a problem hiding this comment.
That's what invalidPortArgument does, I just moved it into a function since there are a few different places dealing with invalid port numbers. Made the function local to setPortForNewAccessKeys so it's closer to the use
There was a problem hiding this comment.
I'd still like to be able to easily see what the HTTP error we are returning, since that's part of the API.
Changing the calls to BadRequestError would make that clearer, I don't think the duplication is a big deal.
Maybe you want a IsValidPort method that validates the port and raises InvalidPortNumber. That would remove the duplication and improve readability.
| logging.debug(`setPort[ForNewAccessKeys request ${JSON.stringify(req.params)}`); | ||
| if (!req.params.port) { | ||
| return next( | ||
| invalidPortArgument(`Expected a port argument but found none. Request: ${req}`)); |
There was a problem hiding this comment.
Remove the request from the reason message. No need to send it back to the user.
Maybe also use more direct text: Parameter port is missing
|
|
||
| const port = req.params.port; | ||
| if (typeof port !== 'number') { | ||
| return next(invalidPortArgument(`Expected an numeric port, instead got ${port}`)); |
There was a problem hiding this comment.
got ${port} of type ${typeof port}, since the type is the sticking point.
| } catch (error) { | ||
| logging.error(error); | ||
| if (error instanceof errors.InvalidPortNumber) { | ||
| return next(invalidPortArgument(error.message)); |
There was a problem hiding this comment.
I feel a little nervous about forwarding error messages like that. It's a good way to leak potentially sensitive information. It's ok to log, but it's risky to send arbitrary messages back to the client.
Maybe rename the message field to reason or explanation, so it's more restricted in its semantics.
There was a problem hiding this comment.
This isn't an arbitrary message, it's from InvalidPortNumber which is only ever made by us and has the stringified port number as the message. I can't rename the message field since it's part of the Error interface.
Are you worried that somehow this response could expose the manager server to a probing attacker?
|
Can you give an example of the output in case of API error? |
Adds endpoints and implementations
Adds tests for new api
Adds tests for newly public functions from get_port
Cleans up some deprecated code from before multiplexing was enabled.
Cleans up some unused imports in tests.
Creates and uses a builder class for the ServerAccessRepository unit tests