pressbooks / pressbooks-saml-sso Goto Github PK

A user writes:

A Shibboleth IdP supports echoing data in the metadata onto its login screen. You may want to look into adding these data elements to your metadata generation process:

1. If you add xmlns:mdui="urn:oasis:names:tc:SAML:metadata:ui" to the <md:EntityDescriptor entityID=...... line before the closing >
2. Then add the following lines after the line starting with:

<md:SPSSODescriptor ...................>
     <md:Extensions>
        <mdui:UIInfo>
            <mdui:DisplayName xml:lang="en">Pressbooks EDU</mdui:DisplayName>
            <mdui:InformationURL xml:lang="en">https://.....................................................</mdui:InformationURL>
            <mdui:PrivacyStatementURL xml:lang="en">https://.........................................................</mdui:PrivacyStatementURL>
            <mdui:Logo height="65" width="161" xml:lang="en">https://pressbooks.org/app/uploads/2018/01/opengraph.png</mdui:Logo>
        </mdui:UIInfo>

The mdui claim has other elements that can be added. The results make the login page look like:

Our plugin attempts to assign the user a username (based on the uid value) and email address (based on the mail attribute):

When parsing the Attribute Statement, we attempt to getAttributesWithFriendlyName first and then fall back to getAttributes if we can't find a UID:

This has caused some problems for clients who do not use the expected FriendlyName attributes for both values. In cases where an attribute with FriendlyName 'uid' is found but not one with 'mail', our flow doesn't failback to getAttributes, rather it just supplies the made up email value derived from the uid.

It would be better for us to use the SAML attribute name (i.e. urn:oid:0.9.2342.19200300.100.1.1 for samAccountName and urn:oid:0.9.2342.19200300.100.1.3 for email-address) instead of the FriendlyNames uid and mail. Justification below.

The NameFormat XML attribute in elements MUST be urn:oasis:names:tc:SAML:2.0:attrname-format:uri ... the Name XML attribute is based on the OBJECT IDENTIFIER assigned to the directory attribute type. Example: urn:oid:2.5.4.3 Since X.500 procedures require that every attribute type be identified with a unique OBJECT IDENTIFIER, this naming scheme ensures that the derived SAML attribute names are unambiguous.

For purposes of human readability, there may also be a requirement for some applications to carry an optional string name together with the OID URN. The optional XML attribute FriendlyName (defined in [SAML2Core]) MAY be used for this purpose. If the definition of the directory attribute type includes one or more descriptors (short names) for the attribute type, the FriendlyName value, if present, SHOULD be one of the defined descriptors.

Two elements refer to the same SAML attribute if and only if their Name XML attribute values are equal in the sense of [RFC3061]. The FriendlyName attribute plays no role in the comparison.
Source: https://docs.oasis-open.org/security/saml/SpecDrafts-Post2.0/sstc-saml-attribute-x500-cd-01.html

A Stack Overflow user writes:

The formal name of the attribute is urn:oid:2.5.4.42 but it also has a human readable name which is 'givenName'. The FriendlyName is only for user interface things like showing the user what attributes have been released. The SP should use the formal name (urn:oid ...) to make authorisation decisions etc. ... You shouldn't rely on FriendlyName for authorisation but should use the SAML2 Name instead. FriendlyName is only to show to users if they want to know which attributes have been released by their IdP.

Source: https://stackoverflow.com/a/48543545

A client writes

You should be using the attribute name instead (in this case urn:oid:0.9.2342.19200300.100.1.3), I would even recommend adding field in the plug-in configuration and allowing user to overwrite the attributes that are used for username or email. Or alternatively add condition to check for both name or friendly name. If the urn is not matching, you can fail to the FriendlyName. Friendly name is only supposed to make the entries in the metadata human readable and not for attribute mapping.

Shibboleth IdP v3 was using definition of attributes such as this:

<AttributeDefinition xsi:type="Simple" id="email">
  <InputDataConnector ref="myLDAP" attributeNames="mail"/>
  <AttributeEncoder xsi:type="SAML1String" name="urn:mace:dir:attribute-def:mail" encodeType="false" />
  <AttributeEncoder xsi:type="SAML2String" name="urn:oid:0.9.2342.19200300.100.1.3" friendlyName="mail" encodeType="false" />
</AttributeDefinition>

you can see the last line of AttributeEncoder contains friendlyName="mail" but the id="email" this was resulting in SAML assertion using correct friendlyName="mail". in new version of IdP the definition was changed to something like this:

        <bean parent="shibboleth.TranscodingProperties">
            <property name="properties">
                <props merge="true">
                    <prop key="id">email</prop>
                    <prop key="transcoder">SAML2StringTranscoder SAML1StringTranscoder</prop>
                    <prop key="saml2.name">urn:oid:0.9.2342.19200300.100.1.3</prop>
                    <prop key="saml1.name">urn:mace:dir:attribute-def:mail</prop>
                    <prop key="displayName.en">E-mail</prop>
                </props>
            </property>
        </bean>

Basically there is no option available and the friendlyName is generated from the attribute id (in this case "email").
We cannot change the id=email as the ID is used in the attribute filter and the attribute filter comes from our portal. Changing the ID would require updating the value on our portal, and on all of 40+ idp servers of all of our clients.

The client also provided an example of configurable settings in a Drupal-based Islandora service provided by Discovery Garden (discoverygarden.ca).

We should consider providing a more descriptive message for the user once they've logged out of Pressbooks after using SSO to let them know that they've been logged out of SSO but may need to close their browser entirely to complete the logout.

A client writes

Usually closing a browser is required to complete a log out session completely, as sometimes if you don't close the close the browser, refreshing or going back to the application in the same session will re-log a user in. Perhaps if a message such as the one you have when you log out of the library is applied on log out (which advises to close the browser to complete the log out) would help.

If you paste https://md.incommon.org/InCommon/InCommon-metadata.xml into the IdP metadata URL field, ie.

Then the plugin will auto-fill the fields using the first found IDPSSODescriptor.

InCommon-metadata.xml contains thousands of entries. The first one might not be the one we want.

\OneLogin\Saml2\IdPMetadataParser::parseRemoteXML can take a 2nd parameter: $entityId

Entity Id of the desired IdP, if no entity Id is provided and the XML metadata contains more than one IDPSSODescriptor, the first is returned.

We should fix Automatic Configuration to use this parameter, as needed.

https://github.com/pressbooks/pressbooks-shibboleth-sso/blob/5d61f5a8d1eead18bf131c233965762521aa245d/inc/class-admin.php#L124-L125

One of our clients has configured our SAML SSO plugin for their IdP, but is consistently seeing a 'logout_not_success' message when they attempt to log out. That error appears to be coming from the OneLogin PHP SAML library: https://github.com/onelogin/php-saml/blob/bb7b82cfa0617b5e2ace4ad7d2bdd8f540f5f54a/lib/Saml2/Auth.php#L261-L262

See https://nuigalwayie-my.sharepoint.com/personal/0097555s_nuigalway_ie/_layouts/15/onedrive.aspx?id=%2Fpersonal%2F0097555s%5Fnuigalway%5Fie%2FDocuments%2FCillian%2Fopenpress%5Fnuigalway%5Flogin%5Fsaml%5Flogout%2Emp4&parent=%2Fpersonal%2F0097555s%5Fnuigalway%5Fie%2FDocuments%2FCillian&originalPath=aHR0cHM6Ly9udWlnYWx3YXlpZS1teS5zaGFyZXBvaW50LmNvbS86djovZy9wZXJzb25hbC8wMDk3NTU1c19udWlnYWx3YXlfaWUvRVN5ekdWcWtMUEJCdTlCZ1JNVEJaaUlCSTBFaFhQSjFPaDNWY1dRS1dOdmd4Zz9ydGltZT15aE1LTDFLUjJFZw (for unsuccessful flow) and https://nuigalwayie-my.sharepoint.com/:v:/g/personal/0097555s_nuigalway_ie/ESCH0BhNNnBCrYh43S917IoBpDTdDE3UQM5ZVf7QB4xU7w for successful flow with another application.

Instead of manual configuration, we could do:

$settings = \OneLogin\Saml2\IdPMetadataParser::parseRemoteXML( 
  'https://www.testshib.org/metadata/testshib-providers.xml' 
);

and fill out the values automatically. Would need a bit of an interface rethink.

@see: https://github.com/onelogin/php-saml/blob/3.0.0/src/Saml2/IdPMetadataParser.php

Replace TravisCI with GitHub Actions. See https://github.com/pressbooks/private/issues/466 for epic

One of our clients reports that their central authentication service allows for usernames with 3 characters. When a user with a 3 character attempts to log in to Pressbooks for the first time, the login fails because the UID doesn't have enough characters to constitute a valid WordPress user name.

Proposed fix: If/when we receive a username from SAML with fewer than four characters, add additional characters to the string to get it up to the minimum length requirement for WordPress usernames (4 characters). The user would continue to use their SAML-based credentials to login and need never know that their PB/WP username differs in any way from their university credential(s).

Allow institutions to send us user's preferred name as an (optional) standard value. Can use displayName, urn:oid:2.16.840.1.113730.3.1.241 (https://commons.lbl.gov/display/IDMgmt/Attribute+Definitions#AttributeDefinitions-displayNamedisplayName)

If we receive it from the institution, save it as the user's Display Name in Pressbooks.

Requested by (for internal purposes only): https://pressbooks.zendesk.com/agent/tickets/3834 (Boise State) & https://pressbooks.zendesk.com/agent/tickets/6451 (UM System)

See: https://github.com/pressbooks/pressbooks-saml-sso/blob/dev/inc/class-saml.php#L445 this function will match the SAML attributes for the PB username.

The default method of encryption for new Shibboleth IdP installations is now AES128-GCM (rather than the older AES128-CBC). We should ensure that we support this method of encryption in our plugin, to reduce problems for users which rely on this for encrypting their outgoing claims. See https://www.ukfederation.org.uk/content/Documents/IdPEncryptionOptions for details.

Appears to be supported in https://github.com/onelogin/php-saml/releases/tag/3.6.0, though more investigation is needed

Summary

Two single institution clients, RMIT and Seneca, encountered an invalid_logout_response error message every time when testing logout during SSO configuration (RMIT in November 2023, Seneca in January 2024).

Logging out and all other aspects of SSO functionally worked for both clients, but this confusing error message appeared every time someone logged out after having logged in with SSO.

Both clients were able to use the same workaround to remove this error message, which was to change the SP Logout URL in their IdP from the SingleLogoutService value in our SP metadata to a generic https://rmit.pressbooks.pub/wp/wp-login.php?action=logout or https://pressbooks.senecapolytechnic.ca/wp/wp-login.php?action=logout. However, this workaround has the side effect of an additional, unwanted prompt upon logout:

More detailed notes from RMIT

See RMIT's explanation of the cause of the error message, the workaround, and the side effect of the workaround here (https://pressbooks.zendesk.com/agent/tickets/18449):

Once the user hits “Log Out” from the Pressbooks application:

The logout URL sending the SAML Logout Request from the app was https://rmit.pressbooks.pub/wp/wp-login.php?action=logout&redirect_to=https%3A%2F%2Frmit.pressbooks.pub%2F&_wpnonce=2059b118fa.

But the SingleLogoutService value in the SP metadata file is https://rmit.pressbooks.pub/wp/wp-login.php?action=pb_shibboleth_sls

Since these two values mismatched, we see an error message "invalid_logout_response"

To fix this, the IDP changed the Single Logout URL returned in the SAML Response to https://rmit.pressbooks.pub/wp/wp-login.php?action=logout so that it matched the URL sending the Logout request.

This worked (Error message didn’t appear again) but added the additional step to the logout process. See below:

1. User hits “log Out” from the app.

2. User gets redirected to the following prompt (additional step):

3. Upon hitting ‘log out’ from the additional prompt, user gets sent to the login page:

This page now shows the correct message “You are now logged out”.

As part of the standardization process we want to make PB PSR compatible, this effort was in the radar before (pressbooks/pressbooks#271).

This task should include

• New directory structure PSR4 compilant
• Rename classes
• Adopt PSR12? coding standards ?
• Refactor some codes to use the new namespaced classes

We have a few problems with the logs:

1. We're logging information a little too late:

   pressbooks-saml-sso/inc/class-saml.php

   Lines 368 to 370 in 6b887a3

   	$this->logData( 'email from SAML attributes', [ $email ] );
   	$this->logData( 'net_id from SAML attributes', [ $net_id ] );
   	$this->logData( 'SAML Settings', [ $this->getSettingsWithoutCertificatesAndPrivateKey() ] );

   
   It would be good to log the contents of self::USER_DATA at the same time we store it to the $attributes variable so we can see what values we have from the claim even before we attempt to set the username and email address. We should have information in the log about the claim information received by Pressbooks in cases when login fails (maybe especially so in this cases). We should also switch the order of lines 369 and 368 so that we log net_id before we log email.
2. We're unneccesarily duplicating some logging information
   
   Each successful login sends the same information twice, except once with some "Auth SAML data" and once without it. See

   pressbooks-saml-sso/inc/class-saml.php

   Line 577 in 6b887a3

   $this->logData( 'Auth SAML data', $log_auth_data, true );

3. The post authentication log already includes all of the information in the two earlier log statements. Not sure why we're producing and recording all three of them.

It would be good to allow network managers to download and inspect these logs from the SSO config page, as we allow with the OIDC plugin's logs.

Finally, logs do not appear to be automatically configured the plugin is activated or used in our production network. Not sure what needs to be done so that necessary S3 buckets or CloudWatch groups are created, but if the plugin is active on one of our hosted networks, logs should automatically be kept.

Currently when users login using the generic Pressbooks/WordPress login routine, the user can specify a post-login redirection location in the login URL by appending redirect_to=URL to the login address, e.g. https://mynetwork.url/wp-login.php?redirect_to=https%3A%2F%2Fmynetwork.url%2Fwp-signup.php (takes the user to the book registration page) or https://mynetwork.url/wp-login.php?redirect_to=https%3A%2F%2Fmynetwork.url%2Fwp-admin (takes the user to the Pressbooks dashboard). When a user attempts to use this URL pattern in combination with login via our SSO method, however, no redirection occurs after login (the login URL is sanitized as part of the SSO login routine).

See

Users who register using SSO do not send a notification to the email address put in "Email Notifications" in Network Admin > Settings > Book & User Registration.

This behaviour makes sense to an extent, in that, SSO provisions the account upon registration which is what the email notifications desires to do by alerting the administrator receiving the email.

This ticket was sent in by Western Sydney University.

We currently show the message 'The Pressbooks SAML Plugin is not configured correctly' when the plugin is active but not configured. This is the ideal message to show when configuration has been attempted, but with incorrect values such as an invalid URL.

On networks where the plugin is active but the fields for configuration haven't been filled out, the message about the plugin not being configured correctly has confused some network managers, who believe that something has gone wrong when in fact the configuration hasn't been attempted yet.

Proposed solution

Use conditional logic to treat empty configuration fields differently from filled-out-but-incorrect fields:

• If the fields are empty, display this warning: 'The Pressbooks SAML Plugin has not been configured yet.'
• If the fields are filled out, but aren't filled out correctly, display the current message: 'The Pressbooks SAML Plugin is not configured correctly.'

In each case, the word configured can link to the SAML configuration page at https://NETWORKURL.wp-admin/network/admin.php?page=pb_saml_admin

We also want to move the placement of this and other alerts/warnings that appear for network managers from their current location in the top of the new network manager dashboard to a dismissable block between the stats & Configure home page blocks.

Notes

current behaviour comes from:

On some networks, new users are being created with unexpected characters (a) appended to the end of their username.
This is not happening for all clients, but is happening for the values being provided by NUI Galway and KPU, at least. See sample screenshots of user accounts from integrations, NUI Galway, and KPU:

In one example, the client informs us that IdP is passing the following claim information:

and the resulting Pressbooks user looks like this:

A client reports:

I am trying to set up our network to use SSO. It looks like the settings are correct on our side (Azure SSO) and on your side in the pressbooks app. But on our side, we use a multifactor authentication setup via a system called Duo. In my troubleshooting inside the Microsoft Azure portal, the system gave me this information. The portal asks us to copy/paste any errors we receive during troubleshooting so I did and here are the suggestions/resolutions suggested:

I believe that both of these are largely handled by the php-saml library we rely on. See https://github.com/onelogin/php-saml/blob/20a2cb3e2c2722f4a06d2b16006d5e140adb48db/lib/Saml2/AuthnRequest.php#L90-L132

What we should try first is to set the Authentication context to false. See https://github.com/onelogin/php-saml/blob/master/README.md

// Authentication context.
        // Set to false and no AuthContext will be sent in the AuthNRequest.
        // Set true or don't present this parameter and you will get an AuthContext 'exact' 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport'.
        // Set an array with the possible auth context values: array ('urn:oasis:names:tc:SAML:2.0:ac:classes:Password', 'urn:oasis:names:tc:SAML:2.0:ac:classes:X509').
        'requestedAuthnContext' => false,

When a user logins from the main Pressbooks landing page they are currently returned to that page as a logged in user. They can't do much except create a new book, however.

A better experience would be the following:

If the user does not have an existing book -- they are redirected to https://network.url/wp/wp-admin/user
If the user does have an existing book -- they are redirected to https://network.url/wp/wp-admin
See TK for current login-redirection behavior

Pseudo code with possible solution at https://github.com/pressbooks/pressbooks-oidc-sso/issues/28#issuecomment-707330562

We can also control what appears to the user on the user dashboard page by looking at https://github.com/pressbooks/pressbooks/blob/716d50e7d65c27c1e5acef9379fa19069855e039/inc/admin/dashboard/namespace.php#L50-L161

Includes:
#46
#49
#61

Aldine was changed so that users are redirected to their relevant dashboard following login from the main site. Users who login via SSO are unaffected, however. These users should also be directed to the correct admin dashboard when logging in from the main site. See comments on pressbooks/pressbooks-aldine#339

One of our clients loaded his network and saw that he was logged in as someone else. Note that he did not log in explicitly -- he just launched the page. The person he was logged in as has never accessed their account on his computer. They do have LTI 1.1 enabled, but he has never logged in as her, nor does the book they both work on use LTI 1.1 to launch their content.

Here is where he emailed us: https://pressbooks.zendesk.com/agent/tickets/4390

The colour contrast for the string "OR" on the login page is insufficient. It produces an error with accessibility checkers like the WAVE plugin. The current contrast is 4.47 (text-color: #777; background-color: #fff), it should be at least 4.5.

Link to the line in the code:

As a Pressbooks network manager, I want to set up my network to work smoothly with my enterprise IdP as quickly as possible. One of the biggest blockers to speedy SSO configuration with this plugin is the current requirement that incoming claims/Responses be both signed AND encrypted. Over the past few years, several IdP administrators have expressed confusion/frustration in setting up the additional encryption step. Apparently, it is unusual for other SPs they work with in higher education to require encrypted claims.

We can help address this problem by making claim/response encryption optional, rather than requiring it by default. Here's the line where that's set in our code:

Notes

Here's two people online describing what encryption means in this context: https://security.stackexchange.com/a/204686 + https://www.componentspace.com/forums/FindPost8820.aspx. Because the claims/Responses sent to us are always transmitted via SSL and don't generally contain sensitive information (Pressbooks only requires a unique identifier and an email address), I think the security concerns/risks involved here would be neglible. Concerned IdPs could of course still encrypt claims/Responses if they so desired, they just wouldn't be required to do so to complete the authentication flow.