OpenAthens integration

[Julian Clementson]

This is a new feature proposal, to provide integration between Evergreen and OpenAthens, a global cloud-based single sign-on service.

Background

The GALILEO Consortium of libraries in Georgia has selected OpenAthens to deliver a state-wide solution for single sign-on, and this contract includes integrating Evergreen into OpenAthens, so that PINES patrons can seamlessly access OpenAthens-authenticated resources.

The OpenAthens development team has been contracted to implement the integration on behalf of GPLS and demonstrated a proof of concept to selected representatives of the Evergreen community and the PINES consortium in July 2019. The aim is to get this feature accepted into an upcoming release so that it is ready for PINES to start using towards the end of the year.

DocuWiki page: https://wiki.evergreen-ils.org/doku.php?id=dev%3Aproposal%3Aopenathens_integration

Code and documentation ready for review:
https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jclementson/lp1842297_openathens_integration

[Galen Charlton (gmc)]

I would be interested in testing this. Is there any chance that a test OpenAthens system could be made available?

[Julian Clementson (oajulianc)]

Thanks Galen, I'll find out if/when we can make an account available for testing. OpenAthens doesn't have the concept of free trial; test accounts are normally only created for customers who are in the process of onboarding. I know Chris Sharp will be interested in testing this. I'll find out more.

[Chris Sharp (chrissharp123)]

Found an issue testing this on a stock master machine. First-time install from the openathens-sso-integration branch installs the application layer code, but not the DB schema changes. I corrected that at the very tip of https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp1842297_openathens_integration. We will be testing this on realistic PINES data over the next few weeks.

[Chris Sharp (chrissharp123)]

Found another issue - the ADMIN_OPENATHENS permission needed to added to the perm list. Done in the same branch as above.

[Julian Clementson (oajulianc)]

Great to see you starting to test this Chris. I've confirmed that we're not in a position to set up test accounts for anyone who isn't already signed up to OpenAthens, so yours will be the tests that count. Let me know if you have any issues on the OpenAthens account side.

[Chris Sharp (chrissharp123)]

(Referencing https://git.evergreen-ils.org/?p=working/Evergreen.git;a=blob;f=docs/RELEASE_NOTES_NEXT/Administration/OpenAthens_SignOn.adoc;h=1c526d3d5093389247f2e7a3a0a014bec1b6171e;hb=refs/heads/user/csharp/lp1842297_openathens_integration)

After setting up the OpenAthens-side configuration, I entered a library account within Evergreen, then after not seeing it appear on the screen after save, I created it again. Then realized (duh) that the "+ Descendants" box was not checked, and when I attempted to delete the duplicate (Actions for Selected Rows -> Delete Selected), the row is not deleted and I see this in the JS console:

ERROR TypeError: "this.pcrud is undefined"
    deleteSelected https://example.org/eg2/en-US/5.154ad4ac26ce40a418c8.js:1
    performAction https://example.org/eg2/en-US/3.13e4e9d394b649459a22.js:1
    g https://example.org/eg2/en-US/3.13e4e9d394b649459a22.js:1
    handleEvent https://example.org/eg2/en-US/main.6d2a2c5a91d2be9f3634.js:1
    handleEvent https://example.org/eg2/en-US/main.6d2a2c5a91d2be9f3634.js:1
    na https://example.org/eg2/en-US/main.6d2a2c5a91d2be9f3634.js:1
    Da https://example.org/eg2/en-US/main.6d2a2c5a91d2be9f3634.js:1
    V https://example.org/eg2/en-US/main.6d2a2c5a91d2be9f3634.js:1
    invokeTask https://example.org/eg2/en-US/polyfills.6a11e44a9ce83c8cb5ef.js:1
    onInvokeTask https://example.org/eg2/en-US/main.6d2a2c5a91d2be9f3634.js:1
    invokeTask https://example.org/eg2/en-US/polyfills.6a11e44a9ce83c8cb5ef.js:1
    runTask https://example.org/eg2/en-US/polyfills.6a11e44a9ce83c8cb5ef.js:1
    invokeTask https://example.org/eg2/en-US/polyfills.6a11e44a9ce83c8cb5ef.js:1
    m https://example.org/eg2/en-US/polyfills.6a11e44a9ce83c8cb5ef.js:1
    b https://example.org/eg2/en-US/polyfills.6a11e44a9ce83c8cb5ef.js:1
main.6d2a2c5a91d2be9f3634.js:1:193550

[Julian Clementson (oajulianc)]

I'll see if I can reproduce this when I have moment. I've basically just copied the crud process from one of the other library settings, so this is most likely an existing bug. It shouldn't prevent you testing the rest of OpenAthens functionality as it just takes the first setting it can find for the library if there are duplicates.

[Jane Sandberg (sandbergja)]

I added a rebased version of this branch at user/sandbergja/lp1842297_openathens_integration.

My branch:
* includes sign offs for Chris' commits. I'm not able to test the actual integration with OpenAthens, so I did not sign off on Julian's commit.
* bumps up the ID of the new permission
* fixes a small issue with the install SQL (a foreign key was declared before both tables had been created)
* fixes the issue with deleting entries in the UI noted by Chris in comment #6. Basically, Angular dependency injection does not play nicely with class inheritance, at least where Components are concerned. More info here: https://angular.io/guide/dependency-injection-in-action#inject-into-a-derived-class -- seems like a pretty obnoxious shortcoming in DI, actually!

Despite that fix, I think that a better way to handle the Angular interface would be to just rely on the auto-generated interface for config.openathens_identity. Julian: I noticed that you put the fields in the fm-editor into a more user-friendly order. The auto-generated UIs don't have that ability yet, but I don't think it would be difficult to add (my vote would be to take Bill's approach mentioned in this comment: https://bugs.launchpad.net/evergreen/+bug/1857351/comments/2). As soon as we add that ability to propagate fieldOrder through route data to the fm-editor, I think the auto-generated admin UI would be able to re-create all of the functionality of Julian's interface.

[Julian Clementson (oajulianc)]

Thanks Jane. I was just starting to think about how to get this moving again.

As it happens I'm learning Angular for some other projects as well now, so you DI fix makes sense.

Chris: I'm still available to guide you through OpenAthens testing when you have time. I hope we can get this signed off in good time for 3.6.

[Jane Sandberg (sandbergja)]

Thanks, Julian! Yes, this feature has waited long enough. :-)

One request: would you be willing to write some tests for OpenAthens.pm? Here's our quick intro to testing Evergreen's perl code: https://wiki.evergreen-ils.org/doku.php?id=dev:contributing:qa#perl_unit_tests. Having some tests would make me feel more confident about the community's ability to maintain your code long-term.

[Julian Clementson (oajulianc)]

Jane: yes, I should get some time to add some tests in the next few weeks. The code relies on OpenAthens as a third party service but hopefully the test framework allows this to be mocked.

[Julian Clementson (oajulianc)]

* Pulled in some previous fixes
* Started adding unit tests (work in progress)

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jclementson/lp1842297_openathens_integration

[Julian Clementson (oajulianc)]

Hi Jane,

I've finished adding Perl unit tests. I couldn't see any examples of Angular tests for any equivalent components so I haven't added anything there.

I've pushed to:
https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jclementson/lp1842297_openathens_integration

This also includes a couple of other fixes I made earlier in the year that you didn't have on your branch.

[Julian Clementson (oajulianc)]

Rebased again and updated adocs format for Antora

[Julian Clementson (oajulianc)]

Pushed some recent fixes arising from PINES testing

[Julian Clementson (oajulianc)]

Fixed:
* a second enabled OpenAthens config was being ignored if there was a first OpenAthens config that was disabled. Docs have been updated to explain that only the first enabled connection will be used.
* optional patron attributes were being sent to OpenAthens with the wrong names.

[Terran McCanna (tmccanna)]

Pull request removed as OpenAthens works on additional fields.

[Julian Clementson (oajulianc)]

Added pullrequest tag. OpenAthens completed all requested enhancements.

[Julian Clementson (oajulianc)]

Squashed and rebased again. Signed off by developer.

[Julian Clementson (oajulianc)]

Squashed and rebased

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/jclementson/lp1842297_openathens_integration

[Julian Clementson (oajulianc)]

@tmccanna I hope we can get this finally signed off this time - @chrissharp123 has been running this on production as a patch for some time. I can rebase it again if needed.

[Jane Sandberg (sandbergja)]

Thanks for your continued work on this, Julian. This code looks good to me, and the tests make me feel much more confident (thanks for bringing in the mocking library too). I've signed off on this code, and added a follow-up to implement my suggestion from the last paragraph of comment #8: user/sandbergja/lp1842297_openathens_integration_signoff

I'll plan to merge this next Wednesday (9/14), to give a chance for any PINES folks to comment on their experience with this in prod and anyone else who may wish to review this.

[Julian Clementson (oajulianc)]

Thanks Jane. I can rebase again before then if it helps - was last done in April.

The only conflict you're likely to get in the merge is the database ID for the new admin permission, which will need bumping up again if there have been new permissions added on master since the last rebase. (Those IDs really ought to be GUIDs instead of integers to avoid this problem!)

PINES folks will need to be aware of that ID changing if/when they upgrade to the next release instead of using a patch for this feature.

[Jane Sandberg (sandbergja)]

I'll never say no to a rebase :-)

[Chris Sharp (chrissharp123)]

Yes, Julian, please rebase - thank you Jane for reviewing this! PINES has run this in production for years now with no issues on our end. The difficulty in testing has been that most of the "does it work?" questions were only answerable by our sibling organization GALILEO, who have been rolling out the feature on their end bit by bit over the last couple of years. I think once the code is rebased I'm happy to add my signoff and Jane can merge.

Nice to see this finally get included in core Evergreen! Thanks to all.

[Julian Clementson (oajulianc)]

Jane, it looks as though your signoff branch is already rebased, so I don't need to do that.

I was going to add my signoff to your commit for using BasicAdminPageComponent instead of a custom component, but during testing I spotted a small issue. Previously my custom component set default values on a few of the fields. These defaults are mentioned in the documentation, and some of them are recommended not to be changed unless you are certain what you're doing. So I would like to keep them.

Rather than re-introducing a custom component with all the resultant code duplication, I have proposed bug 1989348 as a proposal to be able to set defaultNewRecord via routing data.

I have an implementation of that proposal, with unit tests, which I will commit for comment shortly.

[Julian Clementson (oajulianc)]

I've pushed my proposed implementation for bug 1989348 and tagged with pullrequest.

If that is accepted, then we can provide the default values for OpenAthens by adding defaultNewRecord to the routing data as follows:

{
    path: 'config/openathens_identity',
    component: BasicAdminPageComponent,
    data: [{
        schema: 'config',
        table: 'openathens_identity',
        fieldOrder: 'id,org_unit,active,api_key,...',
        defaultNewRecord: {
            active: true,
            auto_signon_enabled: true,
            unique_identifier: 1,
            display_name: 1
        }
    }]
}

[Jane Sandberg (sandbergja)]

Thanks, Julian. I've rebased my signoff branch to get your defaultNewRecord work, and added the configuration you provided in comment #27. I've confirmed that the active and auto sign-out checkboxes are now checked by default in the fm editor, and that the unique identifier and display name fields both default to "id".

I'll let this marinate for another week, and plan on pushing this on 9/22 -- unless somebody beats me to it. :-D

[Jane Sandberg (sandbergja)]

Merged for inclusion in 3.10. Thanks for your work on this, Julian, and to the whole PINES team for implementing the feature and sharing your experience.

[Julian Clementson (oajulianc)]

Jane, I think I missed a step when I added the documentation for this feature, now that I can see what's been published with 3.10.

I added an adoc file under docs/RELEASE_NOTES_NEXT/Administration, which now appears at https://evergreen-ils.org/documentation/release/RELEASE_NOTES_3_10.html - that's ok.

I should have also added the same file under docs/modules/local_administration/pages so that it also appears under https://docs.evergreen-ils.org/eg/docs/latest/local_admin/introduction.html

What's the best way of fixing this up now, after the release?

[Jason Stephenson (jstephenson)]

Julian, the best way to handle that is to open a new bug with the documentation tag.