Picking up on @zachmargolis work, and fixing review comments by luksfarris · Pull Request #191 · openid/AppAuth-iOS

luksfarris · 2018-01-11T18:51:37Z

These are the basic objects needed to support RP-initiated logout, I'd like to make sure they look ship-shape before adding methods to use them and follow up in another PR

I've picked his work, and fixed the comments made by @WilliamDenniss

WilliamDenniss · 2018-01-23T00:03:45Z

+/*! @file OIDLogoutResponse.m
+    @brief AppAuth iOS SDK
+    @copyright
+        Copyright 2015 Google Inc. All Rights Reserved.


Will do, thanks!

WilliamDenniss · 2018-01-23T00:04:06Z

+
+/*! @brief Designated initializer.
+    @param request The serviced request.
+    @param parameters The decoded parameters returned from the Server.


what endpoint is this? rather than "from the Server" we should say "from the XYZ endpoint".

Good point, will rewrite soon!

WilliamDenniss · 2018-01-23T00:10:58Z

Thanks for addressing the review comments from #138. I think this is nearly good to go, but I have a few questions:

1/
Can both you and @zachmargolis please sign the OpenID CLA to cover this contribution?

2/
Do you plan to implement the controllers for logout in addition to the model implemented here?

3/
I don't have much experience with the OpenID Connect Session Management draft. How stable is it in general? Do you have a working server implementation to test the client code against? Do you plan to use this client code in production?

zachmargolis · 2018-01-23T00:37:28Z

Re 1/: I signed. Thanks for picking this back up!

luksfarris · 2018-01-23T15:21:25Z

Re 1/: signed as well. Thanks!

2/ I'm doing it right now. Will update the PR in several hours. I'll also rename everything from Logout to EndSession, to match the spec

3/ We are using okta's oidc, and they, in theory, support the end session flow. We plan to use it in production, as soon as we get it working. This pull request is interesting for us to have it available in the near future

dgommers · 2018-01-23T15:57:11Z

We are also in demand of this feature and I was about to test it with a Keycloak IDC instance. I tried to integrate the fork using Carthage. However, I discovered the newly added files are not attached to all the framework targets. This makes the new functionality unavailable for Carthage users.

To me it is unclear who should construct anOIDLogoutRequest instance and execute it. Could you elaborate on this? Right now it looks quite different from other request/response pairs, for example the OIDTokenRequest.

codecov-io · 2018-01-23T17:25:00Z

Codecov Report

Merging #191 into dev-logout will increase coverage by 0.19%.
The diff coverage is 73.75%.

@@              Coverage Diff               @@
##           dev-logout     #191      +/-   ##
==============================================
+ Coverage       68.03%   68.23%   +0.19%     
==============================================
  Files              40       43       +3     
  Lines            3898     4039     +141     
  Branches           73       73              
==============================================
+ Hits             2652     2756     +104     
- Misses           1244     1281      +37     
  Partials            2        2

Impacted Files	Coverage Δ
Source/OIDEndSessionResponse.m	`0% <0%> (ø)`
Source/OIDServiceDiscovery.m	`93.95% <100%> (+0.08%)`	⬆️
UnitTests/OIDEndSessionRequestTests.m	`100% <100%> (ø)`
UnitTests/OIDServiceDiscoveryTests.m	`82.27% <100%> (+0.07%)`	⬆️
Source/OIDEndSessionRequest.m	`78.78% <78.78%> (ø)`

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 d27db29...527f6ec. Read the comment docs.

luksfarris · 2018-01-23T17:28:49Z

@dgommers I have updated the pull request adding the targets and fixing the Docs. I am currently renaming it from Logout to EndSession and adding a new class method called +performEndSessionRequest:(OIDendSessionRequest *)request completion:(OIDEndSessionCompletion)completion to class OIDAuthorizationService. Do you have a better suggestion of where it should be?

EDIT: contrary to my initial thoughts, the End Session API should be presented to the user in a web page, so the OP should (according to the spec) ask if the end-user whether he wants to logout. IF the user, in the web page says YES, the webpage is redirected to the logout callback parameter that is already implemented. This flow is possible by using OIDSafariViewControllerFactory like is already being done in this SDK. My plan is to provide a similar experience

EDIT2: after further investigation, this looks like it will need a similar protocol to OIDAuthorizationFlowSession, and a class method in OIDAuthState like what we have for authStateByPresentingAuthorizationRequest:presentingViewController:callback:

WilliamDenniss · 2018-01-23T22:54:19Z

Thanks for signing the CLA all!

EDIT2: after further investigation, this looks like it will need a similar protocol to OIDAuthorizationFlowSession, and a class method in OIDAuthState like what we have for authStateByPresentingAuthorizationRequest:presentingViewController:callback:

Yes, you're right.

Previously there was only one place a user-agent (browser / SFAuthenticationSession) was used, now there will be multiple. So whatever way you implement this, the one thing I'm definitely going to be looking for is having the "open URL in user agent / callback" part reused in both cases – I hope there's a good way to generalize that code.

Regarding when we integrate this into master, I think it makes sense to merge this feature into master only when it's usable (models + controller). Do you want a mega PR with the models + controller, OR, we could merge this into a development branch so you can start a new PR with the controller. A development branch still enables people to test it via Carthage/CocoaPods if they specify the branch so that could be useful.

dgommers · 2018-01-24T09:33:20Z

Got it, thanks. Good to hear that logic can be shared with the OIDAuthorizationFlowSession.

Suggestion: A separate protocol for such resumeAuthorizationFlowWithURL: flows would help to keep the AppDelegate code clean. Implemented by both OIDAuthorizationFlowSession and the end session flow class.

Let me know if I can be of any help. I am keeping an eye on this thread.

luksfarris · 2018-01-24T10:48:12Z

I agree with you William, it makes a lot of sense to split this into 2 pull requests. I've renamed the request/response/test classes, and squashed the 3 commits. I'll start working on the other PR right away.
Thanks for the support!

EDIT: I forgot to update the framework headers. Updating PR now

luksfarris · 2018-01-24T16:31:38Z

I've created issue #195 and PR #196 to address the necessary architecture changes for us to create the RP-Initiated logout controller

WilliamDenniss · 2018-01-24T19:27:46Z

OK, are you ready for me to merge this into a development branch for logout?

luksfarris · 2018-01-25T11:11:14Z

Yes we are! But the PR is set to merge to the master branch, and I don't see any development branches in the repository, let me know if I need to create this branch and update the PR

WilliamDenniss · 2018-01-25T19:21:19Z

I created "dev-logout" can you point the PR at that?

Also can you reword your commit message to describe the actual changes that are going in? Feel free to credit Zach in the body of the message, but I think the first line should focus on the code changes.

#196 can go direct to master, but the rest we can merge into dev-logout until it's complete.

luksfarris · 2018-01-26T11:56:53Z

Done, and done!

WilliamDenniss · 2018-02-02T00:42:25Z

 #import "OIDTokenResponse.h"
 #import "OIDTokenUtilities.h"
 #import "OIDURLSessionProvider.h"
+#import "OIDEndSessionRequest.h"


Imports should be in alphabetical order, can you address in a subsequent commit? Here and the framework header.

I will! Thanks William

jaishankar · 2018-02-21T17:21:03Z

@luksfarris @WilliamDenniss Thanks for this logout feature, my understanding is that, after the PR 196 is done, there is going to a new class method (say. authStateByPresentingEndSessionEndpointRequest) in OIDAuthState which returns the callback with EndSessionEndpointResponse. Is this correct or can you clarify on how the SFAuthenticationSession / browser be invoked for the logout flow?
cc @dgommers

luksfarris · 2018-03-02T14:02:47Z

@jaishankar with the other pull request you saw, we'd have the ability to create non authentication external user agent requests. This allows us to create a new Logout FlowSession, and have it called in a class method in OIDAuthState.

How the browser will be invoked, is an open question. SFAuthenticationSession has a terrible user experience, because every time you open it, it asks for a "Sign In" confirmation (therefore our plan of "generic" requests looks and feels weird. With @WilliamDenniss 's implementation of external browsers, we now have other possibilities

This was referenced Jan 11, 2018

Objects for RP-initiated initiated logout #138

Closed

Implement RP-initiated logout #132

Closed

Add support for Logout okta/okta-oidc-ios#7

Closed

WilliamDenniss reviewed Jan 23, 2018

View reviewed changes

WilliamDenniss added openid-cla: no enhancement labels Jan 23, 2018

WilliamDenniss added openid-cla: yes and removed openid-cla: no labels Jan 23, 2018

luksfarris force-pushed the farris-rp-initiated-logout branch from 0768aab to 47d8460 Compare January 24, 2018 10:44

luksfarris mentioned this pull request Jan 24, 2018

Change the FlowSession and UICoordinator classes to be generic #195

Closed

luksfarris force-pushed the farris-rp-initiated-logout branch from ecb6535 to e50f457 Compare January 24, 2018 16:37

luksfarris mentioned this pull request Jan 25, 2018

Adding support for logout, issue #7 okta/okta-oidc-ios#30

Closed

luksfarris changed the base branch from master to dev-logout January 26, 2018 11:52

Added the End Session request/response for the RP-Initiated Logout

527f6ec

luksfarris force-pushed the farris-rp-initiated-logout branch from e50f457 to 527f6ec Compare January 26, 2018 11:56

WilliamDenniss reviewed Feb 2, 2018

View reviewed changes

WilliamDenniss merged commit 0c59e4b into openid:dev-logout Feb 2, 2018

jaishankar mentioned this pull request Feb 28, 2018

Making the FlowSessions and UICoordinators generic #196

Closed

Conversation

luksfarris commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamDenniss Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

luksfarris Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

WilliamDenniss Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

luksfarris Jan 23, 2018

Choose a reason for hiding this comment

Uh oh!

WilliamDenniss commented Jan 23, 2018

Uh oh!

zachmargolis commented Jan 23, 2018

Uh oh!

luksfarris commented Jan 23, 2018

Uh oh!

dgommers commented Jan 23, 2018

Uh oh!

codecov-io commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

luksfarris commented Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamDenniss commented Jan 23, 2018

Uh oh!

dgommers commented Jan 24, 2018

Uh oh!

luksfarris commented Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luksfarris commented Jan 24, 2018

Uh oh!

WilliamDenniss commented Jan 24, 2018

Uh oh!

luksfarris commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WilliamDenniss commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luksfarris commented Jan 26, 2018

Uh oh!

WilliamDenniss Feb 2, 2018

Choose a reason for hiding this comment

Uh oh!

luksfarris Feb 6, 2018

Choose a reason for hiding this comment

Uh oh!

jaishankar commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luksfarris commented Mar 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

luksfarris commented Jan 11, 2018 •

edited

Loading

codecov-io commented Jan 23, 2018 •

edited

Loading

luksfarris commented Jan 23, 2018 •

edited

Loading

luksfarris commented Jan 24, 2018 •

edited

Loading

luksfarris commented Jan 25, 2018 •

edited

Loading

WilliamDenniss commented Jan 25, 2018 •

edited

Loading

jaishankar commented Feb 21, 2018 •

edited

Loading