Support https redirect URIs in browser sign-in - #252
Conversation
> [!WARNING] > These changes require that the server resource is configured to support associated domains for this application.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for HTTPS redirect URIs in browser sign-in by leveraging the newer ASWebAuthenticationSession API available on iOS 17.4+. The changes enhance the authentication flow to handle both custom scheme and HTTPS redirect URIs while maintaining backward compatibility.
- Adds a new
createSessionmethod that selects the appropriate ASWebAuthenticationSession initializer based on platform availability and redirect URI scheme - Updates error handling to include more detailed cancellation reasons
- Improves sample app UI state management during sign-in flow
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/BrowserSignin/Providers/AuthenticationServicesProvider.swift | Core implementation for HTTPS redirect URI support with platform availability checks |
| Sources/BrowserSignin/BrowserSignin.swift | Updated documentation and error case to support cancellation reasons |
| Sources/BrowserSignin/Internal/BrowserSigninError+Extensions.swift | Enhanced error handling for user cancellation with optional reason parameter |
| Sources/BrowserSignin/Resources/en.lproj/BrowserSignin.strings | Added new localized string for detailed cancellation error messages |
| Samples/WebSignIn/WebSignIn (macOS)/WebSignIn/SignIn/SignInView.swift | Improved state management and UI control during sign-in process |
| Samples/WebSignIn/WebSignIn (iOS)/WebSignIn/SignInViewController.swift | Added button state management to prevent multiple concurrent sign-in attempts |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| options.contains(.ephemeralSession) | ||
| } | ||
| set { | ||
| options.insert(.ephemeralSession) |
There was a problem hiding this comment.
Is this a correct logic of property?
There was a problem hiding this comment.
🤦🏼♀️ Yes, you're absolutely right. I have this changed locally, but it didn't get pushed up to the PR.
There was a problem hiding this comment.
@IldarAbdullin-okta I included tests as well in my update, since I feel sheepish for missing it 😆
* Support https redirect URIs in browser sign-in * Improve browser sign in error messaging * Expose the browser providers and their factory for extension and testing > [!WARNING] > These changes require that the server resource is configured to support associated domains for this application.
* Support https redirect URIs in browser sign-in (#252) * Improve browser sign in error messaging * Fix import statements for other platforms (#256) * Fix problems with BrowserSignin on tvOS (#259) * Bump version numbers and include additional docs * Expose the browser providers and their factory for extension and testing > [!WARNING] > These changes require that the server resource is configured to support associated domains for this application.
* Support https redirect URIs in browser sign-in (#252) * Improve browser sign in error messaging * Fix import statements for other platforms (#256) * Fix problems with BrowserSignin on tvOS (#259) * Bump version numbers and include additional docs * Expose the browser providers and their factory for extension and testing > [!WARNING] > These changes require that the server resource is configured to support associated domains for this application.
Note
These changes require that the server resource is configured to support associated domains for this application.
Without associated domain configuration set, a developer would receive the following error response: