Added native support for Password Grant flow

[amaurydavid]

As discussed in #178 , here is a draft for native support in password grant.
I created a new OAuth2PasswordGrantCustom to separate it from the original webview flow.
For now it just bypass the client registration step, as the RFC doesn't mention any requirement of a client_id being send as a parameter of the accessToken request.
I copied the OAuth2AuthorizeUI and OAuth2Authorizer pattern with OAuth2LoginPresentable and OAuth2LoginPresenter (macOS is not supported yet though)

Let me now if there is something I'm missing or wrong.

[p2]

Thanks, wow, that's quite extensive. A few thoughts:

• is there a reason you're not subclassing OAuth2PasswordGrand? Then you'd simply need to override doAuthorize(), where you show a custom login view if username/password has not been set or proceed with obtainAccessToken () if you have both. Now you're duplicating many methods anyway.
• do you need the presenter protocol and the delegate? If you only have a delegate, then ask it for the login view and present it on the auth context, is that not enough? Let me know if this doesn't make sense:
  • doAuthorize() is called, there is no username, the grant flow asks its delegate for the login controller
  • your login controller shows, does its thing and when the user submits, calls a resume(username:password:) method in the custom grant class which calls doAuthorize()
  • doAuthorize() sees it has username/password and continues with regular obtainAccessToken()
  • if your user cancels or if validation fails, you can call didFail() yourself from the login controller
• the validate() method is a good idea, could it only be a delegate method? I see it in the delegate protocol but also implemented in the custom grant flow. If it's just in the delegate you don't need to subclass just to customize validation. If my above flow makes sense, it could even entirely live in your custom login controller and no delegate method is needed
• client registration does nothing if there is no client-id, so you can just leave that in place
• and since I'm a whitespace Nazi: please undo the whitespace changes that Xcode has done for you. Empty lines should have the same indentation as the block they're in.

[amaurydavid]

When I started I wanted the password grant flow to only provide a native view and get rid of the web view thing, because using a web view for this flow doesn't really make sens to me. So there would not be OAuth2PasswordGrantCustom but only a brand new OAuth2PasswordGrant.
In the end I kept the current OAuth2PasswordGrant as it is, both because I wanted interoperability with older versions of the lib and because even if the web view thing doesn't make sense to me it could still be useful to someone else.
An "issue" I will face if I subclass OAuth2PasswordGrant is that I don't want the lib to keep the credentials in memory, that's why I deleted the "username" and "password" properties in OAuth2PasswordGrantCustom: credentials are only passed in parameters to build the request and that's it.

About protocols & delegates, I'm not sure what you mean but I think you want the OAuth2LoginPresenter logic to be included in OAuth2PasswordGrantCustom. That would need OAuth2PasswordGrantCustom to be platform dependent and so duplicate logic that doesn't depend of any platform. I preferred the approach used with OAuth2AuthorizerUI where only the UI logic is duplicated on each platform.
While typing this, I'm thinking of having a OAuth2BaseLoginPresenter<Type:LoginController> and platform dependent subclasses like OAuth2LoginPresenter<UIViewController:LoginController> for iOS or tvOS and OAuth2LoginPresenter<NSViewController:LoginController> for macOS. I'll try it.

I created the OAuth2LoginControllerDelegate to separate this logic from the OAuth2PasswordGrantCustom. The login controller doesn't have to know anything about the OAuth2 instance managing it, it only needs to have an object to notify about the new credentials or the flow abortion.

As for the validate func being in a delegate, I think you want it to be in the OAuth2LoginPresentableDelegate. It indeed makes more sense than having to subclass the OAuth2 instance to customize it. I'll change this, and probably rename the actual validate() func to tryCredentials()or something like that.

The issue with the registration process is that if there is no clientId and no authorizeURI, it will return as an error, hence I'm bypassing this process.

Finally about whitespaces, that's an issue I'm figuring out. I'am actually switching from XCode to AppCode and their indentation/whitespace needs to be matched ;)

[p2]

Great, thanks!

• Web View: agreed, it also doesn't make sense for the client credentials flow. But the web view is only activated in doAuthorize(), so overriding this (as is done in the client credentials flow) is sufficient to prevent it from ever showing. Since it's needed in all other flows I kept it in the base class.
• Credentials: any particular reason why you don't want them in memory? As a workaround you can override accessTokenRequest(), call super, erase username/password and return from that method.
• Protocols & Delegates: absolutely keep the flow platform dependent. What I mean is that it may be possible to not have 4 new protocols, as is the case now. I haven't thought this through and you know more about your code, my thought is that it could work with having a presentable protocol and platform-dependent implementation, plus one delegate that the app implements. Just a suggestion from a high level.
• Validate: I thought this is a hook to e.g. check if the username is an email address or similar. Simplest would be to just call obtainAccessToken() with the credentials, if it's a 403 you catch that error, show something (delegate method?), wait for new input and then try the suggested resume() method again, which forwards to obtainAccessToken() again.
• Registration: If there is no client-id, it will return without an error. If there is a client-id but no registration URL, it will return an error

Thanks for being so constructive!

[amaurydavid]

So here is the draft v2 :)

OAuth2PasswordGrantCustom now inherits from OAuth2PasswordGrant, which had been deprecated. Once OAuth2PasswordGrant is removed there would be only the accessTokenRequest and obtainAccessToken functions to move to OAuth2PasswordGrantCustom.
I left the registerClientIfNedded bypass as without it I'm getting a noRegistrationURI error if I'm not providing a client_id nor a registrationURI.

The new protocols are now simpler:

• removed the OAuth2LoginControllerDelegate to just use OAuth2PasswordGrantCustom instead.
• removed delegates & oauth2 instances from OAuth2LoginPresentable.

To sum up, here is the default workflow using OAuth2PasswordGrantCustom :

At some point in the app, we need the user to log in: a OAuth2DataLoader is created using OAuth2PasswordGrantCustom (it only needs an authorize_uri or a token_uri, a context, and a delegate), and call attemptToAuthorize from it.
The oauth2 will then present the login controller provided by it's delegate.

The user enters his credentials and hit some "log in" button. As this point, the controller performs (or already did) checks on credentials format, and then call the tryCredentials function from its oauth2 instance. We talked early about having a validatefunc in the delegate but it would only provide checks after the user would hit the button, while a best practice would be that the controller perform the checks on the fly while the user is entering the credentials. So no validate, let's let the controller perform the validation its own way.

After some time, the callback is called with the error matching server's response.
If the server rejected the credentials, then the user can enter other ones and call tryCredentials again. The login controller is not dismissed automatically after a tryCredentials failure as it would make the loginController to be presented/dismissed at each user's attempt, and we don't want that.

Once the user is authorized or hit the "actually I don't really want to log in", the login controller calls the endAuthorization function, which dismiss the login controller.

[p2]

Great, thanks. This is moving the right direction; I like the protocols now, much slicker!

I've set up a password grant flow and I cannot reproduce your web view issue, the flow never attempts to show a login screen. It either throws when there is no username/password or performs the request, as designed in doAuthorize().

What I think should be done is to weave your additions into the existing flow (I don't think you can deprecate a class that serves as a superclass, or at least it's weird). If you change doAuthorize() to check if there's username/password, and if not proceed with what you currently have in doAuthorize(), but if they've been specified then proceed with obtainAccessToken() (as the password grant currently does), then you'd keep compatibility with the current password grant (if somebody uses their own view controller without your hooks). Your tryCredentials() should just set the username/password params, then call doAuthorize(). I would also only delete username/password once there is an access token (this would also conform to the spec). Maybe rename tryCredentials() to authorizeWith(username:password:...)?

Do you think this make sense?

Minor Things

• I've just pushed a commit that allows to make client_id optional for password grand (I've done that), so your registerClientIfNeeded() override is no longer necessary.
• endAuthorization() does call didFail(), should it be renamed to reflect that fact?
• Please use /** to start comments, this way Xcode picks these comments up as documentation (as does jazzy, which is used to generate documentation). Also, no newline between comment and class/func.
• Add your name to the top of CONTRIBUTORS!!

[amaurydavid]

Draft v3 is there:
Both web view and native logics are now in OAuth2PasswordGrant.
I removed the registration bypass as you fixed it.
Some updates with the workflow:
didAuthorize and didFail functions are no longer called at tryCredentials completion.
This way, the attemptToAuthorize's closure is only called once when the loginController is dismissed, and not a every request's response. On the other hand, tryCredentials provides a callback so that the loginController still knows whether the request succeeded or failed and why.

tryCredentials is not renamed because there are many functions with authorize and I think an other one will add more complexity. Moreover tryCredentials describes well what it does: it tries the user's credentials against the server.
As for endAuthorization, it can now call both didAuthorize or didFail.

[p2]

Very cool, we're getting there! Now to the details.

[p2]

please undo

[p2]

Should be more descriptive, e.g. noPasswordGrantDelegate.

[p2]

undo

[p2]

Document parameters

[p2]

No indentation from /** to text (yes, was new to me too, but Swift wants it).

[p2]

Please document

[p2]

No newline here and after the import

[p2]

Please document

[p2]

Please document

[p2]

Please document

[p2]

Should leave this newline in place for proper docs generation.

[amaurydavid]

New version there, I hope this is the last ^^

[p2]

Sweet, thanks for hanging in!!

[p2]

Will need to make some changes since with these new dependencies, the framework no longer compiles via swift build (It uses OAuth2LoginPresenter() in the Flows module, where this isn't yet defined).

[p2]

I have changed some of the internals and renamed the protocols (so they are aligned with what's already there). Also OAuth2LoginPresentable is gone since it's not needed.

Can you test if this still works with your setup?

[amaurydavid]

There are now some issues with your master branch now:

• a reference to OAuth2LoginController is still present in OAuth2CustomAuthorizer+iOS
• You removed the dismissLoginController func from OAuth2PasswordGrant but its customAuthorizer is not public. It makes the loginController impossible to dismiss if the authConfig.authorizeEmbeddedAutoDismiss is false

[p2]

Thanks for testing!

• Indeed, hadn't pushed everything, it's up now
• The login controller is supplied by the delegate via loginController(oauth2:), so it should know the controller or at least authConfig.authorizeContext. So you should be able to dismiss it simply by calling iOS's dismiss() on the context, or on a reference to the login controller. Does that work?

[amaurydavid]

As the loginController is presented by the oauth2, only the oauth2 should be responsible of dismissing it. Only the oauth2 is supposed to know how the login controller have been presented (presented modally, pushed on a navigation controller,...) and telling devs to use the authorizeContext to dismiss it by themselves breaks this rule.
If in a future release the login controller is no longer presented modally, all apps working with earlier versions and manually dismissing it by guessing it was presented modally won't work.

[p2]

Good point!

I've added a call to didFail() in dismissLoginController(), which will trigger the callback unless it has been cancelled before. Does this make sense to you?

[amaurydavid]

Sounds fine to me :)

[p2]

Cool, thanks for the feedback and let me know if something else comes up!