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

Authentification openldap et AD #74

Merged
merged 11 commits into from
Jan 26, 2019
Merged

Authentification openldap et AD #74

merged 11 commits into from
Jan 26, 2019

Conversation

wouldsmina
Copy link
Member

un patch express pour permettre l'authentification avec un active directory en plus d'openldap. Je ne l'ai pas testé pour le moment, je le ferai dés que possible.

@prytoegrian
Copy link
Member

⚠️ le test est en échec

@prytoegrian
Copy link
Member

J'ai vu TXR quelques heures. Il me rapporte que ça ne change rien :-/

@wouldsmina
Copy link
Member Author

Je vais tester de mon côté mercredi et ferai les corrections nécessaires.

@prytoegrian
Copy link
Member

J'avoue, c'est velu. La solution est là, utilise-le pour comprendre à ton rythme :

───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
modified: Tests/Units/Tools/Services/LdapAuthentifierService.php
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
@ LdapAuthentifierService.php:21 @ class LdapAuthentifierService extends \Atoum
    {
        parent::beforeTestMethod($method);

-       $this->connection = new \mock\Adldap\Connections\Ldap();
+       $this->mockGenerator->orphanize('__construct');
+       $this->search = new \mock\Adldap\Query\Factory();
        $this->provider = new \mock\Adldap\Connections\Provider();
        $this->ldap = new \mock\Adldap\Adldap();
        $this->calling($this->ldap)->addProvider = '';
        $this->calling($this->ldap)->connect = $this->provider;
-       $this->calling($this->provider)->getConnection = $this->connection;
+       $this->calling($this->search)->select = null;
+       $this->calling($this->search)->users = $this->users();
+       $this->calling($this->provider)->search = $this->search;
+       $this->mockGenerator->orphanize('__construct');
+       $this->guard = new \mock\Adldap\Auth\Guard();
+       $this->calling($this->provider)->auth = $this->guard;
 
        $this->mockGenerator->orphanize('__construct');
        $this->request = new \mock\Slim\Http\Request();
        $configuration = json_decode(json_encode($this->configuration));
@ LdapAuthentifierService.php:43 @ class LdapAuthentifierService extends \Atoum

    public function testIsAuthentificationSucceedFalse()
    {
-       $this->calling($this->provider->auth())->attempt = false;
+       $this->calling($this->guard)->attempt = false;
        $this->newTestedInstance($this->ldap);
        $succeed = $this->testedInstance->isAuthentificationSucceed($this->request);

@ LdapAuthentifierService.php:52 @ class LdapAuthentifierService extends \Atoum

    public function testIsAuthentificationSucceedTrue()
    {
-       $this->calling($this->provider->auth())->attempt = true;
+       $this->calling($this->guard)->attempt = true;
        $this->newTestedInstance($this->ldap);
        $succeed = $this->testedInstance->isAuthentificationSucceed($this->request);

        $this->boolean($succeed)->isTrue();
    }

+   private function users()
+   {
+       return new class($this) {
+           private $outer;
+           public function __construct($outer) {$this->outer = $outer;}
+            public function where() {
+               return $this->outer->first();
+           }
+       };
+   }
 
+   public function first()
+   {
+       return new class($this) {
+           private $outer;
+           public function __construct($outer) {$this->outer = $outer;}
+           public function firstOrFail() {
+               return $this->outer->user();
+           }
+       };
+   }
 
+   public function user()
+   {
+       return new class {
+           public function getDn() {}
+           public function getPassword() {}
+       };
+   }
 
    /**
     * @var \Adldap\AdldapInterface Mock du service LDAP
     */
@ LdapAuthentifierService.php:100 @ class LdapAuthentifierService extends \Atoum
    private $provider;

    /**
-    * @var \Adldap\Connections\ConnectionInterface
+    * @var \Adldap\Query\Factory
+    */
+   private $search;
 
+   /**
+    * @var \Adldap\Auth\Guard
     */
-   private $connection;
+   private $guard;

    /**
     * @var array

@wouldsmina
Copy link
Member Author

ah ouais! velu c'est peu dire 😆

@libertempo libertempo deleted a comment Jan 4, 2019
@libertempo libertempo deleted a comment Jan 4, 2019
@libertempo libertempo deleted a comment Jan 4, 2019
@prytoegrian
Copy link
Member

prytoegrian commented Jan 8, 2019

Avec un bon mdp comme un mauvais, j'obtiens :

{
  "code": 500,
  "status": "error",
  "message": "Internal Server Error",
  "data": "No LDAP query results for filter: [(&(objectclass=user)(objectcategory=person)(!(objectclass=contact))(uid=hr))] in: [dc=libertempo]"
}

Est-ce ma config qui est mauvaise ?

Édit : ça peut être en relation avec #73.

@wouldsmina
Copy link
Member Author

Il est très bizarre ce filtre! D'ailleurs, je n'ai pas utilisé le filtre dans la requête!

@prytoegrian
Copy link
Member

#73 corrige ce bug Adldap2/Adldap2#612, c'est le seul lien que je vois. Je ne m'explique pas la régression par contre

@wouldsmina
Copy link
Member Author

D'ailleurs, je n'ai pas utilisé le filtre dans la requête!

Finalement, il n'est pas nécessaire de filtrer puisque notre recherche se limite à un uid.

@prytoegrian
Copy link
Member

Toujours la même erreur (https://rollbar.com/Libertempo/api/items/40/occurrences/63913474694/).
Est-ce-que c'est mon annuaire qui est mal formé ?

(je pense qu'il manque une capture d'exception si firstOrFail échoue).

La raison est ce bout de code : https://github.com/Adldap2/Adldap2/blob/master/src/Query/Factory.php#L125L128
Si fais :

@ LdapAuthentifierService.php:49 @ class LdapAuthentifierService extends AAuthentifierFactoryService
        $provider = $this->ldap->connect();
        $search = $provider->search();
        $search->select(['dn']);
-       $user = $search->users()->where($configurationLdap->login, $this->getLogin())->firstOrFail();  
+       $user = $search->where($configurationLdap->login, $this->getLogin())->firstOrFail();

chez moi ça fonctionne

@wouldsmina
Copy link
Member Author

décidément, cette lib nous donne bien du fil à retordre! Je test ça et fait un commit si c'est ok aussi de mon coté.

@wouldsmina
Copy link
Member Author

Ok @prytoegrian sans user() pour OpenLDAP. Je test AD tout de suite!

@wouldsmina
Copy link
Member Author

AD ok!!! je demande à TxR de tester sur son AD.

@libertempo libertempo deleted a comment Jan 18, 2019
@wouldsmina
Copy link
Member Author

Method where() is not under control 😭

@wouldsmina
Copy link
Member Author

testé par TxR, tout est ok... sauf le test unitaire! Je vais essayer de comprendre et corriger. Après cela, il faudra récupérer uniquement ce patch sur web avant de passer Akhaten en stable.

@libertempo libertempo deleted a comment Jan 23, 2019
@libertempo libertempo deleted a comment Jan 23, 2019
@libertempo libertempo deleted a comment Jan 23, 2019
@libertempo libertempo deleted a comment Jan 23, 2019
@libertempo libertempo deleted a comment Jan 23, 2019
@prytoegrian
Copy link
Member

───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
modified: Tests/Units/Tools/Services/LdapAuthentifierService.php
───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
@ LdapAuthentifierService.php:28 @ class LdapAuthentifierService extends \Atoum
        $this->calling($this->ldap)->addProvider = '';
        $this->calling($this->ldap)->connect = $this->provider;
        $this->calling($this->search)->select = null;
+       $this->calling($this->search)->where = $this->where();
        $this->calling($this->provider)->search = $this->search;
        $this->mockGenerator->orphanize('__construct');
        $this->guard = new \mock\Adldap\Auth\Guard();
@ LdapAuthentifierService.php:58 @ class LdapAuthentifierService extends \Atoum
        $this->boolean($succeed)->isTrue();
    }

-   public function first()
+   public function where()
    {
        return new class($this) {
            private $outer;

😗

Ensuite, une fois que tu as mergé le squash sur master, il te faut créer une nouvelle release :

make minor

et la consommer dans web; d'après ce que j'ai vu, il n'aura pas d'incompatibilité.

@wouldsmina
Copy link
Member Author

j'étais pas loin d'y arriver! il me manquait public function where() je ne savais pas trop quoi mettre dedans!
gg bro 😄

et la consommer dans web; d'après ce que j'ai vu, il n'aura pas d'incompatibilité.

l'API, dans ça dernière version, nécessite php7.1 minimum et je préfère attendre la sorti de DEB10 qui inclut PHP7.3 avant de pousser cette version vers web.
Je pensais plutôt faire un cherry-pick (ou quelques choses dans ce style) pour ne prendre que les modifications liées au ldap (je me rend compte de la complexité de cette solution en l'écrivant :-/ ).

Je vais faire quelque test avant de me décider ;)
merci pour le coup de main

@wouldsmina wouldsmina merged commit 05bd817 into master Jan 26, 2019
@wouldsmina wouldsmina deleted the ws/patchLdap branch January 26, 2019 22:56
@wouldsmina
Copy link
Member Author

wouldsmina commented Jan 26, 2019

make minor

J'y arrive po :(

make: *** [Makefile:26: minor] Error 1

PS : j'ai installé node-semver

@prytoegrian
Copy link
Member

l'API, dans ça dernière version, nécessite php7.1 minimum et je préfère attendre la sorti de DEB10 qui inclut PHP7.3 avant de pousser cette version vers web.
Je pensais plutôt faire un cherry-pick (ou quelques choses dans ce style) pour ne prendre que les modifications liées au ldap (je me rend compte de la complexité de cette solution en l'écrivant :-/ ).

Le cherry-pick ne sera pas possible car tu es trans-repo. Tu aurais pu par contre faire une application manuelle des changements, un peu à la git apply-patch, mais ce n'est pas possible non plus depuis que les vendor ne sont plus traqués par git (et je déconseille). Ce que je peux proposer, c'est que soit nous mergeons normalement et ceux qui n'ont pas les versions requises des logiciels ne montent pas en version, soit nous laissons LT en beta tant que DEB10 n'est pas sorti.

@prytoegrian
Copy link
Member

@wouldsmina
Copy link
Member Author

Le ldap est une fonctionnalité importante, je tiens à ce qu'elle soit rétabli rapidement et sans migrer vers php7.1. Je suis tenté de faire une PR sur beta, ça pose un problème techniquement?

@prytoegrian
Copy link
Member

Je ne sais pas quel changement tu voudrais faire sur beta à vrai dire.

@wouldsmina
Copy link
Member Author

en gros écraser Tools/Services/LdapAuthentifierService.php et adldap2 avec la dernière version.

@prytoegrian
Copy link
Member

Entendu. Mais ça, ça n'est pas possible car le faire dans web implique de modifier vendor/libertempo/api/... ; puisque ce répertoire n'est pas traqué pour ne pas faire de mauvaises suppositions sur le statut de l'infra qui installe, tout ce que tu feras ne sera fait que chez toi. Au vu de ce que tu demandes, nous ne pouvons pas proposer de release actuellement, c'est pour ça que je proposais d'attendre la sortie. J'ai pas mieux :-/

@wouldsmina
Copy link
Member Author

wouldsmina commented Feb 1, 2019

puisque ce répertoire n'est pas traqué pour ne pas faire de mauvaises suppositions sur le statut de l'infra qui installe, tout ce que tu feras ne sera fait que chez toi

Je suis pas sur d'avoir compris... si c'est au sujet du contenu de vendor, dans beta il y a encore l'api et tout le reste. Si c'est autre chose, je te fais confiance, on attend deb10.

@prytoegrian
Copy link
Member

En effet, tu as raison j'avais oublié ce point. Dans ce cas, on peut profiter de ça pour mettre un bout de code histoire d'avancer sur le sujet et (si c'est le dernier sujet important avant livraison), déployer en master. C'est tricher mais bon hein...

@prytoegrian prytoegrian mentioned this pull request Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants