-
Notifications
You must be signed in to change notification settings - Fork 17
feat: add auto_validate_new_user boolean field #143
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ | |
| import datetime | ||
| import logging | ||
| from typing import List | ||
| from urllib.parse import urljoin | ||
| from urllib.parse import urlencode, urljoin | ||
|
|
||
| import sqlalchemy as sa | ||
| from flask import ( | ||
|
|
@@ -216,7 +216,17 @@ def logout(): | |
| @routes.route("/authorize/<provider>", methods=["GET", "POST"]) | ||
| def authorize(provider="local_provider"): | ||
| auth_provider = current_app.auth_manager.get_provider(provider) | ||
| authorize_result = auth_provider.authorize() | ||
| try: | ||
| authorize_result = auth_provider.authorize() | ||
| except (Unauthorized, Forbidden) as exc: | ||
| log.exception("Authorization error for provider %s", provider) | ||
| error_description = exc.description or "Unauthorized" | ||
| login_url = f"{current_app.config['URL_APPLICATION']}/#/login" | ||
| query_params = { | ||
| "login_error": error_description, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Pour l'instant pas de prise en compte dans le frontend de GeoNature de ce querry param, donc, quand on essaye de créer un compte avec AUTO_VALIDATE_NEW_USER à false, ça nous renvoie sur la page sans message de succès ou d'erreur. Par ailleurs, pour ce cas spécifique, est-ce que c'est vraiment une erreur que l'on veut afficher ? Il semblerait plus logique de mettre un succès ou une info reprennant
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah tiens, j'avais oublié de faire la PR. C'est chose faite :) J'ai opté pour l'erreur pour se brancher au plus simplement sur la page existante. Je suis ouvert à d'autres options, mais j'ai peur que ce soit plus/trop impactant
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Je regarde ça avec ton autre PR, merci :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bon en effet, ça demande beaucoup de modification, j'ai essayé de le faire et je me suis un peu lancé dans un tunnel ... J'ai fais deux PR en rapport naturalsolutions/geonature#10, et naturalsolutions#1 à voir ce qu'on en fait
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Le travail de @christophe-ramet sera merge après le merge de cette branche |
||
| } | ||
| return redirect(f"{login_url}?{urlencode(query_params)}", code=302) | ||
|
|
||
| if isinstance(authorize_result, models.User): | ||
| login_user(authorize_result, remember=True) | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le fonctionnement par défaut est donc modifié (jusque là, le user se créait automatiquement). Toutefois, c'est le fonctionnement par défaut dans la connexion classique
AUTO_ACCOUNT_CREATION=false, donc je pense que c'est bien de faire comme tu as fait et qu'il faudra juste le notifier dans le changelog.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a la base, c'était une erreur ! J'ai mis faux pour le dev, j'ai juste oublié de remettre à vrai pour la PR.
Je laisse comme ça du coup ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui, ça me semble bien de le laisser comme ça. Il faut juste mettre à jour la description de la PR où tu dis :
En notifiant bien que le comportement à changer pour mettre en cohérence avec celui de l'auth de base.