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.

Sign up for GitHub

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

Jump to bottom

tvOS Device Authorization Flow #26

Merged
merged 51 commits into from
Apr 3, 2024
Merged

tvOS Device Authorization Flow #26

merged 51 commits into from
Apr 3, 2024

Conversation

bleege
Copy link
Contributor

@bleege bleege commented Nov 5, 2023 •
edited by dcaunt
Loading

Adds tvOS support for OAuth 2.0 Device Authorization Grant to the SDK.

To run the tvOS sample app and test Device Auth, you must change the OpenPassClientId in Info.plist to 51c42041a7de48f59bff4f8a8a6ad18b.

  • Adds a new OpenPassManager property deviceAuthorizationFlow for initiating the flow.
  • Update sample app for tvOS to use Device Auth Flow
  • Run tests on tvOS simulator (previously was build-only)

@bleege bleege self-assigned this Nov 5, 2023
@bleege bleege force-pushed the bwl-TTDCP-4198-device-flow branch from a052fce to cfe24a1 Compare November 8, 2023 20:21
@bleege bleege marked this pull request as ready for review November 13, 2023 21:55
@bleege bleege requested a review from IanDBird November 14, 2023 14:22
dcaunt added 5 commits March 25, 2024 19:48
@dcaunt
Merge branch 'main' into bwl-TTDCP-4198-device-flow
25e2905 
# Conflicts:
#	Development/OpenPassDevelopmentApp/Info.plist
#	Development/OpenPassDevelopmentApp/Localizable.strings
#	Development/OpenPassDevelopmentApp/OpenPassTokensView.swift
#	Development/OpenPassDevelopmentApp/RootView.swift
#	Development/OpenPassDevelopmentApp/RootViewModel.swift
#	Sources/OpenPass/OpenPassClient.swift
#	Sources/OpenPass/OpenPassManager.swift
#	Sources/OpenPass/ResponseData/OpenPassTokensResponse.swift
#	Tests/OpenPassTests/OpenPassClientTests.swift
@dcaunt
Refactor Flow for async await
ad0bad5
@dcaunt
Restore client ID
c2cb077
@dcaunt
Add manager test, restore client tests
9ca6921
@dcaunt
Clean up comments
7655b64
IanDBird
IanDBird requested changes Mar 29, 2024
View reviewed changes
Copy link
Collaborator

@IanDBird IanDBird left a comment

Choose a reason for hiding this comment

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

One other observation is that by default, we are using the production API (fine) but also a client id that isn't allowed to use Device Auth Flow (since it's not a TV app). I'm assuming this is because we have a single app, so single plist and therefore only a single client id can be extracted. I wonder if this limitation is something we should revisit? Something to discuss anyway

Development/OpenPassDevelopmentApp/DeviceAuthorizationViewModel.swift Outdated Show resolved Hide resolved
Development/OpenPassDevelopmentApp/DeviceAuthorizationViewModel.swift Outdated Show resolved Hide resolved
Development/OpenPassDevelopmentApp/DeviceAuthorizationViewModel.swift Show resolved Hide resolved
Development/OpenPassDevelopmentApp/DeviceAuthorizationViewModel.swift Show resolved Hide resolved
Sources/OpenPass/DeviceAuthorizationFlow.swift Show resolved Hide resolved
Sources/OpenPass/DeviceAuthorizationFlow.swift Outdated Show resolved Hide resolved
Sources/OpenPass/OpenPassManager.swift Show resolved Hide resolved
Sources/OpenPass/Request.swift Show resolved Hide resolved
Tests/OpenPassTests/DeviceAuthorizationFlowTests.swift Outdated Show resolved Hide resolved
@dcaunt dcaunt force-pushed the bwl-TTDCP-4198-device-flow branch 3 times, most recently from 7727830 to 0de5f59 Compare April 3, 2024 10:48
@dcaunt dcaunt force-pushed the bwl-TTDCP-4198-device-flow branch from 0de5f59 to 389459c Compare April 3, 2024 11:08
@dcaunt
Copy link
Contributor

dcaunt commented Apr 3, 2024

One other observation is that by default, we are using the production API (fine) but also a client id that isn't allowed to use Device Auth Flow (since it's not a TV app). I'm assuming this is because we have a single app, so single plist and therefore only a single client id can be extracted. I wonder if this limitation is something we should revisit? Something to discuss anyway

I've added a note to Development/README.md to cover this, and make it easy to toggle the ID. In the future if we change how the SDK is configured, or split the Dev app, I think we can revisit this.

IanDBird
IanDBird previously approved these changes Apr 3, 2024
View reviewed changes
Copy link
Collaborator

@IanDBird IanDBird left a comment

Choose a reason for hiding this comment

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

Just a minor nit about some, now outdated, documentation.

Sources/OpenPass/DeviceAuthorizationFlow.swift Show resolved Hide resolved
IanDBird
IanDBird approved these changes Apr 3, 2024
View reviewed changes
Copy link
Collaborator

@IanDBird IanDBird left a comment

Choose a reason for hiding this comment

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

LGTM

@dcaunt dcaunt merged commit 3d1834a into main Apr 3, 2024
1 check passed
@dcaunt dcaunt deleted the bwl-TTDCP-4198-device-flow branch April 3, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcaunt dcaunt dcaunt left review comments

@christriddle-thetradedesk christriddle-thetradedesk christriddle-thetradedesk left review comments

@IanDBird IanDBird IanDBird approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bleege @dcaunt @IanDBird @christriddle-thetradedesk