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

Implement OIDC RP-Initiated Logout #259

Merged
merged 7 commits into from
Jun 11, 2019
Merged

Implement OIDC RP-Initiated Logout #259

merged 7 commits into from
Jun 11, 2019

Conversation

WilliamDenniss
Copy link
Member

@codecov-io
Copy link

codecov-io commented Jun 26, 2018

Codecov Report

Merging #259 into master will decrease coverage by 1.67%.
The diff coverage is 44.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #259      +/-   ##
==========================================
- Coverage   73.93%   72.25%   -1.68%     
==========================================
  Files          58       63       +5     
  Lines        5102     5374     +272     
==========================================
+ Hits         3772     3883     +111     
- Misses       1330     1491     +161
Impacted Files Coverage Δ
Source/OIDAuthorizationService.h 0% <ø> (ø) ⬆️
Source/OIDEndSessionResponse.m 0% <0%> (ø)
Source/OIDEndSessionResponse.h 0% <0%> (ø)
Source/OIDEndSessionRequest.h 100% <100%> (ø)
Source/OIDServiceConfiguration.h 100% <100%> (ø) ⬆️
Source/OIDServiceDiscovery.m 93.5% <100%> (-0.13%) ⬇️
UnitTests/OIDEndSessionRequestTests.m 100% <100%> (ø)
UnitTests/OIDServiceDiscoveryTests.m 84.61% <100%> (-0.34%) ⬇️
Source/OIDAuthorizationService.m 53.86% <12.08%> (-8.7%) ⬇️
Source/OIDServiceConfiguration.m 72.89% <50%> (-5.52%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28fc6c6...ea5ad45. Read the comment docs.

@WilliamDenniss WilliamDenniss added enhancement openid-cla: yes in-scope The AppAuth 406 Not Acceptable believe this is in-scope for AppAuth. labels Jun 26, 2018
@WilliamDenniss WilliamDenniss force-pushed the dev-logout branch 2 times, most recently from 27b4833 to 8680736 Compare July 6, 2018 19:19
@StevenEWright
Copy link
Collaborator

LGTM! Awesome!

@param redirectonURL The second redirect URI to compare.
@return YES if the URLs match down to the path level (query params are ignored).
*/
+ (BOOL)URL:(NSURL *)URL matchesRedirectonURL:(NSURL *)redirectonURL {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo matchesRedirectionURL

OIDIsEqualIncludingNil(standardizedURL.host, standardizedRedirectURL.host) &&
OIDIsEqualIncludingNil(standardizedURL.port, standardizedRedirectURL.port) &&
OIDIsEqualIncludingNil(standardizedURL.path, standardizedRedirectURL.path);
NSURL *standardizedRedirectURL = [redirectonURL standardizedURL];
Copy link
Collaborator

@julienbodet julienbodet Jul 7, 2018

Choose a reason for hiding this comment

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

[redirectionURL standardizedURL]

- (BOOL)shouldHandleURL:(NSURL *)URL {
/*! @brief Does the redirection URL equal another URL down to the path component?
@param URL The first redirect URI to compare.
@param redirectonURL The second redirect URI to compare.
Copy link
Collaborator

@julienbodet julienbodet Jul 7, 2018

Choose a reason for hiding this comment

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

Typo redirectionURL

BOOL authorizationFlowStarted =
[_externalUserAgent presentExternalUserAgentRequest:_request session:self];
if (!authorizationFlowStarted) {
NSError *safariError = [OIDErrorUtilities errorWithCode:OIDErrorCodeSafariOpenError
Copy link
Collaborator

@julienbodet julienbodet Jul 7, 2018

Choose a reason for hiding this comment

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

I know this is not directly related to this PR, but I wonder if this error type should be renamed to something like OIDErrorCodeBrowserOpenError or OIDErrorCodeExternalUserAgentOpenError since AppAuth now supports other browsers? The description is also mentioning Safari.

@Amnell
Copy link

Amnell commented Aug 2, 2018

I've been looking for a nice way to handle logout and this PR looks great. I've tested it in my project and it works great, except that the completion is not called. I've had a look at the code and it looks like shouldHandleURL: returns false as it compares the callbackURL from SFAuthenticationSession with _request.endSessionRequestURL. My callbackURL from SFAuthenticationSession is my custom url scheme for my app.

I'm not fluent in the openid spec, but is this really expected behaviour?

@WilliamDenniss @julienbodet

@dilee
Copy link

dilee commented Aug 10, 2018

Hi @Amnell
Do you have any sample code where you've used this improvement? I couldn't find a way to persist the session to avoid clearing session cookies when the app is closed and cleared from memory.

@WilliamDenniss @julienbodet

@danipralea
Copy link

danipralea commented Aug 17, 2018

Hello @WilliamDenniss @julienbodet I am having the same issues as @dilee . I am unable to remove the cookies so that at the next login the login is a fresh one. It automatically logs in with the previous user instead.

@Amnell would you be kind enough to share what you have working? Thanks.

@omgitstom
Copy link

Hey @WilliamDenniss - curious if you had any status/timeline for this. Okta is getting a few requests a week for this functionality in https://github.com/okta/okta-sdk-appauth-ios

We are trying to figure out if we can rely on this functionality in this library or if we need to circumvent and built it ourselves.

Thanks for any updates!

@WilliamDenniss
Copy link
Member Author

Hey @omgitstom, do you mean the timeline of when it gets merged into master?

The main thing I was waiting for was confirmation that it actually works as I don't have a test server with an end-session endpoint myself. Looks like there's at least one bug which should be fixed by #285, but other than that it might be ready.

Have you tried out this branch with Okta's end session endpoint? Does it work? Is there a public test instance I can try?

I would encourage everyone who has tried this code against their own end session endpoints to post their results. If the code is demonstrated to work in an interoperable way, I'll get it merged.

@mattio
Copy link
Contributor

mattio commented Oct 18, 2018

I've been using this in our Identity Server internal test environment along with @AndrewMcDrew's tweak in #285, my tweak in #312, and iOS 12 support from that branch added manually. So far it's been working as expected for me.

@WilliamDenniss WilliamDenniss force-pushed the dev-logout branch 2 times, most recently from 45bb8c5 to 4635c48 Compare October 18, 2018 17:42
@WilliamDenniss
Copy link
Member Author

@mattio, thanks for reporting back your results! I don't have any need for Logout myself, so I'm relying on y'all to test & validate the implementation here.

Based on your comments, I merged those 2 PRs. I also rebased our dev branch onto master, for iOS 12 / Xcode 10 support. The old work pre-rebase is here if anyone needs macOS 32-bit support for some reason (NB, that tag is not supported at all, you'd be own your own there).

Can everyone checkout the latest dev-logout branch, give it a try, and report back? You'll need to git rest --hard origin/dev-logout due to the rebase (which will also blast away any local changes, be warned).

Once a few people independently confirm that this actually works for them, we can look at getting it merged.

@WilliamDenniss
Copy link
Member Author

@julienbodet rebased onto latest master (#393)

@WilliamDenniss
Copy link
Member Author

WilliamDenniss commented May 31, 2019

@yurikoles

There is another issue, on newer iPads there is an alert/dialog before opening browser window on logout, that disappears immediately with no chance to accept it.

Do you have a strong reference to the session object? I think this happens if the object gets deallocated.

Please file a bug if you have a test case that can be repro'd

@WilliamDenniss
Copy link
Member Author

@blork

This is looking good, just implemented it and it's working great. However, I have my users signing in using the embedded browser (SFAuthenticationSession/ASWebAuthenticationSession), so kicking them out to "real" Safari to logout is a bit jarring. Is there a way a method for showing SFSafariViewController for this could be added (or can someone give me a hint on how to implement this myself)?

I'm not quite following, Logout should use the same user-agent as login, should it not? At least, that is the intention.

@WilliamDenniss
Copy link
Member Author

@yurikoles

@WilliamDenniss logout doesn't work in Swift:

When I try to call: OIDAuthorizationService.present(endSessionRequest, externalUserAgent: OIDExternalUserAgentIOS, callback: { (response, error)} )

Auth.swift:322:13: error: ambiguous reference to member 'present(_:externalUserAgent:callback:)'
            OIDAuthorizationService.present(endSessionRequest, externalUserAgent: OIDExternalUserAgentIOS, callback: { (response, error) in
            ^~~~~~~~~~~~~~~~~~~~~~~
AppAuth.OIDAuthorizationService:12:21: note: found this candidate
    open class func present(_ request: OIDAuthorizationRequest, externalUserAgent: OIDExternalUserAgent, callback: @escaping OIDAuthorizationCallback) -> OIDExternalUserAgentSession
                    ^
AppAuth.OIDAuthorizationService:15:21: note: found this candidate
    open class func present(_ request: OIDEndSessionRequest, externalUserAgent: OIDExternalUserAgent, callback: @escaping OIDEndSessionCallback) -> OIDExternalUserAgentSession

Can you file a separate issue for this one? It's getting too unwieldy to track so many issues in one thread.

When you do, can you share the code snippit as well?

@yurikoles
Copy link

Swift ambiguity is still there. Even login is broken in Swift on this branch.

error: ambiguous reference to member 'authState(byPresenting:externalUserAgent:callback:)'
            OIDAuthState.authState(byPresenting: request,
            ^~~~~~~~~~~~
AppAuth.OIDAuthState:12:21: note: found this candidate
    open class func authState(byPresenting authorizationRequest: OIDAuthorizationRequest, externalUserAgent: OIDExternalUserAgent, callback: @escaping OIDAuthStateAuthorizationCallback) -> OIDExternalUserAgentSession
                    ^
AppAuth.OIDAuthState:2:21: note: found this candidate
    open class func authState(byPresenting authorizationRequest: OIDAuthorizationRequest, presenting presentingViewController: UIViewController, callback: @escaping OIDAuthStateAuthorizationCallback) -> OIDExternalUserAgentSession

@WilliamDenniss
Copy link
Member Author

Can you file a dedicated issue for this, and provide the sample code that generates the error?

@yurikoles
Copy link

@WilliamDenniss tried to create a use-case for issue, but I can't reproduce this in new project or official example. Seems to be very rare issue which my project.

@WilliamDenniss
Copy link
Member Author

Thanks for confirming. I was surprised by this error, as I would expect Swift to infer the type and therefore not have any ambiguity. Maybe you could explicitly declare the variable with the type?

@WilliamDenniss
Copy link
Member Author

WilliamDenniss commented May 31, 2019

Well, I'm happy that enough people have tried this without problems.

Merging soon. #YOLO

@pgorzelany-objectivity
Copy link

pgorzelany-objectivity commented Jun 4, 2019

@WilliamDenniss any chance this will be merged and released soon?

@WilliamDenniss
Copy link
Member Author

Merged.

The dev-logout branch will be deleted, please update your Podfiles if you were using that branch.

@wilmarvh
Copy link

@WilliamDenniss Any timing on an officially tagged release with logout functionality included?

@ivanhjel
Copy link

☝️ Is this available now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement in-scope The AppAuth 406 Not Acceptable believe this is in-scope for AppAuth. openid-cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[dev-logout] OIDEndSessionResponse doesn't preserve additional response params