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.

Sign up for GitHub

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

Jump to bottom

Shibboleth dynamic login handler #1607

Merged
merged 6 commits into from Sep 11, 2020
Merged

Shibboleth dynamic login handler #1607

merged 6 commits into from Sep 11, 2020

Conversation

jchssystems
Copy link

@jchssystems jchssystems commented May 27, 2020 •
edited

This feature allows to add Shibboleth users in a dynamic way from configuration.

  • Configuration is located in mh-default-org.xml.
  • Shibboleth attributes are mapped using SpEL.
  • It uses a new class DynamicLoginHander instead of ConfigurableLoginHandler.

@jchssystems jchssystems force-pushed the Shibboleth_Dynamic_login_handler branch 3 times, most recently from 823b691 to c66e404 Compare June 3, 2020 10:47
lkiesow
lkiesow requested changes Jun 6, 2020
View reviewed changes
Copy link
Member

@lkiesow lkiesow left a comment

Choose a reason for hiding this comment

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

I haven't actually compiled or even tested this but just skimmed through the code. I didn't find any critical issues, just a bunch of minor comments so far…

modules/security-aai/src/main/java/org/opencastproject/security/aai/api/AttributeMapper.java Outdated Show resolved Hide resolved
modules/security-aai/src/main/java/org/opencastproject/security/aai/api/AttributeMapper.java Outdated Show resolved Hide resolved
modules/userdirectory/src/main/java/org/opencastproject/userdirectory/JpaGroupRoleProvider.java Outdated Show resolved Hide resolved
modules/userdirectory/src/main/java/org/opencastproject/userdirectory/JpaGroupRoleProvider.java Outdated Show resolved Hide resolved
.../userdirectory/src/main/java/org/opencastproject/userdirectory/JpaUserReferenceProvider.java Outdated Show resolved Hide resolved
modules/security-aai/src/main/java/org/opencastproject/security/aai/DynamicLoginHandler.java Outdated Show resolved Hide resolved
modules/security-aai/src/main/java/org/opencastproject/security/aai/DynamicLoginHandler.java Outdated Show resolved Hide resolved
modules/security-aai/src/main/java/org/opencastproject/security/aai/DynamicLoginHandler.java Outdated Show resolved Hide resolved
modules/security-aai/src/main/java/org/opencastproject/security/aai/DynamicLoginHandler.java Outdated Show resolved Hide resolved
modules/security-aai/src/main/java/org/opencastproject/security/aai/DynamicLoginHandler.java Outdated Show resolved Hide resolved
hsssystems
hsssystems reviewed Jun 11, 2020
View reviewed changes
modules/userdirectory/src/main/java/org/opencastproject/userdirectory/JpaGroupRoleProvider.java Outdated
@@ -81,7 +82,7 @@
immediate = true,
service = { RoleProvider.class, JpaGroupRoleProvider.class }
)
public class JpaGroupRoleProvider extends AbstractIndexProducer implements RoleProvider, GroupProvider {
public class JpaGroupRoleProvider extends AbstractIndexProducer implements RoleProvider, GroupProvider, GroupRoleProvider {
Copy link

Choose a reason for hiding this comment

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

Isn't it sufficient to implement only GroupRoleProvider?

@jchssystems jchssystems force-pushed the Shibboleth_Dynamic_login_handler branch 2 times, most recently from 8eafa18 to d50d7b2 Compare June 23, 2020 10:51
@jchssystems
Copy link
Author

jchssystems commented Jun 23, 2020 •
edited

Please, review tthe code again. Some changes were made.
I'm still not sure if both logins should exist or if the new one will replace the old one.
And about the documentation, should I have a totally independent page for the new login handler? when the code is ok, fix the documentation.

@gregorydlogan
Copy link
Member

@staubesv any chance you can take another pass through this? I'm unfamiliar with any other major adopter using Shibboleth.

@hsssystems
Copy link

@staubesv any chance you can take another pass through this? I'm unfamiliar with any other major adopter using Shibboleth.

Yesterday we made another integration test on a OC9 test installation with this branch and it worked. However, we found some issues:

  • we need to adapt the the documentation (example configuration) and the unit test
  • Updating the displayName or group memberships (only removing users from groups) needs several logins. Do we need some index / cache invalidation?
  • The GroupRoleProvider interface needs refactoring

@otti-ssystems
Copy link
Contributor

Hi.

I adjusted the structure of the interfaces.
DynamicLoginHandler is not extending GroupRoleProvider anymore.
Instead we are using now AAIRoleProvider which contains getRoles, so
i haven't had to touch RoleProvider.

I also ajusted the Tests, because the mapping of givenName and sn is not
needed in this context.

Travis is not able to build because of a weird Exception.

https://travis-ci.com/github/opencast/opencast/jobs/366125028

jchernandezr and others added 5 commits August 3, 2020 14:22
@jchernandezr
refs #502450 ADD Shibboleth Dynamic Loginhandler
e7d19b6 
  refs #502450 ADD Shibboleth Dynamic Loginhandler

  refs #501517 FIX Update group memberships in Shibboleth LoginHandler

refs #501517 ADD Dynamic login handler based on spring expressions

  refs #501517 ADD Tests for dynamic login handler based on spring expressions

  refs #501517 FIX Interfaces and maven-dependency-plugin

refs opencast#4870 FIX map email and name

refs opencast#4943 FIX Fallback to user create when there is no user reference for existing users

  - FIXME Check for existence of non-ref user first

  FIX securitty.aai Checkstyle

  FIX userdirectory Checkstyle

  FIX all

  refs #502450 FIX Shibboleth Dynamic Loginhandler

refs opencast#2965 FIX Example configuration for Dynamic loginhandler

refs opencast#2965 FIX Use spring util 3.1 xsd - not tested

  - FIXME We may need to update pom.xml

refs opencast#2965 FIX Mockdata
@jchernandezr
refs opencast#5049 FIX Do not use sn or givenName since we have only …
e7ae266 
…displayName in OC
@otti-ssystems @jchernandezr
refs opencast#2965 REFACTOR structure of interfaces
423fa4a
@otti-ssystems @jchernandezr
refs opencast#2965 REFACTOR deleting unnecessary variables
774194d
@otti-ssystems @jchernandezr
refs opencast#2965 REFACTOR Interfaces and config for the Dynamic Log…
c37b22a 
…in Handler
@jchssystems jchssystems force-pushed the Shibboleth_Dynamic_login_handler branch from 0203bc5 to c37b22a Compare August 3, 2020 13:41
@jchssystems
Copy link
Author

@lkiesow @gregorydlogan
I have fixed the problem that was there some time ago and it compiles well. The code was also refactored. Could you please check it again? So we can close the issue as soon as possible.
Thanks in advance.

@otti-ssystems
Copy link
Contributor

Hi Guys.
Sorry but i dont get it.

Is there still something to be done or is this ready to merge?

@gregorydlogan
Copy link
Member

Hi @jchssystems @otti-ssystems we don't get notifications when things change, so unless someone manually checks we don't know that you'd pushed changes. If you don't see something happen within a week or so please ping the people who have been active in the ticket, or pipe up on the dev list.

I have no way to test this unfortunately, so I"m somewhat hamstrung in terms of getting it merged. I'm assuming you're writing this for a client, can they verify that it's working in prod?

@hsssystems
Copy link

Hi Greg,

the handler is in production on several clients! However, we will deploy again an Opencast 9 with the handler and test it another time systematically (we did already twice). Is that OK for you?

Our QA guys will test it, not the programmers, so it should be safe :-)

@gregorydlogan
Copy link
Member

Ok, sounds fair. Let's merge this then.

@gregorydlogan gregorydlogan merged commit ebba73b into opencast:develop Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JulianKniephoff JulianKniephoff JulianKniephoff left review comments

@staubesv staubesv staubesv left review comments

@hsssystems hsssystems hsssystems left review comments

@lkiesow lkiesow lkiesow requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jchssystems @gregorydlogan @hsssystems @otti-ssystems @JulianKniephoff @lkiesow @staubesv @jchernandezr