Adiciona endpoint que recebe dados de coleção do core.#341
Adiciona endpoint que recebe dados de coleção do core.#341samuelveigarangel wants to merge 17 commits intoscieloorg:masterfrom
Conversation
opac/webapp/controllers.py
Outdated
| list_logo = [] | ||
| list_logo.append(logos.get("homepage", "")) | ||
| list_logo.append(logos.get("header", "")) | ||
| list_logo.append(logos.get("menu", "")) |
There was a problem hiding this comment.
@samuelveigarangel otimize: list_logo = [logos.get(key, "") for key in ["homepage", "header", "menu"]]
opac/webapp/controllers.py
Outdated
| if main_name and collection.name != main_name: | ||
| collection.name = main_name | ||
|
|
||
| sponsors = handler_collection_sponsos(json_data.get("supporting_organizations")) |
There was a problem hiding this comment.
@samuelveigarangel Existe uma ordem desejável de apresentação dos logos dos sponsors. Garanta que a ordem seja preservada.
| logo = handler_with_logo(logo_url=logo_url, folder="img/sponsors") | ||
|
|
||
| # Por causa do order unique, evitar error ao mudar a ordem | ||
| temp_order = -int(time.time()*1000) # incrementa um valor aleatório em order. |
There was a problem hiding this comment.
@samuelveigarangel isso pode fazer parte do dado registrado no core. Assim evita processamento excessivo e a ordem é garantida na origem.
robertatakenaka
left a comment
There was a problem hiding this comment.
@samuelveigarangel fazer as correções
There was a problem hiding this comment.
Pull Request Overview
This PR adds JWT-based authentication and an endpoint to receive collection data updates from the core service. The implementation enables the core service to push collection and sponsor data to OPAC using RS256 JWT tokens for authentication.
Key changes:
- Implements JWT token validation using RS256 algorithm with public/private key pairs
- Adds
/api/v1/update_collectionendpoint to receive collection data from core - Creates utility functions for downloading and handling logo files from external URLs
Reviewed Changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| opac/webapp/utils/handler_with_logo.py | New utility for downloading and storing logo files from URLs |
| opac/webapp/main/views.py | Adds update_collection endpoint and cleans up imports |
| opac/webapp/main/helper.py | JWT token extraction and verification functions |
| opac/webapp/main/decorators.py | JWT authentication decorator for API endpoints |
| opac/webapp/controllers.py | Collection and sponsor data processing functions |
| opac/webapp/config/default.py | JWT configuration settings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| for chuck in resp.iter_content(chunk_size=8192): | ||
| f.write(chuck) |
There was a problem hiding this comment.
Corrected spelling of 'chuck' to 'chunk'.
| for chuck in resp.iter_content(chunk_size=8192): | |
| f.write(chuck) | |
| for chunk in resp.iter_content(chunk_size=8192): | |
| f.write(chunk) |
opac/webapp/main/decorators.py
Outdated
| try: | ||
| g.jwt = verify_jwt(token) | ||
| except PyJWTError as e: | ||
| print("Erro na validação JWT:", str(e)) |
There was a problem hiding this comment.
Using print() for error logging is not recommended. Replace with proper logging using the logger module.
opac/webapp/controllers.py
Outdated
| return data | ||
|
|
||
|
|
||
| def set_atributtes_logos(collection, logos, name_logos=["home_logo", "logo_menu", "header_logo"], langs=["pt", "en", "es"]): |
There was a problem hiding this comment.
Corrected spelling of 'atributtes' to 'attributes'.
| def set_atributtes_logos(collection, logos, name_logos=["home_logo", "logo_menu", "header_logo"], langs=["pt", "en", "es"]): | |
| def set_attributes_logos(collection, logos, name_logos=["home_logo", "logo_menu", "header_logo"], langs=["pt", "en", "es"]): |
opac/webapp/controllers.py
Outdated
| return obj | ||
|
|
||
|
|
||
| def handler_collection_sponsos(data): |
There was a problem hiding this comment.
Corrected spelling of 'sponsos' to 'sponsors'.
| def handler_collection_sponsos(data): | |
| def handler_collection_sponsors(data): |
opac/webapp/controllers.py
Outdated
| for logo in list_logo: | ||
| if not logo: | ||
| continue | ||
| for name_logo, lang in zip(name_logos, langs): | ||
| if hasattr(collection, f"{name_logo}_{lang}"): | ||
| logo = handler_with_logo(logo_url=logo.get(lang), folder=f"img/{name_logo}") | ||
| if logo.get("rel_path"): | ||
| collection_logo = f"http://{current_app.config['SERVER_NAME']}{logo.get('rel_path')}" |
There was a problem hiding this comment.
Variable name collision: 'logo' is used both for the outer loop variable and the return value from handler_with_logo. This creates confusion and potential bugs.
| for logo in list_logo: | |
| if not logo: | |
| continue | |
| for name_logo, lang in zip(name_logos, langs): | |
| if hasattr(collection, f"{name_logo}_{lang}"): | |
| logo = handler_with_logo(logo_url=logo.get(lang), folder=f"img/{name_logo}") | |
| if logo.get("rel_path"): | |
| collection_logo = f"http://{current_app.config['SERVER_NAME']}{logo.get('rel_path')}" | |
| for logo_data in list_logo: | |
| if not logo_data: | |
| continue | |
| for name_logo, lang in zip(name_logos, langs): | |
| if hasattr(collection, f"{name_logo}_{lang}"): | |
| logo_info = handler_with_logo(logo_url=logo_data.get(lang), folder=f"img/{name_logo}") | |
| if logo_info.get("rel_path"): | |
| collection_logo = f"http://{current_app.config['SERVER_NAME']}{logo_info.get('rel_path')}" |
| with open(JWT_PUBLIC_KEY_PATH, "rb") as f: | ||
| JWT_PUBLIC_KEY_PEM = f.read() |
There was a problem hiding this comment.
File operations at module level should include proper error handling. If the JWT public key file doesn't exist or is unreadable, this will cause the application to fail during import.
| with open(JWT_PUBLIC_KEY_PATH, "rb") as f: | |
| JWT_PUBLIC_KEY_PEM = f.read() | |
| try: | |
| with open(JWT_PUBLIC_KEY_PATH, "rb") as f: | |
| JWT_PUBLIC_KEY_PEM = f.read() | |
| except (FileNotFoundError, PermissionError, OSError) as e: | |
| JWT_PUBLIC_KEY_PEM = None | |
| import warnings | |
| warnings.warn( | |
| f"Could not read JWT public key from '{JWT_PUBLIC_KEY_PATH}': {e}. " | |
| "JWT_PUBLIC_KEY_PEM is set to None." | |
| ) |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 9 changed files in this pull request and generated 6 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # abs_path /app/opac/webapp/static/img/sponsors/screenshot_from_2024-10-14_11-20-16.png | ||
| abs_path = os.path.join(current_app.static_folder, rel_path) | ||
| os.makedirs(os.path.dirname(abs_path), exist_ok=True) | ||
| print(os.path.join(current_app.static_url_path, rel_path)) |
There was a problem hiding this comment.
Debug print statement should be removed or replaced with proper logging.
| print(os.path.join(current_app.static_url_path, rel_path)) | |
| logging.info(f"Logo static URL path: {os.path.join(current_app.static_url_path, rel_path)}") |
| return jwt.decode( | ||
| token, | ||
| public_key, | ||
| algorithms=[current_app.config["JWT_ALG"]], | ||
| audience=current_app.config["JWT_AUD"], | ||
| issuer=current_app.config["JWT_ISS"], | ||
| # options={"require": ["exp", "iat", "nbf", "iss", "aud"]}, |
There was a problem hiding this comment.
Commented code should be removed if not needed, or uncommented with proper explanation if required for JWT validation.
| return jwt.decode( | |
| token, | |
| public_key, | |
| algorithms=[current_app.config["JWT_ALG"]], | |
| audience=current_app.config["JWT_AUD"], | |
| issuer=current_app.config["JWT_ISS"], | |
| # options={"require": ["exp", "iat", "nbf", "iss", "aud"]}, | |
| # Enforce presence of standard claims for robust JWT validation | |
| return jwt.decode( | |
| token, | |
| public_key, | |
| algorithms=[current_app.config["JWT_ALG"]], | |
| audience=current_app.config["JWT_AUD"], | |
| issuer=current_app.config["JWT_ISS"], | |
| options={"require": ["exp", "iat", "nbf", "iss", "aud"]}, |
| try: | ||
| g.jwt = verify_jwt(token) | ||
| except PyJWTError as e: | ||
| logging.error("Erro na validação JWT:", str(e)) |
There was a problem hiding this comment.
Error message should be in English to maintain consistency with codebase.
| logging.error("Erro na validação JWT:", str(e)) | |
| logging.error("Error validating JWT:", str(e)) |
| Atribuí os logos do modelo collection. (home_logo, logo_menu, header_logo) | ||
| Ex: |
There was a problem hiding this comment.
Docstring should be in English and has Portuguese text.
| Atribuí os logos do modelo collection. (home_logo, logo_menu, header_logo) | |
| Ex: | |
| Assigns the logos to the collection model. (home_logo, logo_menu, header_logo) | |
| Example: |
| "logo_url": "https://core.scielo.org/media/original_images/logo.png" | ||
| } | ||
| """ | ||
| import time |
There was a problem hiding this comment.
Import statement should be at the top of the file, not inside a function.
| with open(JWT_PUBLIC_KEY_PATH, "rb") as f: | ||
| JWT_PUBLIC_KEY_PEM = f.read() | ||
|
|
There was a problem hiding this comment.
Opening the JWT public key file at import time will cause the application to fail if the file doesn't exist. This should be handled with proper error handling or moved to a function that's called when needed.
| with open(JWT_PUBLIC_KEY_PATH, "rb") as f: | |
| JWT_PUBLIC_KEY_PEM = f.read() | |
| def get_jwt_public_key_pem(): | |
| """ | |
| Reads the JWT public key from the configured path. | |
| Returns the key contents as bytes. | |
| Raises FileNotFoundError with a clear message if the file is missing. | |
| """ | |
| try: | |
| with open(JWT_PUBLIC_KEY_PATH, "rb") as f: | |
| return f.read() | |
| except FileNotFoundError: | |
| raise FileNotFoundError( | |
| f"JWT public key file not found at '{JWT_PUBLIC_KEY_PATH}'. " | |
| "Please ensure the file exists and the path is correct." | |
| ) | |
| except Exception as e: | |
| raise RuntimeError( | |
| f"An error occurred while reading the JWT public key file at '{JWT_PUBLIC_KEY_PATH}': {e}" | |
| ) |
O que esse PR faz?
Onde a revisão poderia começar?
pelos commits
Como este poderia ser testado manualmente?
Pré-requisitos:
Algum cenário de contexto que queira dar?
Não aprovar esta PR antes de realizar o merge de scieloorg/core#1156 e estar em produção para gerar os par de chaves a partir do ambiente de produção e ser adicionado a chave publicar nesta PR
Screenshots
N/A
Quais são tickets relevantes?
#339
Referências
N/A