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(HTTPSend): Add OAuth 2.0 and Bearer Token options #329

Merged

Conversation

ianshade
Copy link
Contributor

@ianshade ianshade commented May 24, 2024

About the Contributor

This PR is opened on behalf of TV 2 Norge,

Type of Contribution

This is a:

Feature

Current Behavior

Unless handled completely outside of the TSR, requests made through HTTPSend can't be authorized.

New Behavior

Two authorization methods are supported in the HTTPSend integration:

  • OAuth 2.0 Client Credentials Flow: when configured, credentials are exchanged for a Bearer token that is added to EVERY outgoing request made through this instance of HTTPSend, and automatically refreshed.
  • Long-lived Bearer token that is added to EVERY outgoing request. (probably not that common, but can be useful for testing purposes)

Testing Instructions

Other Information

Status

  • PR is ready to be reviewed.
  • The functionality has been tested by the author.
  • Relevant unit tests has been added / updated.
  • Relevant documentation (code comments, system documentation) has been added / updated.

@codecov-commenter
Copy link

codecov-commenter commented May 24, 2024

Codecov Report

Attention: Patch coverage is 3.22581% with 60 lines in your changes missing coverage. Please review.

Project coverage is 37.36%. Comparing base (c782645) to head (76592f2).

Files Patch % Lines
...tegrations/httpSend/AuthenticatedHTTPSendDevice.ts 0.00% 59 Missing ⚠️
...ges/timeline-state-resolver/src/service/devices.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##           release50     #329      +/-   ##
=============================================
- Coverage      37.58%   37.36%   -0.22%     
=============================================
  Files            106      107       +1     
  Lines          10247    10306      +59     
  Branches        2452     2511      +59     
=============================================
  Hits            3851     3851              
- Misses          6395     6454      +59     
  Partials           1        1              

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

@ianshade ianshade marked this pull request as ready for review June 12, 2024 20:59
@jstarpl jstarpl requested a review from a team June 13, 2024 05:27
},
"oauthTokenHost": {
"type": "string",
"ui:title": "OAuth 2.0 Token Host",
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be useful to specify that this is the "base URL" of the oauth flow, i.e. the way the "token host" is described in the simple-oauth2: "Base URL used to obtain access tokens". I don't think "token host" is a widely used term in the OAuth world. Also, specifying that this is in fact a URL (and not, say a hostname + path) would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made changes to the UI descriptions of this setting, and I added another field to allow specifying the path

@jstarpl jstarpl self-assigned this Jun 17, 2024
@jstarpl
Copy link
Member

jstarpl commented Jun 17, 2024

Hello!

Thank you for contributing to the Sofie Project!

If you haven’t already, please give our contribution guidelines a read. I have found a a single usability-related issue, with the term "token host" that I would argue should be called something that contains a URL to specify that this is in fact a base URL for the OAauth flow. Once that's addressed, it should be merged within a few days.

@ianshade ianshade requested a review from jstarpl June 17, 2024 12:30
@ianshade ianshade force-pushed the contribute/EAV-243/http-oauth branch from 92af351 to 76592f2 Compare June 17, 2024 12:32
@jstarpl
Copy link
Member

jstarpl commented Jun 18, 2024

Since release50 is feature-frozen and this is a new feature, I'll redirect this PR towards release51.

@jstarpl jstarpl changed the base branch from release50 to release51 June 18, 2024 08:35
@jstarpl jstarpl changed the base branch from release51 to release50 June 18, 2024 08:56
@jstarpl jstarpl changed the base branch from release50 to release51 June 18, 2024 08:56
@jstarpl jstarpl merged commit 1517e68 into nrkno:release51 Jun 18, 2024
17 of 18 checks passed
@ianshade ianshade deleted the contribute/EAV-243/http-oauth branch June 18, 2024 12:59
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.

None yet

3 participants