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

MH-13209, Put CAS Feature In Distributions #544

Merged
merged 1 commit into from Dec 3, 2018

Conversation

lkiesow
Copy link
Member

@lkiesow lkiesow commented Nov 4, 2018

This patch ensures that Opencast's CAS security feature is contained in
all distributions and can easily be installed/enabled without enabling
it by default or making it a core dependency.

This is an alternative solution to pull request #543

This patch ensures that Opencast's CAS security feature is contained in
all distributions and can easily be installed/enabled without enabling
it by default or making it a core dependency.
@miesgre
Copy link
Contributor

miesgre commented Nov 5, 2018

It works @lkiesow. Now the opencast-secitity-cas libraries are in the correct place (system/org/opencastproject/) and when you add the feature to bootFeatures, it os loaded correctly:
karaf@root> bundle:list | grep security
146 | Resolved | 82 | 6.0.0.SNAPSHOT | Opencast :: security-aai, Hosts: 111
147 | Resolved | 82 | 6.0.0.SNAPSHOT | Opencast :: security-cas, Hosts: 111
148 | Active | 82 | 6.0.0.SNAPSHOT | Opencast :: security-cas-client-wrapper
149 | Resolved | 82 | 6.0.0.SNAPSHOT | Opencast :: security-shibboleth, Hosts: 111

But, why other authentication methods like ldap or shibboleth are in the core and loaded by default and CAS not? Should we create feature for each authentication method?

@lkiesow
Copy link
Member Author

lkiesow commented Nov 5, 2018

Good point. I'll bring it up at tomorrows technical meeting.

I think this is the better solution since it's easy to activate and has less code running by default. It would also be a good idea to do something like this for e.g. LDAP as well. But let's see what others think.

@miesgre
Copy link
Contributor

miesgre commented Nov 5, 2018

Yes, I think so.
Another think is that the kernel module has a LDAP dependencies (https://github.com/opencast/opencast/blob/develop/modules/kernel/pom.xml#L267-L270). I think LDAP dependencies should be removed from kernel bundle and added it to the security-ldap bundle like CAS or shibboleth. :)

@miesgre
Copy link
Contributor

miesgre commented Dec 3, 2018

@lkiesow Will this be merged before 6.0? Is there anything I can do to help?
I tested it, and it works for me.

@staubesv staubesv self-requested a review December 3, 2018 13:05
@staubesv staubesv self-assigned this Dec 3, 2018
@staubesv
Copy link
Contributor

staubesv commented Dec 3, 2018

I don't have CAS in place for testing but as @miesgre tested this and it looks fine for me, I'll accept the PR.

@staubesv staubesv merged commit b352530 into opencast:r/6.x Dec 3, 2018
lkiesow added a commit to lkiesow/opencast that referenced this pull request Mar 29, 2019
This is an alternative to pull request opencast#795.

The discussion on pull request opencast#795 is about wanting to put all optional
features/modules into the Opencast core. Pull request opencast#543 was declined
for exactly the reason that we wouldn't want to do that. If we decide to
do this way after all, we should also accept the old pull request.

This patch reverts pull request opencast#544 and applies the previously declined
pull request opencast#543 instead. The pull request adds all of the CAS modules
to Opencast's core feature, always starting those modules with Opencast,
regardless of users needing the feature or not.

This change has the advantage of making the feature slightly easier to
configure as well as better tested since problems with those features
will affect all installations.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants