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

Issue #1963: add Shibboleth authentication capability #2661

Merged
merged 36 commits into from
Oct 20, 2017

Conversation

crism
Copy link
Contributor

@crism crism commented Jul 19, 2017

Fixes #1963.
Tested with multiple users with existing accounts and new ones, from multiple entry and exit points.


// We rely on these headers being present.
if (!isset($_SERVER[$uinHeader])) {
syslog(
Copy link
Member

Choose a reason for hiding this comment

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

@crism, we've generally used error_log rather than syslog in the past -- is there a reason for introducing a different logging pattern here? I'd worry that admins won't be used to looking there for OJS-related stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The previous Shibboleth plugin used syslog() and I tried to reuse as much of that code as possible. I’d rather use error_log().

*
* @return ShibbolethAuthPlugin
*/
function &_getPlugin() {
Copy link
Member

Choose a reason for hiding this comment

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

References on objects aren't needed with PHP5+.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More cut-and-paste from the predecessor (or maybe some other plugin)—that can go.

$userId = $user->getId();
$adminFound = array_search($uin, $admins);

$userGroupDao =& DAORegistry::getDAO('UserGroupDAO');
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary reference use (probably also elsewhere) -- there are lots of these in the codebase, but we're trying to clean them up as we go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More cut-and-paste.

@asmecher
Copy link
Member

Looks good, @crism, and sorry I missed this earlier. Just a few questions/tweaks, but this could also use a fresh push to trigger Travis testing -- a lot of master branch stuff has changed since July.

@vieville
Copy link

vieville commented Oct 4, 2017

@crism, I tried to load this pull with the command :
git fetch origin pull/#2661/head:shib
which returns : fatal: Couldn't find remote ref pull/#2661/head
It's certainly not the rigth command !

Please, can you send me the rigth command ?

Thank you

Claude

@crism
Copy link
Contributor Author

crism commented Oct 12, 2017

@asmecher, I think this addresses your comments. Sorry for the delay.

@crism
Copy link
Contributor Author

crism commented Oct 12, 2017

@vieville, you can’t pull a pull request. Add my fork as a remote and check out the issue-1963 branch.

@asmecher asmecher merged commit c587960 into pkp:master Oct 20, 2017
@asmecher
Copy link
Member

Excellent, @crism, and just in time for OJS 3.1 -- many thanks!

@asmecher
Copy link
Member

...except I moved it out of the pkp-lib repo into the ojs repo!

@kasioumis
Copy link

This is excellent news, @crism @asmecher thank you very much!

@crism
Copy link
Contributor Author

crism commented Oct 20, 2017

@asmecher it should work with O*S… why move it?

@asmecher
Copy link
Member

Ooh, neat! I spotted a reference to journal managers in ShibbolethSettingsForm and assumed -- but I'm more than happy to hear it's agnostic.

The best thing to do with this will be to move it to its own repository, then make it available via the plugin gallery. I'll do that and drop you a link.

@asmecher
Copy link
Member

It lives! https://github.com/pkp/shibboleth

@crism
Copy link
Contributor Author

crism commented Oct 20, 2017

@asmecher, yes, journal managers are mentioned in a comment; I tried to genericize everything, but missed that one. Thanks for setting up that repo!

@asmecher
Copy link
Member

@crism, I've added you as an admin to that repo. The next things that'll need doing are tagging and building a .tar.gz release, and adding it to the plugin gallery -- I'll do that after the OJS 3.1 release comes out, and use it as an opportunity to document that process in a blog post.

@crism crism deleted the issue-1963 branch October 20, 2017 15:42
@kasioumis
Copy link

Hi @asmecher and @crism, thanks again for this plugin :-) As far as I understand the plan is to host it in the generic plugins and not that auth ones, right? I'm looking forward to the official .tar.gz release and respective blog post about the plugin gallery. In the meantime, what is the best way to install shibboleth? Simply copy the folder structure into plugins/generic or manually package the folder structure into a single .tar.gz and then install that via the web form?

@asmecher
Copy link
Member

asmecher commented Nov 7, 2017

@kasioumis, I've just added the plugin to the Plugin Gallery; please let me know whether that works for you.

@kasioumis
Copy link

kasioumis commented Nov 7, 2017

@asmecher Thanks!

  • On vanilla OJS 3.1.0 I installed the shibboleth plugin via the Plugin Gallery. When trying to enabled it in the list of Installed Plugins I get the The plugin "Shibboleth Authentication Plugin" has been enabled. notification but the checkbox is not checked. The console reports the following:
Uncaught Error: Row with id googlescholarplugin not found!
    at a.pkp.controllers.grid.GridHandler.resequenceRows (pkp.min.js:273)
    at a.pkp.controllers.grid.GridHandler.replaceElementResponseHandler (pkp.min.js:281)
    at Object.success (pkp.min.js:124)
    at j (jquery.min.js:2)
    at Object.fireWith [as resolveWith] (jquery.min.js:2)
    at x (jquery.min.js:4)
    at XMLHttpRequest.b (jquery.min.js:4)

Note that the Settings Google Scholar Indexing Plugin happens to be the first plugin in the list of Generic Plugins.

  • On our test OJS 3.0.2 I installed the shibboleth plugin gia .tar.gz upload. I was able to enable it, configure it and use it successfully!

I'll be happy to provide more feedback and/or move this query to the forum.

@asmecher
Copy link
Member

asmecher commented Nov 9, 2017

Hi @kasioumis,

Could you check if this will resolve the issue on your OJS 3.1.0 installation?

If you can confirm that this works, I'll update the version in the plugin gallery.

@kasioumis
Copy link

Hi @asmecher,

I confirm that the updated shibboleth release (ojs-3_1_0-0-1) can be installed and set up without any problems on OJS 3.1.0.

Thanks!

@asmecher
Copy link
Member

Great, thanks! I've already put the updated version into the plugin galley.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants