Skip to content

Phase 5: User Management and OAuth Authentication#153

Open
devonjones wants to merge 1 commit intotestfrom
phase_5
Open

Phase 5: User Management and OAuth Authentication#153
devonjones wants to merge 1 commit intotestfrom
phase_5

Conversation

@devonjones
Copy link
Collaborator

Implemented comprehensive user management system with OAuth authentication support for Google and Patreon providers.

Features

  • OAuth authentication with Google and Patreon
  • Session-based authentication system
  • User role management (admin/user)
  • Patreon tier tracking
  • User blocking/unblocking functionality
  • Session management and revocation

API Endpoints

  • GET /api/users - List all users (admin only)
  • GET /api/users/:id - Get user details
  • PATCH /api/users/:id - Update user
  • DELETE /api/users/:id - Delete user
  • POST /api/users/:id/block - Block user
  • POST /api/users/:id/unblock - Unblock user
  • GET /api/users/:id/sessions - Get user sessions
  • DELETE /api/users/:id/sessions - Revoke all sessions

Database Changes

  • Added users table with role and Patreon tier tracking
  • Added user_identities table for OAuth provider mapping
  • Added user_id foreign key to sessions table
  • Added created_by/updated_by audit columns to content tables
  • Created indexes for performance optimization

Testing

  • Comprehensive unit tests for services
  • Integration tests for API endpoints
  • Test coverage for all authentication flows

🤖 Generated with Claude Code

Implemented comprehensive user management system with OAuth authentication
support for Google and Patreon providers.

## Features
- OAuth authentication with Google and Patreon
- Session-based authentication system
- User role management (admin/user)
- Patreon tier tracking
- User blocking/unblocking functionality
- Session management and revocation

## API Endpoints
- GET /api/users - List all users (admin only)
- GET /api/users/:id - Get user details
- PATCH /api/users/:id - Update user
- DELETE /api/users/:id - Delete user
- POST /api/users/:id/block - Block user
- POST /api/users/:id/unblock - Unblock user
- GET /api/users/:id/sessions - Get user sessions
- DELETE /api/users/:id/sessions - Revoke all sessions

## Database Changes
- Added users table with role and Patreon tier tracking
- Added user_identities table for OAuth provider mapping
- Added user_id foreign key to sessions table
- Added created_by/updated_by audit columns to content tables
- Created indexes for performance optimization

## Testing
- Comprehensive unit tests for services
- Integration tests for API endpoints
- Test coverage for all authentication flows

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @devonjones, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request delivers the foundational user management and authentication system, marking a significant step towards enabling user accounts and role-based access. It integrates external OAuth providers (Google and Patreon) for seamless sign-in, establishes a secure session management framework, and provides comprehensive administrative controls over user accounts. This lays the groundwork for future features requiring user identity and permissions.

Highlights

  • Comprehensive User Management System: Introduced new database tables (users, user_identities) and a dedicated UserService to handle user creation, retrieval, updates, and deletion, including user roles (user/admin) and Patreon tier tracking.
  • OAuth 2.0 Authentication: Implemented full OAuth 2.0 flows for Google and Patreon, allowing users to sign in via these providers. This includes token exchange, user information retrieval, and mapping Patreon tiers to internal roles.
  • Enhanced Session Management: Extended the existing session service to support user-specific sessions, including secure token generation, CSRF protection, and session revocation. Sessions now carry user data and are invalidated if a user is blocked.
  • New API Endpoints for Authentication and User Management: Added a suite of new API endpoints under /api/auth for login, callback, logout, and fetching current user details, and under /api/users for listing, getting, updating, deleting, blocking, unblocking users, and managing user sessions (admin-only or self-access where appropriate).
  • Database Schema Updates: Introduced a new database migration (version_15.py) to create users and user_identities tables, add a patreon_tier_enum type, and link existing sessions and content tables (blueprints, tags, tag_descriptions, images) to the new user system with user_id, created_by, and updated_by columns.
  • Robust Testing Coverage: Added extensive unit and integration tests for the new OAuth services, user services, session management, and API endpoints, ensuring reliability and security of the new authentication and user management features.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive user management and OAuth authentication system, which is a major and well-executed feature addition. The implementation covers database schema changes, new services for handling OAuth and user logic, new API endpoints, and a suite of unit and integration tests. My review focuses on enhancing security, improving data integrity through database constraints, and increasing the robustness of the new endpoints. I've identified a critical security vulnerability related to OAuth state validation that must be addressed. Other suggestions include improving token handling, adding database constraints, and making the code more resilient to invalid input.

Comment on lines +49 to +62
# state = request.args.get("state") # TODO: Implement CSRF validation
error = request.args.get("error")

if error:
# OAuth error (user denied, etc)
return redirect(
f"{current_app.config['FRONTEND_URL']}/auth/error?"
+ urlencode({"error": error})
)

if not code:
raise BadRequest("Missing authorization code")

# TODO: Validate state for CSRF protection
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

The state parameter from the OAuth flow is not being validated in the callback. This is a critical security vulnerability that exposes the application to Cross-Site Request Forgery (CSRF) attacks. An attacker could trick a user into authorizing the attacker's account on the user's session.

The state value generated during the login request should be stored (e.g., in a short-lived, signed cookie) and then compared with the state parameter received in the callback. The authentication flow should only proceed if they match.

Comment on lines +107 to +115
return redirect(
f"{current_app.config['FRONTEND_URL']}/auth/success?"
+ urlencode(
{
"token": session_data["session_token"],
"csrf": session_data["csrf_token"],
}
)
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Passing session and CSRF tokens in URL query parameters is a security risk. These sensitive tokens can be leaked through:

  • Browser history
  • Server logs (on your side and on any intermediate proxies)
  • Referrer headers when navigating away from the page

A more secure practice is to set these tokens as HttpOnly cookies on the response, which prevents client-side JavaScript from accessing them.

CREATE TABLE users (
id uuid PRIMARY KEY DEFAULT gen_random_uuid(),
email text NOT NULL UNIQUE,
role text NOT NULL DEFAULT 'user',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The users.role column is defined as text without a CHECK constraint. While the application layer validates the role, adding a database-level constraint (CHECK (role IN ('user', 'admin'))) provides a stronger guarantee of data integrity. This prevents invalid data from entering the database through any means, not just the application API.

Suggested change
role text NOT NULL DEFAULT 'user',
role text NOT NULL DEFAULT 'user' CHECK (role IN ('user', 'admin')),

Comment on lines +40 to +45
import json

app.config["PATREON_TIER_MAP"] = json.loads(os.environ["PATREON_TIER_MAP"])

if os.environ.get("PATREON_TIER_AMOUNTS"):
import json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The import json statements are located inside if blocks. According to PEP 8, imports should be at the top of the file. This improves readability and makes it easier to see the module's dependencies at a glance.

)
)

except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Catching a generic Exception is too broad. It can hide unexpected errors and make debugging difficult. It's better to catch more specific exceptions that you expect to handle, such as requests.exceptions.RequestException for network issues or ValueError for JSON parsing errors. This allows unknown exceptions to propagate and be logged as server errors.

Suggested change
except Exception as e:
except (requests.exceptions.RequestException, ValueError) as e:

Comment on lines +37 to +38
limit = min(int(request.args.get("limit", 100)), 1000)
offset = int(request.args.get("offset", 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The int() conversion for limit and offset query parameters is not safe. If a non-numeric string is passed (e.g., ?limit=abc), it will raise a ValueError and result in a 500 Internal Server Error. You should wrap this in a try-except block to handle invalid input gracefully and return a 400 Bad Request.

Suggested change
limit = min(int(request.args.get("limit", 100)), 1000)
offset = int(request.args.get("offset", 0))
try:
limit = min(int(request.args.get("limit", 100)), 1000)
offset = int(request.args.get("offset", 0))
except ValueError:
raise BadRequest("Invalid 'limit' or 'offset' parameter. Must be an integer.")

role text NOT NULL DEFAULT 'user',
patreon_tier patreon_tier_enum,
blocked boolean DEFAULT false,
blocked_at timestamp,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The blocked_at column uses timestamp, while other timestamp columns in the project use timestamptz. Using timestamp without time zone can lead to ambiguity and bugs if the application servers run in different timezones or if the server timezone changes. It's best practice to consistently use timestamp with time zone (timestamptz) for all timestamp columns to store them unambiguously in UTC.

Suggested change
blocked_at timestamp,
blocked_at timestamptz,

Comment on lines +269 to +271
if response.status_code != 200:
print(f"Response status: {response.status_code}")
print(f"Response: {response.json()}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This if block with print statements appears to be leftover debugging code. A similar block exists in test_block_user_flow. These should be removed from the final test implementation to keep the tests clean.

devonjones added a commit that referenced this pull request Dec 29, 2025
🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
devonjones added a commit that referenced this pull request Dec 30, 2025
* feat: add sprite sheet support for 3D model thumbnails

Implements sprite sheet generation and interactive viewing for OpenForge 3D models, reducing storage costs and improving user experience with 360° model previews.

## Backend Changes
- Add `sprite_metadata` JSONB column to images table (schema v15-16)
- Update image SQL queries to handle sprite_metadata
- Add `/api/sprites` endpoint for sprite sheet generation
- Fix image comparison to detect sprite_metadata changes
- Update `insert_image()` to use ON CONFLICT DO UPDATE for sprite metadata

## Scanner Improvements
- Add `--sprites` flag to generate sprite sheets during scan
- Add `bin/generate_sprite` tool for sprite sheet creation
- Add `bin/resort_fixtures` tool for stable fixture sorting
- Fix fixture sorting to use full_name for top-level array
- Use /tmp for temp sprite files to avoid path length issues
- Preserve sprite_metadata.angles sort order by index

## Frontend Components
- Add SpriteViewer component with interactive 3D rotation
- Implement unwrapped cube layout for angle selection (10 angles)
- Add keyboard navigation (arrow keys for rotation)
- Add mouse drag support for horizontal rotation
- Auto-focus viewer for immediate keyboard interaction
- Reset angle when switching between blueprints

## Type Definitions
- Add SpriteThumbnailData, SpriteAngle types
- Update ThumbnailData union type
- Add comprehensive test coverage (27 tests for SpriteViewer)

## Testing
- Add sprite_metadata change detection test
- Update image-gallery tests for sprite support
- Add full SpriteViewer test suite (rendering, controls, keyboard, mouse, a11y)

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* adding design

* fix: address review feedback - improve code quality and maintainability

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: use tempfile.gettempdir() for subprocess cwd

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: decompose generate_sprite into focused helper functions

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add sprite_metadata support to update_image()

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* chore: remove accidentally shipped progress tracking file

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* chore: remove accidentally shipped fixture data files

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* chore: restore sewers.json to original state

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* chore: remove version_15.py - should come from PR #153

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: extract sprite viewer event handling into custom hooks

- Extract keyboard rotation logic into useKeyboardRotation hook
- Extract mouse drag rotation logic into useMouseDragRotation hook
- Reduce SpriteViewer component from handling state + rendering + keyboard + mouse events to just state + rendering
- Follows single responsibility principle and reduces cognitive load
- Raise NotImplementedError for --db-update flag instead of silent warning

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* feat: remove --db-update flag from generate_sprite

Database updates will be handled through the fixture workflow,
not directly by this CLI tool.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: use key prop pattern for sprite viewer state reset

Replace useEffect-based state reset with React key prop pattern.
When sprite_url changes, React will unmount and remount the
component with fresh state - more idiomatic than manual reset.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: address code quality improvements from Gemini review

1. Add try/finally cleanup for sprite files in bin/generate_sprite
2. Validate default_angle against actual sprite_metadata instead of hardcoded max
3. Add try/finally cleanup for temp files in create_and_upload_thumbnail
4. Simplify CTE in insert_image (remove unnecessary WITH clause)
5. Derive angle constants from sprite metadata instead of hardcoding

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: use blueprint.id for key and dynamic angleMap

1. Use blueprint.id instead of sprite_url for SpriteViewer key
   - More explicit and robust for component remounting
2. Build angleMap dynamically from angle names
   - Decouple frontend from backend array order
   - Component adapts if angle structure changes

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix: add specific error handling for stl-thumb failures

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* refactor: improve exception handling in helper functions

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

* security: only pass required environment keys to config

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant