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

feat: typed publications in gateways/peripheraldevices SOFIE-1183 #1056

Merged
merged 3 commits into from
Nov 10, 2023

Conversation

Julusian
Copy link
Member

@Julusian Julusian commented Oct 26, 2023

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature

  • What is the current behavior? (You can also link to an open issue here)

Subscribing to publications in peripheraldevices is loosely typed, and requires each gateway to know the type of each collection it is accessing.

  • What is the new behavior (if this is a feature change)?

the PubSub enum has been split up to reflect what is available at different levels, and each gateway is able to define what subset it wants to access, resulting in the correct typings being available for publications.
One portion resides in shared-lib, and is available to all peripheral devices (and is the default, unless they specify another set).
Another portion resides in corelib, and contains publications for the Rundown/RundownPlaylist and its contents. This is intended to be used by the live-status gateway.
The final portion resides in the old location, and is intended to be used solely by the Meteor UI. The Meteor UI also has access to the rest of the publication types.

As part of this, some typings had to be adjusted inside LSG to make the compiler happy, resulting in it having some more usage of our id types, and some additional generics to get the publications typed properly.

Additionally, the observe method in server-core-integration has been made typed to match the collections. This did not have much impact and has improved type safety

  • Other information:

I have prototyped the required changes for package-manager and input-gateway, PRs will be raised when this gets merged.

Status

  • Code documentation for the relevant parts in the code have been added/updated by the PR author
  • The functionality has been tested by the PR author
  • Automated tests to cover the new functionality and/or guard against regressions have been added
  • The functionality has been tested by NRK

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 328 lines in your changes are missing coverage. Please review.

Comparison is base (cbc551a) 58.20% compared to head (ef22d87) 58.36%.
Report is 13 commits behind head on release51.

Additional details and impacted files
@@              Coverage Diff              @@
##           release51    #1056      +/-   ##
=============================================
+ Coverage      58.20%   58.36%   +0.15%     
=============================================
  Files            507      507              
  Lines          81414    81475      +61     
  Branches        4279     4398     +119     
=============================================
+ Hits           47391    47552     +161     
+ Misses         33965    33877      -88     
+ Partials          58       46      -12     
Files Coverage Δ
meteor/server/api/rest/v0/index.ts 93.93% <100.00%> (+0.39%) ⬆️
packages/server-core-integration/src/index.ts 100.00% <100.00%> (ø)
...kages/server-core-integration/src/lib/ddpClient.ts 90.17% <100.00%> (-0.05%) ⬇️
...teor/client/lib/reactiveData/reactiveDataHelper.ts 71.69% <50.00%> (ø)
meteor/server/lib/customPublication/publish.ts 66.98% <90.00%> (+0.63%) ⬆️
...publications/blueprintUpgradeStatus/publication.ts 23.28% <66.66%> (ø)
...r/server/publications/peripheralDeviceForDevice.ts 53.74% <85.71%> (+0.61%) ⬆️
...ver/publications/segmentPartNotesUI/publication.ts 65.84% <66.66%> (ø)
.../server-core-integration/src/lib/coreConnection.ts 83.44% <96.77%> (+0.48%) ⬆️
meteor/client/ui/i18n.ts 0.00% <0.00%> (ø)
... and 26 more

... and 26 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Julusian Julusian force-pushed the feat/typed-gateway-publications2 branch from 8fc02e9 to e9286d6 Compare October 30, 2023 10:55
@Julusian Julusian marked this pull request as ready for review October 30, 2023 10:59
@Julusian Julusian requested a review from a team October 30, 2023 10:59
@Julusian Julusian changed the title feat: typed publications in gateways/peripheraldevices feat: typed publications in gateways/peripheraldevices SOFIE-1183 Oct 30, 2023
meteor/server/api/rest/v0/index.ts Outdated Show resolved Hide resolved
meteor/server/publications/_publications.ts Outdated Show resolved Hide resolved
meteorPublish(
PeripheralDevicePubSub.expectedPlayoutItemsForDevice,
async function (deviceId: PeripheralDeviceId, token: string | undefined) {
if (!deviceId) throw new Meteor.Error(400, 'deviceId argument missing')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this publication use a better authentication? Like in PeripheralDevicePubSub.rundownsForDevice?

https://github.com/nrkno/sofie-core/pull/1056/files#diff-0a1e64f58116bd28a7682f7327410f9d24631a4b0c877257e54eeecb917cb4f6R59

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this publication can do anything else. The mongo collection doesnt have an organisationId, the only filter we can use is for the studio.
The peripheralDeviceContent check seems to be the most relevant thing to check here, as it is documented as Check for read access for all peripheraldevice content (commands, mediaWorkFlows, etc..)
But I didn't put that much thought into it, as I know that if anyone wants to enable the security, various other methods need fixing to work at all, and the rest need auditing as they can leak data in various ways.

In fact, I would question if rundownsForDevice is a good example. The deviceId parameter there is not even being used

packages/corelib/src/pubsub.ts Show resolved Hide resolved
packages/playout-gateway/src/coreHandler.ts Show resolved Hide resolved
packages/corelib/src/pubsub.ts Outdated Show resolved Hide resolved
packages/server-core-integration/README.md Outdated Show resolved Hide resolved
packages/server-core-integration/README.md Outdated Show resolved Hide resolved
@Julusian Julusian requested a review from nytamin November 8, 2023 17:13
@Julusian Julusian merged commit 0c3c1bf into release51 Nov 10, 2023
65 of 66 checks passed
@Julusian Julusian deleted the feat/typed-gateway-publications2 branch November 10, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants