[services] Tunnelbroker - Skipping `deviceID` validation in tools function
ClosedPublic
Actions

Authored by • max on Feb 2 2023, 4:50 AM.

Details

Reviewers

• jon
bartek

Commits

rCOMM044b8239d2c4: [services] Tunnelbroker - Skipping `deviceID` validation in tools function

Summary

This diff adds skipping of the deviceID format validation in the validateDeviceID tools function by checking the sessions.disable_deviceid_validation config flag (from D6523).

We are disabling the validation of the deviceID format for using the Tunnelbroker as a Redis replacement, in this case, the deviceID format will be different and we should not validate it.

Linear task: ENG-2640

Test Plan

CI checks are successfully passed.
Manual testing:

Adding the disable_deviceid_validation flag to the [sessions] section of the config file results in always true returns from the validateDeviceID tools function.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Feb 2 2023, 4:50 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptFeb 2 2023, 4:50 AM

Herald added subscribers: atul, tomek, ashoat. · View Herald Transcript

• max added a parent revision: D6523: [services] Tunnelbroker - Add disable `deviceID` format validation config parameter.Feb 2 2023, 4:50 AM

• max edited the summary of this revision. (Show Details)Feb 2 2023, 4:59 AM

• max edited the test plan for this revision. (Show Details)

• max added a reviewer: bartek.

• max added inline comments.

services/tunnelbroker/src/libcpp/src/Tools/Tools.cpp
30 ↗	(On Diff #21843)	Logging here in case we are skipping the validation is a good debugging point.

• max published this revision for review.Feb 2 2023, 5:00 AM

Harbormaster completed remote builds in B15999: Diff 21843.Feb 2 2023, 5:04 AM

• jon accepted this revision.Feb 2 2023, 10:18 AM

This revision is now accepted and ready to land.Feb 2 2023, 10:18 AM

ashoat added inline comments.Feb 2 2023, 11:44 AM

services/tunnelbroker/src/libcpp/src/Tools/Tools.cpp
32 ↗	(On Diff #21843)	`deviceID` is user-identifiable... this creates a log of all the times a given user has accessed Tunnelbroker. I'd rather we didn't implicitly create access logs like this. Is `LOG(INFO)` going to be active in production?

Removing logging.

• max marked an inline comment as done.Feb 15 2023, 4:00 PM

• max added inline comments.

services/tunnelbroker/src/libcpp/src/Tools/Tools.cpp
32 ↗	(On Diff #21843)	`deviceID` is user-identifiable... this creates a log of all the times a given user has accessed Tunnelbroker. I'd rather we didn't implicitly create access logs like this. Makes sense to remove this logging. Is `LOG(INFO)` going to be active in production? No, `ERROR` and `FATAL` are going to be printed in production.