[services] Tunnelbroker - Skipping session checks in a stream when `skip_authentication` flag is set
ClosedPublic
Actions

Authored by • max on Jan 12 2023, 8:05 AM.

Details

Reviewers

• jon
bartek

Commits

rCOMMdc0769825150: [services] Tunnelbroker - Skipping session checks in a stream when…

Summary

This diff introduces changes to the bidirectional stream gRPC handler to skip session authentication checks by adding the sessions.skip_authentication config flag.
In case of the flag was provided we are skipping getting the session parameters from the database and expecting the device will be provided as metadata instead. Also, we are creating an empty SessionItem instance to provide compatibility with the following code in the handler. We are adding skipping of the changing of "Online" status for the session in case the flag is provided.

Linear task: ENG-2642

Test Plan

Add the skip_authentication flag to the [sessions] section of the config file.
Connect to the bidirectional messages stream by providing the deviceID parameter instead of sessionID.

The expected result is messages are delivered to the certain deviceID without prior session creation and authentication.

Diff Detail

Repository

rCOMM Comm

Branch

add-skip-session-check-in-stream

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Jan 12 2023, 8:05 AM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptJan 12 2023, 8:05 AM

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

• max added a parent revision: D6252: [services] Tunnelbroker - Expose `isConfigParameterSet` function to the Rust.Jan 12 2023, 8:06 AM

• max mentioned this in D6251: [services] Tunnelbroker - Add skip authentication config parameter.Jan 12 2023, 8:12 AM

• max mentioned this in D6252: [services] Tunnelbroker - Expose `isConfigParameterSet` function to the Rust.Jan 12 2023, 8:15 AM

Harbormaster completed remote builds in B15299: Diff 20874.Jan 12 2023, 8:20 AM

• max published this revision for review.Jan 12 2023, 8:21 AM

• max edited the summary of this revision. (Show Details)

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

• max added a reviewer: bartek. • max added 1 blocking reviewer(s): • jon.

otherwise looks good to me

services/tunnelbroker/src/server/mod.rs
138	Outside the context of this diff, but `SessionItem` sounds like a "one of many" object to me. `SessionContext` would be more accurate in describing its purpose, which is to hold some metadata about the current session.

This revision is now accepted and ready to land.Jan 12 2023, 11:24 AM

• jon added 1 blocking reviewer(s): bartek.Jan 12 2023, 11:24 AM

This revision now requires review to proceed.Jan 12 2023, 11:24 AM

bartek accepted this revision.Jan 12 2023, 11:42 AM

bartek added inline comments.

services/tunnelbroker/src/server/mod.rs
138–146	Nit: I'd extract this to an initializer function, sth like `SessionItem::new(device_id: String)`

This revision is now accepted and ready to land.Jan 12 2023, 11:42 AM

• max added a child revision: D6490: [services] Tunnelbroker - Add disable notifications config parameter.Feb 1 2023, 3:36 AM

• max mentioned this in rCOMMeee4ca130b65: [services] Tunnelbroker - Add skip authentication config parameter.Feb 1 2023, 3:46 AM

• max mentioned this in rCOMMf8a59aa046e2: [services] Tunnelbroker - Expose `isConfigParameterSet` function to the Rust.Feb 1 2023, 5:57 AM

Rebasing on the parent changes.

• max marked 2 inline comments as done.Feb 1 2023, 6:08 AM

• max added inline comments.

services/tunnelbroker/src/server/mod.rs
138	Outside the context of this diff, but `SessionItem` sounds like a "one of many" object to me. `SessionContext` would be more accurate in describing its purpose, which is to hold some metadata about the current session. That name came from the legacy C++ codebase and refactoring it seems doesn't makes sense now as we possibly switch to Rust totally.
138–146	Nit: I'd extract this to an initializer function, sth like `SessionItem::new(device_id: String)` Thanks, @bartek! But this part is a single usage (for our testing Tunnelbroker usage as Redis replacement only) and adding the initializer for that single usage seems maybe overengineering to me.