Project

General

Profile

Feature #4554

Add Shibboleth Authentication

Added by Laurent Opprecht over 7 years ago. Updated about 7 years ago.

Status:
Feature implemented
Priority:
Normal
Category:
-
Target version:
Start date:
26/03/2012
Due date:
% Done:

90%

Estimated time:
Complexity:
Normal
SCRUM pts - complexity:
?

Description

Add support for Shibboleth authentication.


Related issues

Related to Chamilo LMS - Feature #4782: Move login code from local to functionNew21/05/2012

Actions

Associated revisions

Revision c6fbaa3a (diff)
Added by Laurent Opprecht about 7 years ago

see #4554: move shibboleth classes to their namespace, remove redirect bug, remove conf

Revision 75833b02 (diff)
Added by Laurent Opprecht about 7 years ago

see #4554: move shibboleth classes to their namespace, remove redirect bug, remove conf

Revision cc2e7cba (diff)
Added by Laurent Opprecht about 7 years ago

see #4554: minor remove test

Revision 91a850bd (diff)
Added by Laurent Opprecht about 7 years ago

see #4554: shibboleth bug: enable admin login, remove duplicate creation, add staff test

Revision 04b14074 (diff)
Added by Laurent Opprecht about 7 years ago

see #4554 shibboleth small bug for registration screen

History

#2

Updated by Julio Montoya over 7 years ago

Hello Laurent, great work! I was looking to your commits and I recommend you to add only this at the start of the page:

/* For licensing terms, see /license.txt */

Then you can edit that file (license.txt) and add all the credits.
see the page: http://support.chamilo.org/projects/chamilo-18/wiki/Coding_conventions

#3

Updated by Yannick Warnier over 7 years ago

This is not a recommendation, it must be used everywhere (thanks for pointing that out). You can add one line of authorship in functions definitions for example, but the safest is to edit documentation/credits.html and put your name there.

#4

Updated by Laurent Opprecht over 7 years ago

Ah yes, this is my default configuration in NetBeans. Did the change for Shibb - will do the other plugins as well when time allows.

#5

Updated by Julio Montoya over 7 years ago

  • Status changed from New to Needs more info
  • Assignee set to Laurent Opprecht

Hello Laurent,

I see some of your commits and I have some comments:

1. You should not add backticks (`) in queries, that's why we have a Database class (main/inc/lib/database.lib.php)

example:

http://code.google.com/p/chamilo/source/browse/main/auth/shibboleth/app/lib/scaffolder/scaffolder.class.php?
spec=svn.classic.8b9a2cbd54562e3324a764127bbfc36847b92870&repo=classic&r=8b9a2cbd54562e3324a764127bbfc36847b92870#31

Please check the Coding conventions:

http://support.chamilo.org/projects/chamilo-18/wiki/Coding_conventions#Use-of-backticks

You can use something like:

$db_table = Database :: get_main_table(TABLE_MAIN_XXX);

and define the constant "TABLE_MAIN_XXX" in the main/inc/lib/database.constants.inc.php file

2. I found that you're creating controllers, models and views, etc or maybe there's already a shibboleth library that was build that way?

You are also creating new functions to save, edit, delete records (store.class.pgp) in the DB and you are not using the main/inc/lib/database.lib.php class. It will be better to improve the database.lib.php class instead of building a class from scratch.

the store.class.php :

http://code.google.com/p/chamilo/source/browse/main/auth/shibboleth/app/lib/store.class.php?repo=classic&r=8b9a2cbd54562e3324a764127bbfc36847b92870

3. I have the impression that you're building your own mini framework inside Chamilo for this module.
I think we should avoid this way of development until we find something that can be reused everywhere (example: symfony2 components doctrine+ twig) I already added the doctrine2 library just for tests but I didn't have the time to finish it (see the main/inc/db.lib.php) I don't want to discourage you but I don't want that you take your energy rebuilding the database layer of chamilo if we're going to replace it with some third party component later.

4. Remember that one problem that we have in Chamilo 1.9 since Claroline, is that all php files are exposed so any user can have access to some "private" php files like some scaffolders

http://chamilodev.beeznest.com/main/auth/shibboleth/app/lib/scaffolder/template/public.php

#6

Updated by Laurent Opprecht over 7 years ago

Cheers Julio,
Thanks for the comments.

Indeed the Shibboleth is a port of other implementations. It follows a common model view controller approach - more precisely the RoR/CakePhp way of doing things which is not much different than Symphony - where each plugin can follow the same structure as the main application and can introduce its own model view controllers structure.

I agree that many components would be useful elsewhere yet I refrained to do that. The reason being the impact would be too high for going live in a few months. Actually moving some parts to the main library - login for example - would only make sense if we reuse them elsewhere and rewrite several parts of the application - local.inc.php would be my favorite. Basically I choosed to minimize the impact on the global code for sake of stability. Now I certainly agree that once 1.9 is out we can/must start on moving things around.

For the models - user for example - I thing too it is too early to introduce them in the main branch. Expecially though because we want to move th Symphony. We should first stabilize the DB and use Symphony to generate them. Now the problem of course is that I need a model to interact with the DB since there is none, so I created what I need.

For the scaffolding this should not be a problem. The code is contained in classes - which means not executable - and cannot be called unless your are in test mode. Of course once we have Symph in place we can get rid of that.

I don't think either that moving to Symph later on would be much of an issue with the current code structure. Obviously I would like to do that sooner but I believe this would be unwise until we have 1.9 out.

Another thing

one page equal more or less one controller
one db table equals more or less a class - well two if you split rows and table

#7

Updated by Laurent Opprecht over 7 years ago

  • Assignee changed from Laurent Opprecht to Julio Montoya
#8

Updated by Julio Montoya over 7 years ago

  • Status changed from Needs more info to Assigned
  • Assignee changed from Julio Montoya to Laurent Opprecht

Hi Laurent,

I understand your position and I'm agree that the time factor is a constraint. I see that this is an entire library like an add-on for the Chamilo authentication and I understand that you did it that way because dealing with local.inc.php will be kind of a nightmare :)
So, keep the good work and don't forget to use the database.lib.php for those basic stuff.

#9

Updated by Laurent Opprecht about 7 years ago

  • Status changed from Assigned to Needs testing
  • % Done changed from 0 to 90

Yep will take Database into consideration. I believe once we are a bit settled we could talk the DB stuff and see how we want to move forward with it. Especially with Symphony in scope.

#10

Updated by Yannick Warnier about 7 years ago

Should we ask someone to try the Shibboleth authentication? Laurent, could you try it at work? I don't have any Shibboleth server to try it, so not sure how we can do this (normally somebody else than the opener/developer has to validate it). Does Grenoble have a Shibboleth server they can try it against?

#11

Updated by Hubert Borderiou about 7 years ago

Hi,
thanks for the code Laurent
I'll be able to test it in Grenoble with our Shibboleth server.
I'll tell you.
regards,

#12

Updated by Laurent Opprecht about 7 years ago

Thanks Hubert,
We tested here is Geneva and so far it works fine. But another round of tests in another environement would be very much useful.

#13

Updated by Yannick Warnier about 7 years ago

  • Target version set to 1.9 Beta
#14

Updated by Laurent Opprecht about 7 years ago

Todo test with empty shibboleth mapping to see if it works.

#15

Updated by Laurent Opprecht about 7 years ago

  • Assignee changed from Laurent Opprecht to Hubert Borderiou

Cheers Hubert,
I tried to replicate the issue we had last time - the one when we needed to provide all fields for Shibboleth to work.
I create a user with just the basic information: user id, last name, first name and that works.
So I was wondering if you have more information about this issue?

#16

Updated by Fernando Paredes Garcia about 7 years ago

As Laurent said, we need another test to confirm that the issue is resolved. Or at least have more details on the issue to make sure that the only test we had is a false positive.

#17

Updated by Yannick Warnier about 7 years ago

  • Target version changed from 1.9 Beta to 1.9 RC1
#18

Updated by Yannick Warnier about 7 years ago

  • Status changed from Needs testing to Feature implemented

Seems like we don't have another tester. Laurent and Hubert, I'm closing the task. I'm sure we'll get reports soon enough if it doesn't work...

Also available in: Atom PDF