Add_Directory_and_ProfilePicture #9

Merged
lx merged 13 commits from Add_Directory into main 2021-08-16 14:44:53 +00:00
Member

Add a new directory for no-Admin. In the directory, a searchbox is used.
The default visibility's value is private. If the attribute doesn't exist, the value is like private.

Todo:

  • Add a link in the index page to directory page
  • Choose the default visibility (public or private or anything else)
  • Add a checkbox in profile's page for activate or not the visibility
  • Allow to users to create a short description (180 characters)
  • Allow to store a profile picture on garage and store the link in Consul
Add a new directory for no-Admin. In the directory, a searchbox is used. The default visibility's value is private. If the attribute doesn't exist, the value is like private. ### Todo: - ~~Add a link in the index page to directory page~~ - ~~Choose the default visibility (public or private or anything else)~~ - Add a checkbox in profile's page for activate or not the visibility - Allow to users to create a short description (180 characters) - Allow to store a profile picture on garage and store the link in Consul
erwan force-pushed Add_Directory from adc005c888 to 768df58da6 2021-07-21 20:21:21 +00:00 Compare
erwan changed title from WIP: Add_Directory to Add_Directory 2021-07-21 20:21:57 +00:00
erwan requested review from lx 2021-07-21 20:22:22 +00:00
erwan force-pushed Add_Directory from b6202be9b9 to 46141a418d 2021-07-21 20:32:05 +00:00 Compare
erwan force-pushed Add_Directory from aaf00147b8 to 4891eb1aa9 2021-07-26 15:21:08 +00:00 Compare
erwan force-pushed Add_Directory from 07d14cd3ab to 3701abae46 2021-07-27 12:21:48 +00:00 Compare
erwan force-pushed Add_Directory from 8188f35b9a to 3abb38a746 2021-07-29 09:14:50 +00:00 Compare
lx requested changes 2021-07-30 09:14:33 +00:00
lx left a comment
Owner

J'ai fait quelques commentaires sur le code si tu veux bien regarder :)

Aussi, il faudrait appliquer go fmt pour nettoyer le formattage du code

J'ai fait quelques commentaires sur le code si tu veux bien regarder :) Aussi, il faudrait appliquer `go fmt` pour nettoyer le formattage du code
directory.go Outdated
@ -0,0 +22,4 @@
}
type SearchResult struct {
Identifiant string `json:"identifiant"`
Owner

L'identifiant, c'est le CN ? Dans tous les cas Identifiant c'est un mot français, il faudrait appeller ça plutôt juste Id.

L'identifiant, c'est le CN ? Dans tous les cas `Identifiant` c'est un mot français, il faudrait appeller ça plutôt juste `Id`.
erwan marked this conversation as resolved
directory.go Outdated
@ -0,0 +23,4 @@
type SearchResult struct {
Identifiant string `json:"identifiant"`
Name string `json:"name"`
Owner

C'est le displayname que tu prends depuis le LDAP? Dans ce cas il faudrait appeller ça DisplayName

C'est le displayname que tu prends depuis le LDAP? Dans ce cas il faudrait appeller ça `DisplayName`
erwan marked this conversation as resolved
directory.go Outdated
@ -0,0 +65,4 @@
var result Results
for _, values := range sr.Entries {
if strings.Contains(values.GetAttributeValue("cn"), input) {
Owner
  1. Ce n'est pas toujours cn l'attribut qui contient l'identifiant de la personne, c'est le paramètre config.UserNameAttr qui te donne le bon attribut à checker

  2. On aimerait aussi chercher dans le displayname, et peut-être aussi le mail

1. Ce n'est pas toujours `cn` l'attribut qui contient l'identifiant de la personne, c'est le paramètre `config.UserNameAttr` qui te donne le bon attribut à checker 2. On aimerait aussi chercher dans le displayname, et peut-être aussi le mail
erwan marked this conversation as resolved
directory.go Outdated
@ -0,0 +68,4 @@
if strings.Contains(values.GetAttributeValue("cn"), input) {
result = Results{
Search: append(result.Search, SearchResult{
Identifiant: values.GetAttributeValue("cn"),
Owner

Idem, c'est pas cn chez tout le monde

Idem, c'est pas `cn` chez tout le monde
erwan marked this conversation as resolved
directory.go Outdated
@ -0,0 +86,4 @@
Email: "",
Description: "",
DN: "",
}),
Owner
  1. Pourquoi un résultat vide?

  2. Si tu veux créer un résultat vide en Go, pas la peine de mettre tous les champs à "", si tu écris juste SearchResult{} ça crée une structure avec des chaines vides dans tous les champs

1. Pourquoi un résultat vide? 2. Si tu veux créer un résultat vide en Go, pas la peine de mettre tous les champs à `""`, si tu écris juste `SearchResult{}` ça crée une structure avec des chaines vides dans tous les champs
Author
Member

Un résultat vide permet d'avoir un readyStateChange dans le JS ce qui permet d'initialiser le tableau à vide quand il n'y a plus de match.

Pour le go fmt, j'utilise VsCode et à chaque sauvegarde il fait le go fmt et aussi de tous les warnings.

Un résultat vide permet d'avoir un readyStateChange dans le JS ce qui permet d'initialiser le tableau à vide quand il n'y a plus de match. Pour le `go fmt`, j'utilise VsCode et à chaque sauvegarde il fait le `go fmt` et aussi de tous les warnings.
Owner

Mais si tu met pas le SearchResult vide, tu as quand même le readyStateChange non ? Et tu as un tableau JSON vide, c'est tout, mais ta partie JS est sensé gérer ça

Mais si tu met pas le `SearchResult` vide, tu as quand même le readyStateChange non ? Et tu as un tableau JSON vide, c'est tout, mais ta partie JS est sensé gérer ça
Author
Member

Justement au début j'avais fait ça. Ce qui me donnait une valeur null. Mais la fonction JS suivante ne répondez pas, elle ignorait juste la réponse. (même en enlevant le if)

 xhttp.onreadystatechange = function() {
        if (this.readyState == 4 && this.status == 201) {

Elle n'était tout bonnement pas appelée.

Justement au début j'avais fait ça. Ce qui me donnait une valeur `null`. Mais la fonction JS suivante ne répondez pas, elle ignorait juste la réponse. (même en enlevant le if) ```js xhttp.onreadystatechange = function() { if (this.readyState == 4 && this.status == 201) { ``` Elle n'était tout bonnement pas appelée.
main.go Outdated
@ -44,2 +44,4 @@
GroupCanInvite string `json:"group_can_invite"`
GroupCanAdmin string `json:"group_can_admin"`
Endpoint string `json:"endpoint"`
Owner

Endpoint de quoi? Il faudrait appeller ça S3Endpoint pour être clair, et les autres les appeller S3AccessKey et S3SecretKey.

Endpoint de quoi? Il faudrait appeller ça `S3Endpoint` pour être clair, et les autres les appeller `S3AccessKey` et `S3SecretKey`.
erwan marked this conversation as resolved
main.go Outdated
@ -242,3 +251,3 @@
ldap.ScopeBaseObject, ldap.NeverDerefAliases, 0, 0, false,
requestKind,
[]string{"dn", "displayname", "givenname", "sn", "mail", "memberof"},
[]string{"dn", "displayname", "givenname", "sn", "mail", "memberof", "visibility", "description", "profilImage"},
Owner

pas de français ! appller ça profilePicture et non profilImage

pas de français ! appller ça `profilePicture` et non `profilImage`
erwan marked this conversation as resolved
main.go Outdated
@ -390,2 +399,4 @@
session.Values["login_dn"] = user_dn
//Add Value MessageID
session.Values["MessageID"] = uint32(0)
Owner

je pense que tu peux enlever ça du coup

je pense que tu peux enlever ça du coup
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +1,244 @@
package main
Owner

il faudrait renommer ce fichier, moi je l'appellerais juste s3.go

il faudrait renommer ce fichier, moi je l'appellerais juste `s3.go`
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +25,4 @@
)
//Upload image through guichet server.
func uploadImage(w http.ResponseWriter, r *http.Request, login *LoginStatus) (bool, string, error) {
Owner

Pourquoi on renvoie un bool, un string et une error ? je pense qu'on peut enlever le bool, ça complexifie trop et on se perd, et utiliser la sémantique suivante pour (string, error):

  • ("", nil): aucun fichier n'a été envoyé par l'utilisateur, il ne se passe rien
  • ("", une erreur): une erreur se produit lors du traitement
  • ("machintruc.jpg", nil): on a traîté l'image correctement et elle existe sur s3

Il suffit ensuite de checker qu'il n'y a pas d'erreur, puis copier la valeur dans LDAP seulement si elle n'est pas ""

Pourquoi on renvoie un bool, un string et une error ? je pense qu'on peut enlever le bool, ça complexifie trop et on se perd, et utiliser la sémantique suivante pour `(string, error)`: - `("", nil)`: aucun fichier n'a été envoyé par l'utilisateur, il ne se passe rien - `("", une erreur)`: une erreur se produit lors du traitement - `("machintruc.jpg", nil)`: on a traîté l'image correctement et elle existe sur s3 Il suffit ensuite de checker qu'il n'y a pas d'erreur, puis copier la valeur dans LDAP seulement si elle n'est pas `""`
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +40,4 @@
return false, "", err
}
if fileType == "" {
return false, "", nil
Owner
  1. a-t-on vraiment besoin de ce check? car si on arrive ici on a err == nil, et le seul cas où c'est possible c'est quand le filetype est un filetype image/ correct
1. a-t-on vraiment besoin de ce check? car si on arrive ici on a `err == nil`, et le seul cas où c'est possible c'est quand le filetype est un filetype `image/` correct
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +50,4 @@
return false, "", err
}
mc, err := newMimioClient()
Owner

Minio, et pas Mimio ;)

Minio, et pas Mimio ;)
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +54,4 @@
if err != nil {
return false, "", err
}
if mc == nil {
Owner

tu peux mettre les deux if en un seul avec ||

tu peux mettre les deux `if` en un seul avec `||`
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +60,4 @@
var name, nameFull string
if nameConsul := login.UserEntry.GetAttributeValue("profilImage"); nameConsul != "" {
Owner

profilePicture au lieu de profilImage. Pour être sûr d'avoir la même valeur que partout, tu peux définir ça dans une constante:

const PROFILE_PICTURE_FIELD_NAME = "profilePicture"

et ensuite dans le code tu utilises toujours la constante, tu ne répète jamais "profilePicture"

`profilePicture` au lieu de `profilImage`. Pour être sûr d'avoir la même valeur que partout, tu peux définir ça dans une constante: `const PROFILE_PICTURE_FIELD_NAME = "profilePicture"` et ensuite dans le code tu utilises toujours la constante, tu ne répète jamais `"profilePicture"`
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +68,4 @@
nameFull = "full_" + name
}
_, err = mc.PutObject(context.Background(), "bottin-pictures", name, buff_thumbnail, int64(buff_thumbnail.Len()), minio.PutObjectOptions{
Owner

Le nom du bucket bottin-pictures ça doit être dans config aussi, d'autres gens peuvent avoir un bucket qui s'appelle autrement

Le nom du bucket `bottin-pictures` ça doit être dans config aussi, d'autres gens peuvent avoir un bucket qui s'appelle autrement
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +85,4 @@
return true, name, nil
}
func newMimioClient() (*minio.Client, error) {
Owner

Minio*

Minio*
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +95,4 @@
minioCLient, err := minio.New(endpoint, &minio.Options{
Creds: credentials.NewStaticV4(accessKeyID, secretKeyID, ""),
Secure: useSSL,
Region: "garage",
Owner

La région ça devrait aussi être dans la config

La région ça devrait aussi être dans la config
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +131,4 @@
if err != nil {
return errors.New("Decode: " + err.Error())
}
//Encode image to jpeg a first time to eliminate all problems
Owner

Pas besoin du premier encodage en jpeg, tu peux le virer

Pas besoin du premier encodage en jpeg, tu peux le virer
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +144,4 @@
}
buff.Reset()
images = resize.Thumbnail(200, 200, images, resize.Lanczos3)
images_thumbnail := resize.Thumbnail(80, 80, images, resize.Lanczos3)
Owner

Si l'image d'entrée est pas carrée, qu'est-ce que ça fait ?

Si l'image d'entrée est pas carrée, qu'est-ce que ça fait ?
Author
Member

J'ai testé avec des screenshots/ wallpapers et il n'y a eu aucun problèmes. ca les transforme en 200x196 le plus souvent. Si la taille est déjà inférieure à 200 alors il n'y touche pas.

J'ai testé avec des screenshots/ wallpapers et il n'y a eu aucun problèmes. ca les transforme en 200x196 le plus souvent. Si la taille est déjà inférieure à 200 alors il n'y touche pas.
lx marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +177,4 @@
dn,
ldap.ScopeBaseObject, ldap.NeverDerefAliases, 0, 0, false,
"(objectclass=*)",
[]string{"profilImage"},
Owner

PROFILE_PICTURE_FIELD_NAME

`PROFILE_PICTURE_FIELD_NAME`
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +191,4 @@
}
imageName = sr.Entries[0].GetAttributeValue("profilImage")
if imageName == "" {
http.Error(w, "User doesn't have profile image", http.StatusInternalServerError)
Owner

ça serait plutôt une 404 Not found et pas une 500 Internal error

ça serait plutôt une 404 Not found et pas une 500 Internal error
erwan marked this conversation as resolved
mimioClient.go Outdated
@ -0,0 +240,4 @@
if _, err := w.Write(buff); err != nil {
http.Error(w, "WriteBody: "+err.Error(), http.StatusInternalServerError)
return
}
Owner

tu peux simplifier tout ça en utilisant io.Copy(w, obj), ça t'évite de gérer un buffer et de prendre le risque de te planter sur les tailles ou les offset

tu peux simplifier tout ça en utilisant `io.Copy(w, obj)`, ça t'évite de gérer un buffer et de prendre le risque de te planter sur les tailles ou les offset
erwan marked this conversation as resolved
@ -0,0 +36,4 @@
identifiant.innerHTML = `<a href="/admin/ldap/${jsonResponse.search[i].dn}">${jsonResponse.search[i].identifiant}</a>`
name.innerHTML = jsonResponse.search[i].name
email.innerHTML = jsonResponse.search[i].email
description.innerHTML = jsonResponse.search[i].description
Owner

faudrait utiliser innerText plutôt que innerHTML, sinon on peut trivialement faire une injection de script

faudrait utiliser innerText plutôt que innerHTML, sinon on peut trivialement faire une injection de script
Author
Member

?? une injection de script dans un JS ? je ne comprends pas bien. J'ai quand-même appliqué tes modifications. Mais je ne serais pas contre plus de détails.

?? une injection de script dans un JS ? je ne comprends pas bien. J'ai quand-même appliqué tes modifications. Mais je ne serais pas contre plus de détails.
Owner

Imagine dans ma description je met <script>alert('coucou')</script>.

Avec innerText, ça apparait tel quel.

Avec innerHTML, quand quelqu'un affiche mon profil, ça lui fait une popup qui affiche coucou.

Imagine dans ma description je met `<script>alert('coucou')</script>`. Avec `innerText`, ça apparait tel quel. Avec `innerHTML`, quand quelqu'un affiche mon profil, ça lui fait une popup qui affiche coucou.
Author
Member

Donc si je comprends bien, si un utilisateur crée un compte qui se nomme <script>alert('coucou')</script> alors à chaque fois qu'on le cherchera sur l'annuaire ça produira cette pop-up ?

Donc si je comprends bien, si un utilisateur crée un compte qui se nomme `<script>alert('coucou')</script>` alors à chaque fois qu'on le cherchera sur l'annuaire ça produira cette pop-up ?
erwan force-pushed Add_Directory from 71638d4e0c to 27ea5101e4 2021-07-30 14:33:01 +00:00 Compare
erwan changed title from Add_Directory to Add_Directory_and_ProfilePicture 2021-07-30 14:33:21 +00:00
erwan requested review from lx 2021-07-30 14:33:38 +00:00
lx force-pushed Add_Directory from 27ea5101e4 to f00702b51c 2021-08-16 10:54:14 +00:00 Compare
lx added 2 commits 2021-08-16 13:30:41 +00:00
Improve profile editing page & photo uploading
Some checks are pending
continuous-integration/drone/push Build is pending
continuous-integration/drone/pr Build is pending
e94bd728ec
lx added 1 commit 2021-08-16 14:27:38 +00:00
Fix directory searching
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
01bf4aa522
lx added 1 commit 2021-08-16 14:28:49 +00:00
go fmt
Some checks are pending
continuous-integration/drone/push Build is pending
continuous-integration/drone/pr Build is pending
b1316fb653
lx added 1 commit 2021-08-16 14:31:13 +00:00
remove useless newline
Some checks are pending
continuous-integration/drone/pr Build is pending
continuous-integration/drone/push Build is passing
a187ae72cb
lx merged commit a187ae72cb into main 2021-08-16 14:44:53 +00:00
Sign in to join this conversation.
No reviewers
lx
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Deuxfleurs/guichet#9
No description provided.