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

Docker Protocol Implementation #3565

Closed
wants to merge 1 commit into from

Conversation

josh-cain
Copy link

See KEYCLOAK-3592.

@stianst stianst added the 3.x label Nov 30, 2016
@tronicum
Copy link

That sounds great

@mstruk
Copy link
Contributor

mstruk commented Mar 8, 2017

@cainj13, any suggestion what I might be doing wrong setting up client with docker-v2 protocol here?

@stianst
Copy link
Contributor

stianst commented Mar 20, 2017

Docs PR: keycloak/keycloak-documentation#55

@josh-cain
Copy link
Author

@mstruk I think it's not set up for HTTP Basic auth. I tried to flesh the instructions out a little on the keycloak-documentation PR #55. Did that help at all?

@mstruk
Copy link
Contributor

mstruk commented Mar 20, 2017

@cainj13 I'll try again, and let you know.

@mstruk
Copy link
Contributor

mstruk commented Mar 22, 2017

@cainj13 I got it working, but I'm seeing some issues, so maybe I didn't configure everything properly. See here for how exactly I configured it.

The issue I'm seeing now is when I authenticate as a non-existing user. If user exists but the password is incorrect everything works correctly - the server returns 401 Unauthorized.
If user does not exist, however, the server return 500 Internal Server Error, and there is a stacktrace on the server:

10:11:54,509 WARN  [org.keycloak.services] (default task-41) KC-SERVICES0013: Failed authentication: org.keycloak.authentication.AuthenticationFlowException
	at org.keycloak.authentication.DefaultAuthenticationFlow.processResult(DefaultAuthenticationFlow.java:249)
	at org.keycloak.authentication.DefaultAuthenticationFlow.processFlow(DefaultAuthenticationFlow.java:192)
	at org.keycloak.authentication.AuthenticationProcessor.authenticateOnly(AuthenticationProcessor.java:792)

@@ -52,17 +53,24 @@ public String getDisplayType() {

@Override
public String getReferenceCategory() {
return null;
return "basic";
}

Copy link
Contributor

@mstruk mstruk Mar 22, 2017

Choose a reason for hiding this comment

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

@stianst If HttpBasicAuthenticator is usable beyond saml ecp protocol I suggest we move it to authentication/authenticators/browser

Copy link
Author

Choose a reason for hiding this comment

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

I'd be down for that, but looking at the error responses, I need to make some updates to this MR. With the implementation we've got running, we're sending back JSON responses specific to the docker client for display back to the user. I suppose we'd want to do that as well here, but that's probably going to mean having a docker-specific HTTP Basic authenticator since we'll be crafting custom error responses. Any other way to do that?

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea. But I don't think there's a problem with having docker specific HTTP Basic Authenticator. Maybe we should just call it DockerAuthenticator to avoid any confusion as to what it's there for.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good, will add

@@ -37,7 +37,7 @@
</div>
<kc-tooltip>{{:: 'client.enabled.tooltip' | translate}}</kc-tooltip>
</div>
<div class="form-group clearfix block">
<div class="form-group clearfix block" data-ng-show="protocol != 'docker-v2'">
<label class="col-md-2 control-label" for="consentRequired">{{:: 'consent-required' | translate}}</label>
<div class="col-sm-6">
<input ng-model="clientEdit.consentRequired" name="consentRequired" id="consentRequired" onoffswitch on-text="{{:: 'onText' | translate}}" off-text="{{:: 'offText' | translate}}"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to sort Client Protocol list for #protocol select input in such a way that docker-v2 is the last one in the list - it makes more sense for it to be the last choice as it's reasonable to expect that it will be the least used option?

Copy link
Author

Choose a reason for hiding this comment

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

I can certainly look. I'd agree that it's probably going to be the least-used option.

Copy link
Author

Choose a reason for hiding this comment

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

So I took a look at this - other than one-off hacking the order (which I don't think we wanna do), this would require a pretty broad change to the data model for the serverInfo's ProviderRepresentation. Right now angular is just doing a naive sort:

$scope.protocols = Object.keys(serverInfo.providers['login-protocol'].providers).sort();

Could we perhaps create an issue and do that on a follow-up review? This one's already gotten pretty big, and I'd consider that a more general aesthetic fix. For instance, there is not a determinitive order in authenticator choices in the dropdown, etc. I think the angular models could use a widespread application of some kind of salience/display order.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. If docker-v2 feature has to be enabled explicitly, then I think it's acceptable to have it like this for now.

Sorting is a problem present in multiple locations. If we wanted sorting control at db query layer that would require to model current unsorted collections as lists rather than sets, which is quite a disruptive upgrade in the model. Simple alphabetic sorting (ascending, descending by some field) at REST endpoint would already get us very far for a general case (performance-wise it would be better to do it at db layer), but here we even have a special case where we want specific entry to be at the end of the list. That would still best be done in REST endpoint rather than on the client. For example a special query parameter with special syntax using something like:

?reorder=authenticationProviders[-docker-v2]

might be used to move docker-v2 entry to the end of the collection (leading minus signifying 'move to the end'). We could implement that as part of a bigger REST API overhaul which would add facilities like that across the board in a generic way.

public String getDisplayType() {
return "Quickstart";
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make more sense to call this 'Docker Machine YAML' rather than 'Quickstart'. There is a Keycloak Quickstarts repository, and it would make more sense to create a submodule 'docker' there and move all the quickstart related documentation there.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant 'Docker Compose YAML' ...

Copy link
Author

Choose a reason for hiding this comment

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

Done. I think it leaves out the fact that it generates certs for you to mount for your registry, but no name is perfect. Two hard things.

@@ -67,4 +67,7 @@ keycloak.server.subsys.default.config=\
</properties>\
</provider>\
</spi>\
<spi name="login-protocol">\
<provider name="docker-v2" enabled="false"/>\
</spi>\
Copy link
Contributor

Choose a reason for hiding this comment

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

Profiles should be used to control activation of this feature. See here.

@stianst I guess we have a policy in place that all features are enabled by default, but users can disable them?

import org.keycloak.events.EventBuilder;
import org.keycloak.models.AuthenticationFlowModel;
import org.keycloak.models.ClientSessionModel;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.RealmModel;
import org.keycloak.protocol.LoginProtocol.Error;
import org.keycloak.services.ErrorPageException;
import org.keycloak.services.ServicesLogger;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a redundant import.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. did an import organization on all docker stuff as well.

private KeycloakSession session;
private RealmModel realm;
private UriInfo uriInfo;
private HttpHeaders headers;
Copy link
Contributor

Choose a reason for hiding this comment

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

Fields uriInfo and headers seem redundant - assigned, but never used.

Copy link
Author

Choose a reason for hiding this comment

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

They're part of the setters defined in LoginProtocol. Docker protocol doesn't need them, can make those no-op setters if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's better as it is. I just pointed out what IDEA was telling me. Maybe that part of implementation was still incomplete.

}

public static String formatPublicKeyContents(final PublicKey publicKey) {
return encodeAndPrettify(BEGIN_CERT, publicKey.getEncoded(), END_CERT);
Copy link
Contributor

Choose a reason for hiding this comment

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

CERTIFICATE is not exactly the same as PUBLIC KEY. For public key for example 'openssl' CLI tool uses '-----BEGIN PUBLIC KEY-----' and '-----END PUBLIC KEY-----'.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. Was just testing with compatibility with a docker registry since that's what's going to be consuming it. Want me to make the switch and see how docker likes it?

public void writeToRealZip() throws IOException {
final Response response = fireInstallationProvider();
final byte[] responseBytes = (byte[]) response.getEntity();
FileUtils.writeByteArrayToFile(new File("target/docker-quickstart-install.zip"), responseBytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

Test makes no assertion. It should probably check that what was returned is a zip file.

Copy link
Author

Choose a reason for hiding this comment

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

Was for smoke testing only - so I could validate locally without having to spin up a server. Have marked as ignored, but can remove if you'd like.

import static org.keycloak.protocol.docker.installation.DockerQuickstartInstallationProvider.QUICKSTART_ROOT_DIR;

public class QuickstartInstallationTest {

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally we would just have one test that unpacks returned zip and checks if everything is there as it should be.
Our testsuite performance is not the best, so we should try skip unnecessary processing, especially when it's trivial to achieve.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Should be a single zip operation now.

@mstruk
Copy link
Contributor

mstruk commented Mar 22, 2017

I've noticed another UI issue. When clicking around different clients in Admin Console, it now displays 'docker-v2' protocol for every client.

@josh-cain
Copy link
Author

@mstruk The non-existent user problem should be fixed now. I subclassed the HttpBasic authenticator and made it return Docker error tokens as described in the docs. Now localized docker messages should appear.

I took some liberties in the getMessageForKey() method to try to resolve message bundles. Generally this happens in the Freemarker section, but we're operating outside of a LoginFormsProvider for this protocol. Can you give that a once-over and make sure it's OK?

There also was an issue where the SAML ECP profile didn't seem to check for disabled users. I added the check, but can easily pull it out of the base class if that's not desired behavior.

Also, I couldn't observe a clear pattern between the use of singletons and instantiating a new provider every time (I.E. the cookie provider and ECP HTTP basic provider differ). I went with instantiating one each time, but if there is a preference for the opposite just let me know.

@josh-cain
Copy link
Author

I've noticed another UI issue. When clicking around different clients in Admin Console, it now displays 'docker-v2' protocol for every client.

Yeah.... so turns out that those clients are being created with a 'null' protocol. They happened to look right previously since OIDC was the first protocol alphabetically sorted, but they have a null 'Protocol' column on creation in the DB. Should probably do something about that...

@josh-cain
Copy link
Author

I've noticed another UI issue. When clicking around different clients in Admin Console, it now displays 'docker-v2' protocol for every client.

Yeah.... so turns out that those clients are being created with a 'null' protocol. They happened to look right previously since OIDC was the first protocol alphabetically sorted, but they have a null 'Protocol' column on creation in the DB. Should probably do something about that...

Opened #3998 to deal with this.

import java.util.List;
import java.util.Map;
import java.util.Set;
import org.keycloak.provider.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Imports should not be folded like this. Default IDEA setting is bad.

Copy link
Author

Choose a reason for hiding this comment

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

yeah... just updated and started from a fresh IDEA instance. Haven't taken the time to tune all the things back yet. Fixed now.

if (factory instanceof FeatureDependentProviderFactory) {
return Optional.ofNullable(((FeatureDependentProviderFactory) factory).getRequiredFeatures()).orElse(Collections.emptySet())
.stream().allMatch(requiredFeature -> Profile.isFeatureEnabled(requiredFeature));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

@stianst Is it ok to merge this additional feature in common services into this PR, or should it be a separate PR (addition of FeatureDependentProviderFactory, and its handling here)?

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have EnvironmentDependentProviderFactory#isSupported which covers this and there is no need to introduce yet another way to do it. It's nice and simple to use and is also flexible beyond just profiles. Look at:

https://github.com/keycloak/keycloak/blob/master/services/src/main/java/org/keycloak/authentication/authenticators/browser/ScriptBasedAuthenticatorFactory.java#L155

Copy link
Author

@josh-cain josh-cain Apr 4, 2017

Choose a reason for hiding this comment

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

@stianst @mstruk Done. Just a thought - 'EnvironmentDepndent' becomes a bit of a misnomer here. Perhaps something like 'ConditionalProviderFactory' going forward? Two hard things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I agree conditional would be a much better name. However, renaming it breaks backwards compatibility so that's not an option and I don't like alternatives so it is what it is ;)

@mstruk
Copy link
Contributor

mstruk commented Apr 3, 2017

Authentication seems to work fine now.

One little thing I noticed in UI still is client settings - after creating a new client of type docker-v2, the 'Valid Redirect URIs' field is reported as required. If this information is not used at all maybe 'Root URL', 'Valid Redirect URIs' and 'Base URL' should simply be hidden from the form?

At the minimum it seems the field 'Valid Redirect URIs' should not be marked required, if there's a reason it should stay displayed on the form.

context.success();
} else {
userDisabledFailure(context, realm, user);
}
Copy link
Contributor

@mstruk mstruk Apr 3, 2017

Choose a reason for hiding this comment

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

Yes, I believe this should be in a separate PR.
@pedroigor Can you confirm the way this should fail for disabled users is a proper fix?

Copy link
Author

Choose a reason for hiding this comment

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

OK - so I updated the userDisabledFailure to do what it was doing before in the HttpBasicAuthenticator: letting users in. Otherwise, the refactoring in place is just to expose various action components to the Docker child class. Will that fly?

@Override
protected void authFailure(final AuthenticationFlowContext context, final RealmModel realm, final UserModel user) {
invalidUserAction(context, realm, user.getUsername(), context.getSession().getContext().resolveLocale(user));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that by resolving locale for error messages for existing user we may give away the information that the attempted user is in fact a valid user. For example, if I try 'bob':'1234' I get back "Unknown user or invalid password", but if I try 'jacques':'1234' I may get back something like 'Utilisateur inconnu ou mot de passe invalide'. Now I know jacques is a valid username.

Localisation should only take into account passed language value - extra query parameter in auth request, or a cookie - if docker-v2 protocol supports that.

But then, won't the result of 'docker login' then be something like:

Error response from daemon: Get https://localhost:5000/v2/: unauthorized: Utilisateur inconnu ou mot de passe invalide.

Why even bother with such a halfway solution? I would keep it simple - just return all messages in english.

Copy link
Author

Choose a reason for hiding this comment

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

Valid point. Just assumed that l10n would be a requirement for userspace mssages, but I see what you're saying, and agree that said issue could be used to enumerate at least some of the users. Has been removed, to my knowledge there is no support for that in the docker auth v2 login protocol.

@josh-cain
Copy link
Author

@mstruk - Display settings for client information has been updated per your comments.

Anything else here or are we good?

@mstruk
Copy link
Contributor

mstruk commented Apr 5, 2017

Almost :)

We also need to add some integration tests under testsuite/integration-arquillian as was discussed. Let's assume docker tools are installed on the system. Instructions for preparing the test run should be added to HOW-TO-RUN.md

Maybe @pdrozd or @stianst can chime in with any more specific suggestions.

Finally, there is one big thing that needs to be done before we can merge this PR, and that is - it needs to be squashed into a single commit. Current remerging from master into your branch makes it impossible to clearly see the changes, and makes it messy to undo merge should there be some issue. The simplest way that I see to do this is to simply backup your branch, hard reset it to latest remotes/upstream/master and then cherry-pick into it from your backup branch, resolving conflicts as you move along. Finally, squash it into one commit, and force push it over current branch to update the PR.

@stianst stianst self-assigned this Apr 19, 2017
@josh-cain
Copy link
Author

@stianst So I think I have a reasonably sound test harness. It requires a working docker binary on the host machine, and will just skip if one is not found. The docker client and registry are spun up and configured with some custom settings in containers and the existing arquillian/junit framework is used to stand up a test docker realm.

Will address the idea of an entirely new flow for docker auth shortly.

@mstruk
Copy link
Contributor

mstruk commented Jun 15, 2017

@vmuzikar I've set up a Fedora 25 in VirtualBox to try and find a way to support both Fedora, and macOS. Looks like I found a way: mstruk@2bce514

Could you check, and see if that works for you? See HOW-TO-RUN.md

I had quite a few problems fighting through it. Seems there's an issue with error reporting. At least some of error details seems to be suppressed. For example I don't see AssumptionViolatedExceptions from @BeforeClass logged anywhere. That requires working with source code to debug why the test was skipped for example.

@vmuzikar
Copy link
Contributor

@mstruk Thanks for the update!

Seems it fixed the iptables problems in CI (RHEL 7.3). However, it still persists on my Fedora 25 installations (both on my machine and a virtual one which is a clean fresh installation).
So it seems like some distros/flavors just have more enforcing default iptables rules.

So, I've updated the HOW-TO-RUN accordingly.
Also, I've changed the Fedora instructions a bit so the test wouldn't require root permissions (which would also require to have Keycloak build under root, not just the test).
Could you please check it out? vmuzikar@32be113

One last thing before we are good to go.
The test seems more unstable after the recent changes. Sometimes it happens that the test doesn't wait for the Client container to spin up and proceeds with the testing leading to failing test.
This happened multiple times, both on my local machine, on virtual machine and in the CI as well.
Is it just me again or is it happening for you as well?

@mstruk
Copy link
Contributor

mstruk commented Jun 19, 2017

@vmuzikar I tried your instructions and they work for me.

WRT intermittent failures I think I have experienced that a few times. I have not seen that often, and assumed it was some config change of mine that caused it.

@josh-cain
Copy link
Author

@vmuzikar, @mstruk I think is what you're looking for:

.waitingFor(new HttpWaitStrategy().forStatusCode(200).forPath("/auth"))

Just encountered the same problem on another project. I'll update PR for appropriate wait checks later today.

@josh-cain
Copy link
Author

@vmuzikar - Just updated with a wait to validate that the docker daemon is up and running. Is that what you're after?

Also merged in changes from you and Marek.

@vmuzikar
Copy link
Contributor

@cainj13 Thanks for the update. Unfortunately, it didn't seem to fix the issue.

I've done some research and I've found 2 problems:

  • validateDockerStarted used stdout instead of stderr where 'Cannot connect ...' message is sent
  • when it failed, it actually failed right on the systemctl call so the docker service wasn't started at all (just waiting for docker to start wasn't enough)

I hope I fixed both problems in vmuzikar@9334803
Could you please check it?

Also I've created KEYCLOAK-5079 so we wouldn't forget to update testcontainers once the new version with fixed SELinux is released.

So if my changes are ok and you'll merge them, I believe we are finally ready to merge! :)
The only glitch is the potential problems with default iptables rules, but since this affects only some distros/flavors (RHEL seems to work fine without any modifications) I think we can live with that.

I'm sorry it took too long and perhaps even got a bit frustrating. :) I needed to be sure that the test is ok and working fine before merging so we could start testing it in the CI right away.

@josh-cain
Copy link
Author

@vmuzikar sounds great! Totally understand that this needs to be pretty stable before allowing it into the base. I appreciate the work you, @stianst, and @mposolda have put into this.

It looks like testcontainers-java #334 was actually merged today, so hopefully that'll find its way to a release fairly quickly.

Finally, Travis looks to be having some issues with failing tests. Dare I ask what's up with those? They appear to be unrelated, but I've not had massive Travis failures like this before...

@stianst
Copy link
Contributor

stianst commented Jun 26, 2017

Are we ready to go? One last rebase/squash of commits and this can be merged from my POV.

@vmuzikar
Copy link
Contributor

Yes yes - from QE POV we are good to go.

@josh-cain
Copy link
Author

@stianst @vmuzikar attempted to squash this monstrosity of a PR into a commit. Of course there were plenty of conflicts - I think I resolved them correctly, but would just like to call out that the likelihood of error on those is growing with the size/duration.

Will keep an eye on the builds tonight and see of the results go well.

@stianst
Copy link
Contributor

stianst commented Jun 27, 2017

Looks like something went rather wrong and all tests are broken now :(

@vmuzikar
Copy link
Contributor

@cainj13 Hmm... it seems like the test is now failing for me with:
java.lang.RuntimeException: Could not create new instance of class org.jboss.arquillian.test.impl.EventTestRunnerAdaptor Caused by: java.lang.reflect.InvocationTargetException Caused by: org.jboss.shrinkwrap.descriptor.api.DescriptorImportException: Could not import XML from stream Caused by: org.xml.sax.SAXParseException: The content of elements must consist of well-formed character data or markup.

Also, I quickly glanced at the test and it seems like it's not the latest version - there are missing e.g. my changes (waiting for the container - the while loop). Perhaps something more is missing too.

@josh-cain
Copy link
Author

Sorry guys, I'll get to it when I can. Already sunk a ton of time into the PR process (far more time than it took to actually write the implementation), and I've got some other high priority tasking atm. I knew better than to attempt a rebase of this size without a backup, so see dockerProtocolJustInCase branch if you want a snapshot of a pre-rebase version.

@stianst
Copy link
Contributor

stianst commented Jun 27, 2017

@cainj13 I'm sorry about the lengthy time it's taken to get this PR accepted. I'm partially to blame as I should have given it more love early on. Sorry for that. We're almost at the goal line now though so don't give up hope :)

I'm holding of the 3.2 release until we can get this merged and it's one of the last things we're waiting for now.

@stianst
Copy link
Contributor

stianst commented Jun 28, 2017

@cainj13 I'm trying to squash it now. Hopefully I can get this sorted.

@stianst
Copy link
Contributor

stianst commented Jun 28, 2017

Trying to squash this was a disaster in it's own right. So I took a different strategy and uses "git reset" then added a new commit. This looses the author, but I mentioned you as the author in the commit instead. Hope that's OK. After that it was fairly simple to rebase on master, only a few minor merge conflicts to sort out.

https://github.com/stianst/keycloak/tree/docker-rebase-master

Going to run tests and check it out now.

@stianst
Copy link
Contributor

stianst commented Jun 28, 2017

Opened a new PR here #4267. If it passes tests and I can run the Docker tests locally I'll merge that.

@josh-cain
Copy link
Author

@stianst I pulled in your changes - minus trimming of a bit of whitespace in a couple files these branches should now be identical.

I'll examine the tests, still have failures :(

@vmuzikar
Copy link
Contributor

I've just run the test in the CI with the Stian's branch and it passed.
Looks good to me.

@cainj13 What's it failing on for you?

@josh-cain
Copy link
Author

@vmuzikar I was referring to https://travis-ci.org/keycloak/keycloak/builds/247906586 - had failures.

We'll see what Travis does here ^ I suppose...

@josh-cain
Copy link
Author

@stianst @vmuzikar failures are around stuff like this:

UndertowDemoServletsAdapterTest>AbstractDemoServletsAdapterTest.testTokenMinTTL:524 URL expected to begin with:http://localhost:8180/auth/realms/demo/protocol/openid-connect/auth; actual URL: http://0.0.0.0:8180/auth/realms/demo/protocol/openid-connect/auth?response_type=code&client_id=token-min-ttl&redirect_uri=http%3A%2F%2F0.0.0.0%3A8180%2Ftoken-min-ttl&state=76082516-a9f9-4bf1-8868-eeaf57008281&login=true&scope=openid

Looks like changing the bind settings from 'localhost' to '0.0.0.0' made some tests unhappy.

@stianst
Copy link
Contributor

stianst commented Jun 29, 2017

Fixed the issue with the tests and all tests are now passing with auth-server-wildfly and on Travis:
https://github.com/keycloak/keycloak/pull/4269/files#diff-aef353c345932786727b146e60b1f3a9R63

PR merged here #4269

@stianst stianst closed this Jun 29, 2017
@stianst
Copy link
Contributor

stianst commented Jun 29, 2017

@cainj13 We got there in the end! Thanks for all the work :)

@josh-cain
Copy link
Author

w00t w00t! Glad we got there!

Thanks to y'all as well @stianst, @mposolda, and @vmuzikar! I'll keep my eyes out for bugs, and help where I can as far as mailing list/articles go.

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.

None yet

7 participants