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

Add support for subpaths in Auth0 domain [SDK-2963] #557

Merged
merged 9 commits into from
Nov 30, 2021
Merged

Conversation

Widcket
Copy link
Contributor

@Widcket Widcket commented Nov 29, 2021

Changes

This PR adds support for subpaths in the Auth0 domain, by removing the leading slash from all the paths and adding a trailing slash to the base URL.

⚠️ THIS PR CONTAINS BREAKING CHANGES: an URL extension has been made internal (it was public), as it has no use for developers.

References

Fixes #545

Testing

Unit tests have been added, and the changes were also tested manually (for Web Auth only) with an iOS 15.0 emulator (iPhone 13) and macOS 11.6.1, using the respective playground apps.

Screen Shot 2021-11-29 at 18 43 21

Screen Shot 2021-11-29 at 18 04 08

  • This change adds unit test coverage
  • This change has been tested on the latest version of the platform/language or why not

Checklist

@Widcket Widcket requested a review from a team as a code owner November 29, 2021 23:05
@Widcket Widcket added the medium label Nov 29, 2021
@@ -1,20 +1,17 @@
import Foundation

public extension URL {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the breaking change.

adamjmcgrath
adamjmcgrath previously approved these changes Nov 30, 2021
@@ -66,7 +66,7 @@ class ClaimValidatorsSpec: IDTokenValidatorBaseSpec {
describe("iss validation") {

var issValidator: IDTokenIssValidator!
let expectedIss = "\(URL.a0_url(domain).absoluteString)/"
let expectedIss = "\(URL.httpsURL(from: domain).absoluteString)/"
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it's breaking any of your tests, but this will end in //

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, I'll look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed.

Screen Shot 2021-11-30 at 13 47 34

It does not break any tests because that constant is only used for checking presence and non-nil.

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've removed the extra / in 68be1ea.

@Widcket Widcket merged commit 49309e6 into beta Nov 30, 2021
@Widcket Widcket deleted the v2/domain-subpaths branch November 30, 2021 21:54
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

2 participants