p2 / OAuth2

OAuth2 framework for macOS and iOS, written in Swift.
Other
1.14k stars 276 forks source link

As web authentication presentation context providing #327

Closed james-rant closed 3 years ago

james-rant commented 4 years ago

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.

james-rant commented 4 years ago

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.

fotiDim commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

fotiDim commented 4 years ago

@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 commented 4 years ago

@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?

ossus-lib commented 4 years ago

Version 5.1.1 has these errors fixed!

ernestoSk13 commented 4 years ago

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

ossus-lib commented 4 years ago

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

ykphuah commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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