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

As web authentication presentation context providing #327

Conversation

james-rant
Copy link
Contributor

Since iOS 13, ASWebAuthenticationSession requires that a presentation context is provided in order to present the authentication UI. This context (ASWebPresentationAnchor) is typealiased to UIWindow on iOS.

This PR implements the necessary provider by returning the authorizeContext from OAuath2Config.

I also return the error.asOAuth2Error in the completion handler, as without it the web authentication session was failing silently on iOS 13.

Since iOS 13, ASWebAuthenticationSession requires that a presentation context is provided in order to present the authentication UI. This context (`ASWebPresentationAnchor`) is typealiased to `UIWindow` on iOS.

This commit implements the necessary provider by returning the `authorizeContext` from `OAuath2Config`.
Copy link
Contributor

@larrybrunet larrybrunet left a comment

Choose a reason for hiding this comment

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

Seems incomplete - where are the changes to authConfig?

Documentation should be added for the new field.

@james-rant
Copy link
Contributor Author

james-rant commented Oct 24, 2019

The only new field I added was

/// Used to store the ASWebAuthenticationPresentationContextProvider
private var webAuthenticationPresentationContextProvider: AnyObject?

Which I added a doc line for the same as the other properties in that class, but it's private, so shouldn't need too much documentation as it's not part of the public API that users would interact with.

There were no changes necessary to the authConfig, as this uses the already existing authorizeContext.

This PR just exposes the authorizeContext to the new iOS 13 API which required a context to be passed.

Copy link
Contributor

@larrybrunet larrybrunet left a comment

Choose a reason for hiding this comment

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

Approved - I'm facing the same issue when building with iOS 13 SDK.

@fotiDim
Copy link
Contributor

fotiDim commented Oct 30, 2019

I am getting some errors:
"generateCodeVerifier' is inaccessible due to 'internal' protection level"
"codeChallenge' is inaccessible due to 'internal' protection level"

It would be nice to get this fixed and merged soon

EDIT: This error only happens when I point Swift Package Manager to Jame's repo and branch. If I point it to the latest version (5.1) it works. Not sure why Swift Package Manager complains about it

@james-rant
Copy link
Contributor Author

james-rant commented Oct 30, 2019

@fotiDim This PR is branched from master. the 5.1 release is a couple of commits behind master. One of the commits since the 5.1 release mentions swift PM config changes. That's probably your issue and I don't believe it's related to this PR itself.

@fotiDim
Copy link
Contributor

fotiDim commented Oct 30, 2019

@james-rantmedia can you merge upstream master on your branch so that I can test?

@fotiDim
Copy link
Contributor

fotiDim commented Oct 30, 2019

@james-rantmedia actually I just forked the repo and tried to point SPM to my master and it is failing as well. Apparently unrelated to this PR but still interesting why it happens.

Screenshot 2019-10-30 at 22 04 09

@fotiDim
Copy link
Contributor

fotiDim commented Oct 30, 2019

@james-rantmedia @larrybrunet I narrowed it down being the 36b154d5aae2d7ae121b8bdafbf86324ac5dc499 commit. The previous commit works just fine. Shall we open a new issue for this?

@p2 p2 force-pushed the master branch 2 times, most recently from 34b4fe2 to 5e2f70d Compare November 17, 2019 00:05
@ossus-lib
Copy link
Collaborator

Version 5.1.1 has these errors fixed!

@ernestoSk13
Copy link

Hi, is anybody checking this issue? seems like version 5.1.1 is not working, it failed in the CI

@ossus-lib
Copy link
Collaborator

@ernestoSk13 that issue had been fixe, it was just the Travis config that we needed to update.

@ykphuah
Copy link

ykphuah commented Apr 16, 2020

I managed to get this branch to work, however, there's a fix needed for my code, before this, when I call authorizeEmbedded, I pass in the UIViewController as "from" parameter, which if I do useAuthenticationSession, it will complain that it is not a UIWindow.

So I need to pass in self.view.window as the "from" parameter, then it will work. I suggest to make this automatic in the pull request, so that passing either UIViewController or UIWindow will both work.

@gonzag
Copy link

gonzag commented May 4, 2020

I faced situations where the authorizeContext is nil in presentationAnchor
Instead of crashing I've opted for return ASPresentationAnchor():

public func presentationAnchor(for session: ASWebAuthenticationSession) -> ASPresentationAnchor {
		guard let context = authorizer.oauth2.authConfig.authorizeContext as? ASPresentationAnchor else {
			return ASPresentationAnchor()
		}
		
		return context
	}

@ykphuah
Copy link

ykphuah commented May 8, 2020

Actually it is not nil, its just not a ASPresentationAnchor which is UIWindow.

@@ -142,9 +145,10 @@ open class OAuth2Authorizer: OAuth2AuthorizerUI {
self.oauth2.logger?.warn("OAuth2", msg: "Cannot intercept redirect URL: \(err)")
}
} else {
self.oauth2.didFail(with: nil)
self.oauth2.didFail(with: error?.asOAuth2Error)
Copy link

Choose a reason for hiding this comment

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

I think that the oauth2 depends on this to be nil in order to process and provide OAuth2Error.requestCancelled error.
Please see Oauth2Base.didFail(with:) method for details.

@ossus-lib ossus-lib merged commit 239fe71 into p2:master Jan 2, 2021
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.

8 participants