Supported custom validateRedirectUri()

[FStefanni]

Summary

This pr proposes a feature to allow custom 'validateRedirectUri()' method implementation.

Linked issue(s)

This pr follows issue #89, point 4, and original pr 482.

Added tests?

This is missing.

OAuth2 standard

Unknown

[jorenvandeweyer]

Can you also include some docs and write some tests?

[FStefanni]

Hi,

sorry, I truly have very few time, so all I can do just now is to review original project pr's, and
propose the ones which seems fine to me.

But you are right: it is important to have doc and tests, so: can someone help writing them?

Regards.

[FStefanni]

BTW, changed the target branch to development

[jankapunkt]

@jorenvandeweyer I think if @FStefanni has not much time then we can work on this as well without cloning the PR repository:

$ git fetch origin pull/97/head
$ git checkout -b pullrequest FETCH_HEAD

[jorenvandeweyer]

While writing tests and documentation for this pull request I came across multiple issues.

• No support for promises / callbacks.
• Should return true in case of a valid redirect_uri, now it expects a falsey value in case of a valid redirect_uri.

@jankapunkt I think I can not push my commits to this pull request because I'm not listed as a member of this organisation / maintainer of this repo.

[jankapunkt]

I think we are on good point to add more people @HappyZombies @jwerre

[jorenvandeweyer]

@HappyZombies Thank you for adding me.I was able to push my commits now.

I included some docs and tests. I also made sure promises and callbacks are also supported.

[Uzlopak]

my 2 cents: I dont like it. An integration-implementer of the model could mess up the redirectUri validation. Actually what we need is a standardized way to give the integration-implementer the opportunity to add allowed redirectUri's to the client instance and we should implement a very safe way to validate the incoming redirectUri with the allowed redirectUris.

Redirect_uri is actually quite sensitive, e.g.:
http://blog.intothesymmetry.com/2014/04/oauth-2-how-i-have-hacked-facebook.html

So we should take the responsibility from implementing a correct validation away from integration-implementer. We should actually add a getAllowedRedirectUris-method, if we want to implement it as a model-function.

[jankapunkt]

@Uzlopak thank you for the linked blog article I will read it and review again with this in mind

[jorenvandeweyer]

I disagree..

We should allow them to write their own validation at their own risk. Since we are not able to provide a solution for all use cases.

We can provide in addition some safe helper functions that can be safely used in their own implementation. For example: isTopLevelDomain

[jankapunkt]

I suggest we all take or time to read rfc OAuth 2.0 Threat Model and Security Considerations and ensure the PR implements it in a way that concerns from the rfc are covered. I at least would go this round.

If the risks involve the default behavior

• the code needs to be changed
• tests need to be added to ensure the risks stay removed

If the risks involve the implementor's code I would at least

• add risk warnings to the docs / manual
• link the above mentioned rfc

what do both of you think?

[jorenvandeweyer]

I read the spec quickly, and the solutions a bit more carefully.

At the moment we have the solution from Section-5.2.3.5 implemented and this is used as the default validation.

 An authorization server should require all clients to register their
   "redirect_uri", and the "redirect_uri" should be the full URI as
   defined in [RFC6749]

The risk exists when an implementor creates their own implementation of validateRedirectUri.

We should provide this function as an option in combination with a warning and a link to the full spec, since the spec also aknowledges more complicate Authorization servers require more dynamic solutions and are not able to require a full URI.

I think it's not our right / responsibility to enforce a single solution.

[jankapunkt]

@jorenvandeweyer I agree

[Uzlopak]

After some thoughts I also agree