Skip to content

Add support for wildcards in subdomains for valid redirect URIs#5826

Closed
josh-cain wants to merge 1 commit intokeycloak:masterfrom
josh-cain:subdomainWildcard
Closed

Add support for wildcards in subdomains for valid redirect URIs#5826
josh-cain wants to merge 1 commit intokeycloak:masterfrom
josh-cain:subdomainWildcard

Conversation

@josh-cain
Copy link
Copy Markdown
Contributor

See https://issues.jboss.org/browse/KEYCLOAK-3585.

This is a re-work of previous MR.

@stianst stianst added the Hold label Jan 2, 2019
@stianst stianst removed Hold labels Feb 21, 2019
@stianst stianst requested a review from mposolda February 21, 2019 10:13
@stianst stianst assigned mposolda and pedroigor and unassigned mposolda Feb 21, 2019
@stianst stianst removed the request for review from mposolda February 21, 2019 10:13
@stianst stianst assigned stianst and unassigned pedroigor Feb 21, 2019
Comment thread pom.xml
<hamcrest.version>1.3</hamcrest.version>
<jmeter.version>2.10</jmeter.version>
<junit.version>4.12</junit.version>
<mockito.version>1.10.19</mockito.version>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We have decided not to use mocking frameworks so we can't accept this

@@ -0,0 +1,117 @@
package org.keycloak.protocol.oidc.utils;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Look at OAuthRedirectUriTest which can be extended to cover these tests properly

@stianst stianst assigned pedroigor and unassigned stianst Feb 21, 2019
@davidholsgrove
Copy link
Copy Markdown

Hi, any chance this PR will be merged into a Keycloak release soon?
Allowing the (optional) use of wildcard redirect urls on a controlled subdomain would vastly reduce administrative overhead for dynamic environment creation.
thanks,
David

@josh-cain
Copy link
Copy Markdown
Contributor Author

Closing. It's clear that this is not going to happen (at least not with this PR), and it's so stale at this point, might as well re-write it.

@josh-cain josh-cain closed this Apr 25, 2019
@sharedprophet
Copy link
Copy Markdown

@josh-cain Could you link the new PR here if you create one, so I can watch it?

@josh-cain
Copy link
Copy Markdown
Contributor Author

josh-cain commented Apr 25, 2019

@sharedprophet - my apologies, I can see where that comment was not clear. I have no intentions of re-writing this myself at this point. Per @stianst 's comment, they're not going to take anything with mocks, so a re-write will entail one of:

  1. refactoring to decouple the data domain from a utility function as simple as wildcard validation, as you must have a correctly populated RealmModel to test the exposed verifyRedirectUri function.
  2. manipulating the accessibility of the private matchesRedirects (commonly done with something like PowerMock, which I suppose would once again be taboo).
  3. writing massive integration tests involving an application server to test its ability to properly parse a url string since mocks cannot be used.

Pretty sure #1 or #2 above would get rejected, and I don't have the time/patience for #3. Sorry to abandon it, hopefully the community can band together for this one soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants