Support check-sso option in the Node.js adapter

[bernardopacheco]

Hi all,

A check SSO will only authenticate the client if the user is already logged-in, if the user is not logged-in the browser will be redirected back to the originally-requested URL and remain unauthenticated.

The 'check-sso' is really important for public websites that don't require a user to be logged in to see the page, but still, the server needs to know the current user's auth status in Keycloak servers. Currently, the Node.js adapter only provides a protect() middleware that means 'login-required', i. e., an unauthenticated user is always redirected to the login page.

We can implement the same 'check-sso' option from the Keycloak Javascript library into the Node.js adapter using response_mode=query and prompt=none during the communication with Keycloak servers.

Example:

app.get('*', keycloak.checkSso(), (req, res) => {
  const isLoggedIn = !!req.session['keycloak-token'];
  res.render('index', { 'isLoggedIn': isLoggedIn });
});

This way the index.html file can be served carrying the auth info to the client.

Ticket reference: https://issues.jboss.org/browse/KEYCLOAK-10043

Checklist

• Integration tests
• Squash the commits into a single one
• Update the documentation

[bernardopacheco]

More details about each change being proposed.

[pedroigor]

Hi @bernardopacheco, could you also provide tests for the check-sso functionality ?

[bernardopacheco]

Hi @pedroigor, thanks for following this up. Sure thing, I'm trying to add tests but I saw some issues.

I'm running make to run the test suite, but some of them are failing. For example, there is an invalid function reference:

# GrantManager should be ensure that a grant is fresh
(node:4259) UnhandledPromiseRejectionWarning: TypeError: extend is not a function
    at manager.obtainDirectly.then (/Users/bernardo/src/zwift/keycloak-nodejs-connect/test/grant-manager-spec.js:395:23)

If I commented it out, the tests continued being executed, but there was another fail in the sequence:

# Should be able to access resource protected by the policy enforcer
ok 126 User should not be authenticated
ok 127 User should be authenticated
ok 128 User can access resource protected by the policy enforcer
# teardown
# setup
# Should be able to access public page

It stays here forever. It seems that page.output().getText() never return.

The last issue I saw was in the documentation. It says to add tests on the keycloak-connect-test.js, but this file does not exist:

Bug fixes and features should come with tests. Add your tests in the keycloak-connect-test.js file.

Could you please help me with this?

Regards,

Bernardo

[bernardopacheco]

Hi @pedroigor, I managed to add a test for the Check SSO middleware.

Could you please review it?

However, I still keep my considerations from my last comment. I see some tests failing when I run make and there is no keycloak-connect-test.js file, so I added my test in the keycloak-connect-web-spec.js file.

Instead of running make I could check my test running node test/keycloak-connect-web-spec.js.

Thanks,

Bernardo

[pedroigor]

@bernardopacheco, thanks.

@abstractj, this one looks good to me. What about you ?

[abstractj]

@pedroigor in order to get this change merge we need to merge first #191. After @bernardopacheco reported the issues with the test suite I found out that the logic on Travis was giving false positives. If we merge that first, we are going to make sure that everything is working fine with this PR.

@bernardopacheco you're welcome to provide your feedback on #191 too. Thanks for your contribution. Could you please squash your commits into a single one?

Another thing is, because we're introducing a new functionality we need to update the docs here https://github.com/keycloak/keycloak-documentation/blob/master/securing_apps/topics/oidc/nodejs-adapter.adoc. I believe one section before Protecting Resources should be enough.

Let me know if you have any questions.

[abstractj]

@bernardopacheco #191 was already merged and I did a rebase and all the tests are passing. I'm going to add a checklist of what's missing in the description of this PR so we can tackle the remaining items.

[bernardopacheco]

Hi @pedroigor, @abstractj, it's my pleasure to contribute and thank you all for reviewing this PR and fixing the integration tests.

@abstractj I squashed the commits into a single one. I also submitted a PR to update the Keycloak documentation as requested: keycloak/keycloak-documentation#644

However, Travis is complaining about a test on the docs:

Failed tests: checkExternalLinks(org.keycloak.documentation.test.SecuringAppsTest): Invalid links

I tried to figure out why the testing is failing but without luck. Could you please help me with this?

Thanks,

Bernardo

[abstractj]

@bernardopacheco Hi Bernardo, don't bother about that, we can handle it. We are in the middle of a release, so I will just hold the merge until I get a positive answer that we can merge safely. I believe won't take too much time.

Thanks for the updates to the docs.

[bernardopacheco]

That's great, @abstractj, thank you. Let me know if you need anything.

[abstractj]

@pedroigor @bernardopacheco this PR was superseded by #195. What changed was just the commit message, the codebase was left exactly the same as it was submitted in this PR.

@bernardopacheco thanks for your contribution.

[andymunro]

I am happy to give input, but I am not sure which part needs review.

[bernardopacheco]

Thank you @abstractj and @pedroigor, it was a pleasure!

[abstractj]

@andymunro Hi Andy, I believe your input is needed here keycloak/keycloak-documentation#644. Thank you and have a great weekend.