Use OneLogin's PHP SAML library for SAML auth without SimpleSAMLphp #68

danielbachhuber · 2017-05-01T23:17:02Z

See #60

Sessions aren't shared between WP SAML Auth and SimpleSAMLphp

This reverts commit 3c730c3.

danielbachhuber · 2017-05-24T14:24:51Z

Interestingly, here are the cookies that are present in the SimpleSAMLphp login screen locally:

And here's what's in the test environment:

This reverts commit b377941.

…sion"" This reverts commit f17ea23.

danielbachhuber · 2017-05-24T17:07:01Z

Downgrading SimpleSAMLphp seems to have fixed the cookie issue.

However, for whatever reason, the login form doesn't seem to want to submit:

Or, it's submitting and returning "invalid username / password".

WIP - Test fix for one login branch

danielbachhuber · 2017-05-26T01:44:19Z

@stevector This is finally ready for a review.

In addition to reading through the code, it'd be helpful to have you attempt to configure this locally... following the instructions in the readme, and suggesting clarifications where you have ideas.

If you try to use Auth0 as a SAML IdP, you may get stuck. The last time I tried, Auth0 wouldn't pass responses back to URLs ending with wp-login.php, only the homepage. SAML responses are processed as a part of the login / authentication process, so having your IdP send responses to wp-login.php is a requirement at this point.

I ended up setting up SimpleSAMLphp as a SAML IdP locally and it worked well enough. You could copy the necessary configuration out of the Behat setup steps.

stevector · 2017-05-29T20:29:11Z

Thanks @danielbachhuber! I'm booked up solid early in the week. I've blocked time on Friday to review.

danielbachhuber · 2017-06-15T08:01:57Z

@stevector How's the review coming along? I'd love to get this landed and out the door :)

stevector

Hi @danielbachhuber, I got this PR working on a Pantheon site today.

I added some minor suggestions/requests inline.

My overall impression is that it still takes a lot of wrangling to get this plugin up and running. I don't want to block the PR on this, but I'm tempted to sign up for a GSuite account just to make myself an admin of an easy identity provider that would could write tests/instructions against. Using an ID provider on the same server makes the process confusing.

stevector · 2017-06-02T21:20:49Z

inc/class-wp-saml-auth.php

-		$this->provider = new SimpleSAML_Auth_Simple( self::get_option( 'auth_source' ) );
+		$connection_type = self::get_option( 'connection_type' );
+		if ( 'internal' === $connection_type ) {
+			$autoload = dirname( __DIR__ ) . '/vendor/autoload.php';


Let's allow for the possibility of an all-Composer setup where vendor directory would not be within the plugin directory.

We handled that in LCache like this: https://github.com/lcache/wp-lcache/blob/master/object-cache.php#L12

stevector · 2017-06-02T21:23:53Z

README.md


    function wpsax_filter_option( $value, $option_name ) {
        $defaults = array(
            /**
+             * Type of SAML connection bridge to use.
+             *
+             * 'internal' uses OneLogin bundled library; 'simplesamlphp' uses SimpleSAMLphp.


Decisions, not options.

😄

Should we be deprecating SimpleSAMLphp at some point?

Should we be deprecating SimpleSAMLphp at some point?

At this point, I'm leaning towards no. The cost of us maintaining the SimpleSAMLphp implementation is trivial compared to the cost of forcing existing users to migrate to OneLogin. Plus, I'm not positive there's feature parity between SimpleSAMLphp and OneLogin for everything users may be using (e.g. being able to select between 2 IdPs).

stevector · 2017-06-22T19:41:32Z

README.md

@@ -1,43 +1,94 @@
 # WP SAML Auth #


Can you draft the Changelog entry?

I typically do this just before release.

stevector · 2017-06-22T19:43:47Z

circle.yml

@@ -51,4 +51,5 @@ test:
  override:
    - ./bin/behat-test.sh --strict
  post:
-    - ./bin/behat-cleanup.sh
+    # - ./bin/behat-cleanup.sh
+    - echo 'disabled'


This line will need to change back before merge.

stevector · 2017-06-22T19:49:43Z

tests/behat/login.feature

@@ -9,6 +9,8 @@ Feature: SAML Login
    And I fill in "username" with "employee"
    And I fill in "password" with "employeepass"
    And I press "submit"
+    And print last response


This line can be taken out.

stevector · 2017-06-22T20:38:07Z

inc/class-wp-saml-auth.php

+				add_action( 'admin_notices', function() {
+					if ( current_user_can( 'manage_options' ) ) {
+						// Translators: Links to the WP SAML Auth plugin.
+						echo '<div class="message error"><p>' . wp_kses_post( sprintf( __( "WP SAML Auth wasn't able to find the <code>SimpleSAML_Auth_Simple</code> class. Please check the <code>simplesamlphp_autoload</code> configuration option, or <a href='%s'>visit the plugin page</a> for more information.", 'wp-saml-auth' ), 'https://wordpress.org/plugins/wp-saml-auth/' ) ) . '</p></div>';


This is the message I see after just enabling the module (without doing an config). It suggests that SimpleSAML, rather than OneLogin is the preferred option. I think we should consider flipping some ifs/else statements.

Because I've introduced a new connection_type=>(internal|simplesamlphp) configuration argument, the default connection type still needs to be simplesamlphp to avoid a backwards compatibility break on existing installs.

The code path is such that you'll see one error message with one connection type, and the other with the other connection type. You must've had connection_type=simplesamlphp at the time.

stevector · 2017-06-22T20:53:20Z

README.md


 See installation instructions for full configuration details.

 ## Installation ##

-This plugin requires access to a SimpleSAMLphp installation running in the same environment. If you are already running SimpleSAMLphp, then you are good to go. Otherwise, you'll need to install and configure SimpleSAMLphp before you can begin using this plugin. For local testing purposes, the [Identity Provider QuickStart](https://simplesamlphp.org/docs/stable/simplesamlphp-idp) is a good place to start.
+This plugin requires access to a functioning SAML identity provider. If all you need is SAML authentication, then you should use the bundled OneLogin SAML library. If you have more complex authentication needs, then you can also use a SimpleSAMLphp installation running in the same environment.


Can you split up the code snippets that follow such that there is one for OneLogin and one for SimpleSAMLphp?

Why? There's more overlap between them than there are differences.

stevector · 2017-06-22T21:30:07Z

bin/fixtures/functions.php

+        return $value;
+    }
+    if ( 'user_login_attribute' === $option_name ) {
+        return 'urn:oid:0.9.2342.19200300.100.1.1';


Where did these values come from?

I sniffed it out of the SAML response. It looks like this is SAML 2 spec though: https://commons.lbl.gov/display/IDMgmt/Attribute+Definitions#AttributeDefinitions-uiduid

danielbachhuber · 2017-06-28T13:16:20Z

My overall impression is that it still takes a lot of wrangling to get this plugin up and running. I don't want to block the PR on this, but I'm tempted to sign up for a GSuite account just to make myself an admin of an easy identity provider that would could write tests/instructions against. Using an ID provider on the same server makes the process confusing.

Could you clarify on this? Were you confused specifically about the testing implementation, or the testing implementation and plugin configuration generally?

stevector · 2017-06-29T15:01:00Z

@danielbachhuber and I were just chatting about this on Slack. I think we're good to go.

danielbachhuber added 23 commits May 1, 2017 16:15

First pass at incorporating OneLogin's SAML library

46af7ca

Exclude some directories from wpcs

b5acb55

Merge branch 'master' into 60-one-login

91cc853

Abstract OneLogin configuration to config pattern

a68a5f6

Explain why we're using wp_login_url()

1e3c9a2

Don't need to load WordPress, because we're using SimpleSAMLphp as idp

9682007

SimpleSAMLphp works fine with wp-login.php as the URL

45521dc

Provide more contextual information to errored processing

94ab4b6

Better clarification of configuration options

d4fe920

Make this more generic

e607d26

Include SingleLogoutService in default config

e95f031

WP Native PHP Sessions is no longer necessary

3c730c3

Sessions aren't shared between WP SAML Auth and SimpleSAMLphp

Use SimpleSAMLphp as idp for testing

c7fbbe1

We need the vendor directory now

d446945

Update PHPUnit tests for new query arg

98d46f4

Don't permit WP login, to force redirect to SimpleSAMLphp

a9b228b

Properly handle SAMLResponse when permit_wp_login=true

b65f232

Disable cleanup so I can see what's errored

4a6ca15

wp-saml-auth is still necessary to activate

aa344b7

Use session file handler, just for testing purposes

2254b8a

Fix filenames for SAML config

401cf72

Revert "WP Native PHP Sessions is no longer necessary"

45b403d

This reverts commit 3c730c3.

Load WP again to use WP Native PHP Sessions

db88bd4

danielbachhuber added this to the 0.3.0 milestone May 12, 2017

danielbachhuber mentioned this pull request May 23, 2017

Consider using OneLogin Simple SAML toolkit for PHP #60

Closed

Merge branch 'master' into 60-one-login

96b6513

danielbachhuber added 3 commits May 24, 2017 09:00

Merge branch 'master' into 60-one-login

951e155

Strip out broken JavaScript that prevents form submission

b377941

Revert "Strip out broken JavaScript that prevents form submission"

f17ea23

This reverts commit b377941.

danielbachhuber added 3 commits May 24, 2017 09:44

Revert "Revert "Strip out broken JavaScript that prevents form submis…

92951cd

…sion"" This reverts commit f17ea23.

Remove id attribute from second button instance

d71b21a

Fix id attribute stripping

b65fdee

stevector and others added 7 commits May 24, 2017 17:06

Hacky way to get tests to pass

7f2d27a

Merge pull request #79 from pantheon-systems/60-one-login--test-hack

7446c08

WIP - Test fix for one login branch

Update readme description to reflect OneLogin

f0218d8

Commit composer.lock to lock to a known state

a30b0df

Copy over new configuration details

cdfb544

Clarify which strings should be used

4f603bc

Further clarify comments

9bfe355

danielbachhuber requested a review from stevector May 26, 2017 01:39

Clarify setting wp-login.php as the callback URL

0fe32b3

Merge branch 'master' into 60-one-login

7bad7a5

stevector requested changes Jun 22, 2017

View reviewed changes

danielbachhuber mentioned this pull request Jun 28, 2017

Way to specify which IDP to use #82

Closed

danielbachhuber added 2 commits June 28, 2017 06:04

Restore environment cleanup

aaa0a3a

Remove debugging step

2bd0d04

danielbachhuber added 2 commits June 28, 2017 06:28

Abstract autoloader definition to an overrideable constant

c9368da

Mention origin of this special value

f7ead3a

stevector approved these changes Jun 29, 2017

View reviewed changes

danielbachhuber merged commit 7bcb8d9 into master Jun 29, 2017

danielbachhuber deleted the 60-one-login branch June 29, 2017 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use OneLogin's PHP SAML library for SAML auth without SimpleSAMLphp #68

Use OneLogin's PHP SAML library for SAML auth without SimpleSAMLphp #68

danielbachhuber commented May 1, 2017

danielbachhuber commented May 24, 2017

danielbachhuber commented May 24, 2017

danielbachhuber commented May 26, 2017

stevector commented May 29, 2017

danielbachhuber commented Jun 15, 2017

stevector left a comment

stevector Jun 2, 2017

danielbachhuber Jun 28, 2017

stevector Jun 2, 2017

danielbachhuber Jun 28, 2017

stevector Jun 22, 2017

danielbachhuber Jun 28, 2017

stevector Jun 22, 2017

danielbachhuber Jun 28, 2017

stevector Jun 22, 2017

danielbachhuber Jun 28, 2017

stevector Jun 22, 2017

danielbachhuber Jun 28, 2017

stevector Jun 22, 2017

danielbachhuber Jun 28, 2017

stevector Jun 22, 2017

danielbachhuber Jun 28, 2017

danielbachhuber commented Jun 28, 2017

stevector commented Jun 29, 2017

Use OneLogin's PHP SAML library for SAML auth without SimpleSAMLphp #68

Use OneLogin's PHP SAML library for SAML auth without SimpleSAMLphp #68

Conversation

danielbachhuber commented May 1, 2017

danielbachhuber commented May 24, 2017

danielbachhuber commented May 24, 2017

danielbachhuber commented May 26, 2017

stevector commented May 29, 2017

danielbachhuber commented Jun 15, 2017

stevector left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielbachhuber commented Jun 28, 2017

stevector commented Jun 29, 2017