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

Implemented passive authentication support for Shibboleth #3762

Merged
merged 2 commits into from
Apr 14, 2017
Merged

Implemented passive authentication support for Shibboleth #3762

merged 2 commits into from
Apr 14, 2017

Conversation

sudoflyy
Copy link

@sudoflyy sudoflyy commented Apr 11, 2017

Related Discussion Thread:

https://groups.google.com/forum/#!topic/dataverse-community/Fc0wC4fLyeI

Pull Request Checklist (needs review)

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 10.103% when pulling 320b924 on aivanov100:develop into 308f169 on IQSS:develop.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

It would be nice to document this new setting but QDR might be the only ones using it for now. We could always document it later.

/**
* Whether Shibboleth passive authentication mode is enabled
*/
ShibPassiveLoginEnabled;
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, you'd document this new database setting so that it appears (near the bottom) of a future version of http://guides.dataverse.org/en/4.6.1/installation/config.html#database-settings

@djbrooke
Copy link
Contributor

We talked about this in standup 4/11 and everyone is fine with the code review from @pdurbin here.

Moving to QA.

@pdurbin
Copy link
Member

pdurbin commented Apr 12, 2017

Vaguely related to #3765.

Not a solution for #3535 (sort of the opposite) but related.

@kcondon
Copy link
Contributor

kcondon commented Apr 13, 2017

@aivanov100
Is the configuration for this simply:

  1. enable Shibboleth according to existing instructions
  2. set :ShibPassiveLoginEnabled to be true

I was able to see isPassive.js passed when set to true and not when false. However, I thought I'd read something about enabling lazy sessions.

@sudoflyy
Copy link
Author

sudoflyy commented Apr 13, 2017

@kcondon yes, your basic configuration for testing this feature is correct

I would just add that there is an additional configuration change that should be made at the Service Provider level to improve error-handling for passive authentication. It is documented here:
https://wiki.shibboleth.net/confluence/display/SHIB2/isPassive

The Shibboleth wiki suggests two approaches:
A) add redirectErrors="#THIS PAGE#" to the Errors element. Make sure #THIS PAGE# is protected with a lazy session
B) As of SP 2.2 you can set the ignoreNoPassive on your AssertionConsumerService. If you don't have an but only an element (new simplified configuration), it is enough to add a conf:ignoreNoPassive="true" attribute to it.

For our QDR implementation I tried both approaches. Although Approach B is simpler to configure, I found that it did not work well in practice. With this configuration, an anonymous user to our Drupal site would be directed to the IdP and then re-directed back to the Drupal path https://stage.qdr.org/?q=shib_login/home instead of being re-directed to the original page that was requested. Also, for some reason an anonymous user to our Dataverse site would be re-directed to the IdP login page and not re-directed back to Dataverse at all, which is not the intended effect because the anonymous user should not see the IdP login page until they attempt to access content that requires a login.

Due to these shortcomings of Approach B I decided to use Approach A. I set the redirectErrors attribute equal to the Drupal homepage url for the Drupal configuration, and the Dataverse homepage url for the Dataverse configuration. I'm using an ApplicationOverride in our shibboleth2.xml configuration, so it looks something like this:
<Errors ... redirectErrors="https://stage.qdr.org"/> <ApplicationOverride id="dataverse" entityID="https://dv.stage.qdr.org/shibboleth" attributePrefix="AJP_"> <Errors ... redirectErrors="https://dv.stage.qdr.org"/> </ApplicationOverride>

@sudoflyy
Copy link
Author

@pdurbin @kcondon I've added documentation for this feature
modified: doc/sphinx-guides/source/installation/config.rst

Let me know if my documentation is too verbose or if any changes are needed to the text. Thanks.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 10.103% when pulling f524f0a on aivanov100:develop into 308f169 on IQSS:develop.

@kcondon
Copy link
Contributor

kcondon commented Apr 14, 2017

Thanks, verified isPassive.js is passed when enabled, unauthorized access to pages is still prevented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants