fix: Fixes 500 ORCID bug. Closes #96

Author: drorganvidez

app/modules/orcid/routes.py

@@ -1,4 +1,4 @@
-from flask import current_app, redirect, url_for
+from flask import current_app, flash, redirect, url_for
 from flask_login import login_user
 
 from app.modules.orcid import orcid_bp
@@ -7,19 +7,43 @@
 
 @orcid_bp.before_app_request
 def before_request():
+    # If you keep this pattern for now, at least guarantee the service exists.
+    # Consider moving OAuth client initialization to app factory later.
     current_app.orcid_service = OrcidService()
 
 
 @orcid_bp.route("/orcid/login")
 def login():
     redirect_uri = url_for("orcid.authorize", _external=True, _scheme="https")
-    return current_app.orcid_service.orcid_client.authorize_redirect(redirect_uri)
+
+    try:
+        return current_app.orcid_service.orcid_client.authorize_redirect(redirect_uri)
+    except Exception as exc:
+        current_app.logger.exception("ORCID authorize_redirect failed: %s", exc)
+        flash("ORCID login failed. Please try again.", "danger")
+        return redirect(url_for("auth.login"))  # adjust to your login route
 
 
 @orcid_bp.route("/orcid/authorize")
 def authorize():
-    token = current_app.orcid_service.orcid_client.authorize_access_token()
-    user_info = current_app.orcid_service.get_orcid_user_info(token)
-    user = current_app.orcid_service.get_or_create_user(user_info)
+    try:
+        token = current_app.orcid_service.orcid_client.authorize_access_token()
+    except Exception as exc:
+        # Covers state mismatch, missing session, provider errors, etc.
+        current_app.logger.exception("ORCID authorize_access_token failed: %s", exc)
+        flash("ORCID authorization failed (invalid session or provider error). Please try again.", "danger")
+        return redirect(url_for("auth.login"))
+
+    user_info, err = current_app.orcid_service.get_orcid_user_info(token)
+    if err:
+        flash(err, "danger")
+        return redirect(url_for("auth.login"))
+
+    user, err = current_app.orcid_service.get_or_create_user(user_info)
+    if err:
+        flash(err, "danger")
+        return redirect(url_for("auth.login"))
+
     login_user(user)
-    return redirect("/")
+    flash("Signed in with ORCID.", "success")
+    return redirect(url_for("public.index"))  # adjust to your home route

app/modules/orcid/services.py

@@ -1,5 +1,6 @@
 import os
 import secrets
+from sqlite3 import IntegrityError
 
 from authlib.integrations.flask_client import OAuth
 from flask import current_app
@@ -20,6 +21,12 @@ def __init__(self):
         super().__init__(OrcidRepository())
         self.client_id = self.get_orcid_client_id()
         self.client_secret = self.get_orcid_client_secret()
+
+        if not self.client_id or not self.client_secret:
+            # This will be caught by the routes and shown as flash if you wrap service usage,
+            # but right now you're creating it in before_app_request, so this would 500 early.
+            current_app.logger.error("ORCID_CLIENT_ID/ORCID_CLIENT_SECRET not configured")
+            # If you want to avoid raising here, set a flag and handle it in routes.
         self.oauth, self.orcid_client = self.configure_oauth(current_app)
 
     def get_orcid_client_id(self):
@@ -44,54 +51,67 @@ def configure_oauth(self, app):
         return oauth, orcid
 
     def get_orcid_user_info(self, token):
-        # Valida respuesta
-        resp = self.orcid_client.get("https://orcid.org/oauth/userinfo", token=token)
-        if not resp or resp.status_code != 200:
-            current_app.logger.error("ORCID userinfo fallo: %s", getattr(resp, "text", None))
-            return None
+        try:
+            resp = self.orcid_client.get("https://orcid.org/oauth/userinfo", token=token)
+        except Exception as exc:
+            current_app.logger.exception("ORCID userinfo request failed: %s", exc)
+            return None, "Could not reach ORCID. Please try again."
+
+        if not resp:
+            current_app.logger.error("ORCID userinfo empty response")
+            return None, "ORCID did not return user information. Please try again."
+
+        if resp.status_code != 200:
+            current_app.logger.error("ORCID userinfo failed (%s): %s", resp.status_code, getattr(resp, "text", None))
+
+            # Optional: special-case rate limiting
+            if resp.status_code == 429:
+                return None, "ORCID is rate-limiting requests. Please try again in a minute."
+
+            return None, "ORCID user information request failed. Please try again."
+
         data = resp.json() or {}
-        # 'sub' es el ORCID iD en OIDC
-        if "sub" not in data:
-            current_app.logger.error("ORCID userinfo sin 'sub': %s", data)
-            return None
-        return data
+
+        orcid_id = (data.get("sub") or "").strip()
+        if not orcid_id:
+            current_app.logger.error("ORCID userinfo missing 'sub': %s", data)
+            return None, "ORCID did not provide an ORCID iD. Please try again."
+
+        return data, None
 
     def get_or_create_user(self, user_info):
         if not user_info:
-            return None
+            return None, "Missing ORCID user information."
 
         orcid_id = (user_info.get("sub") or "").strip()
         if not orcid_id:
-            return None
+            return None, "Missing ORCID iD."
 
-        given_name = user_info.get("given_name") or ""
-        family_name = user_info.get("family_name") or ""
-        affiliation = user_info.get("affiliation") or ""  # si no existe, queda ""
+        given_name = (user_info.get("given_name") or "").strip()
+        family_name = (user_info.get("family_name") or "").strip()
+        affiliation = (user_info.get("affiliation") or "").strip()
 
         try:
-            # 1) ¿Existe registro ORCID?
+            # 1) Existing ORCID link?
             orcid_record = Orcid.query.filter_by(orcid_id=orcid_id).first()
-
             if orcid_record:
                 profile = UserProfile.query.get(orcid_record.profile_id)
                 user = User.query.get(profile.user_id) if profile else None
 
                 if user:
-                    return user
+                    return user, None
 
-                # Enlace roto: limpiamos y reconstruimos
-                if not profile or not user:
-                    db.session.delete(orcid_record)
-                    db.session.flush()  # no commit aún, seguimos para recrear
+                # Broken link: remove and recreate
+                db.session.delete(orcid_record)
+                db.session.flush()
 
-            # 2) Crear usuario + perfil + enlace ORCID
+            # 2) Create new user + profile + ORCID link
             user = User(
-                # puedes guardar email si algún día lo pides a ORCID con más scope
                 password=generate_password_hash(secrets.token_urlsafe(24)),
                 active=True,
             )
             db.session.add(user)
-            db.session.flush()  # para obtener user.id
+            db.session.flush()
 
             profile = UserProfile(
                 user_id=user.id,
@@ -106,9 +126,27 @@ def get_or_create_user(self, user_info):
             db.session.add(orcid_record)
 
             db.session.commit()
-            return user
+            return user, None
+
+        except IntegrityError as exc:
+            # Typical race condition: two callbacks creating the same ORCID simultaneously
+            current_app.logger.warning("IntegrityError creating ORCID user (%s): %s", orcid_id, exc)
+            db.session.rollback()
 
-        except SQLAlchemyError as e:
-            current_app.logger.exception("Error creando usuario ORCID: %s", e)
+            # Re-read and return the existing user if it was created in parallel
+            try:
+                orcid_record = Orcid.query.filter_by(orcid_id=orcid_id).first()
+                if orcid_record:
+                    profile = UserProfile.query.get(orcid_record.profile_id)
+                    user = User.query.get(profile.user_id) if profile else None
+                    if user:
+                        return user, None
+            except Exception as reread_exc:
+                current_app.logger.exception("Failed to reread ORCID user after IntegrityError: %s", reread_exc)
+
+            return None, "Could not create your account due to a concurrency issue. Please try again."
+
+        except SQLAlchemyError as exc:
+            current_app.logger.exception("Database error creating ORCID user (%s): %s", orcid_id, exc)
             db.session.rollback()
-            return None
+            return None, "Could not create your account due to a database error. Please try again."