Skip to content
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

First release #1

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open

First release #1

wants to merge 25 commits into from

Conversation

KraPete
Copy link
Collaborator

@KraPete KraPete commented Jan 14, 2025

Сервис аутентификации и управления пользователями для Homeui

echo "deb http://deb.wirenboard.com/all experimental.homeui-https main" > /etc/apt/sources.list.d/homeui-https.list

Summary by CodeRabbit

Release Notes: Wiren Board Home UI Authentication Service

New Features

  • Introduced a comprehensive authentication and authorization service for the Wiren Board web interface.
  • Added user management capabilities including login, logout, and user type management.
  • Implemented secure JWT-based authentication with bcrypt password hashing.
  • Added a new command for directory management in the authentication service.
  • Implemented standardized HTTP response generation for various scenarios.

Security Enhancements

  • Added cryptographic key management system.
  • Implemented role-based access control for different user types.
  • Introduced secure cookie management.

Documentation

  • Updated project README to reflect recent changes.
  • Added MIT License.
  • Created comprehensive Debian packaging configuration.

Testing

  • Developed extensive unit test suite for authentication handlers.
  • Added test coverage for various authentication scenarios.

Infrastructure

  • Created Debian package configuration.
  • Added Jenkins build pipeline configuration.
  • Implemented system service configuration for the authentication service.
  • Introduced a logging mechanism for better traceability.
  • Updated .gitignore to streamline repository by excluding unnecessary files.

@KraPete KraPete requested a review from a team January 14, 2025 07:42
LICENSE Outdated
@@ -0,0 +1,21 @@
MIT License

Copyright (c) 2022-2023 Wiren Board
Copy link

Choose a reason for hiding this comment

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

Suggested change
Copyright (c) 2022-2023 Wiren Board
Copyright (c) 2025 Wiren Board

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Поменял

self.process_request({"users": delete_user_handler})


def main():
Copy link

Choose a reason for hiding this comment

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

Может порт и базу аргументами?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Вынес в параметры командной строки

from setuptools import setup


def get_version():

Choose a reason for hiding this comment

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

понадежнее бы что-нибудь

предлагаю - или регексп
re.match(r"wb-homeui-auth \((?P<version>.*)\)", "wb-homeui-auth (1.0.0) stable; urgency=medium").group("version")

или вообще - subprocess и дебиан-тулзы
dpkg-parsechangelog --show-field Version

я за регексп

Copy link

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Сделал re, как Вова хотел

try:
decoded = jwt.decode(cookie, keys_storage.get_key(), algorithms=["HS256"])
return decoded.get("id")
except Exception:

Choose a reason for hiding this comment

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

логгировать хотя бы?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Убрал обработку ошибки, теперь это падает в ответ 500 Internal Server Error

return ["Set-Cookie", f"id={user_id}; HttpOnly; SameSite=Lax;"]


def get_cookie_params_dict(params: str) -> dict:

Choose a reason for hiding this comment

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

вроде есть какаят стандартная парсилка куков

https://stackoverflow.com/questions/32281041/converting-cookie-string-into-python-dict

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Я знаю, но раньше это всё запускалось отдельным скриптом через fcgiwrap, не хотел тащить зависимости, чтоб время загрузки скрипта уменьшить. Сейчас не важно, переделал на SimpleCookie

keys_storage: KeysStorage = None


def get_required_user_type(request: BaseHTTPRequestHandler) -> UserType:

Choose a reason for hiding this comment

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

мб return UserType(request.headers.get("Required-User-Type", UserType.ADMIN) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Я хотел, чтоб при неправильном типе пользователя тоже был админ, но чё-то сейчас подумал, что не надо. Пусть лучше будет 500 Internal Server Error

length = int(request.headers.get("Content-Length"))
form = json.loads(request.rfile.read(length).decode("utf-8"))
validate_login_request(form)
except Exception as e:

Choose a reason for hiding this comment

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

логи?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Все ответы, которые не 200, идут через http.server.send_error, который по умолчанию в лог падает

USER = "user"

@dataclass
class User:
Copy link

@vdromanov vdromanov Jan 15, 2025

Choose a reason for hiding this comment

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

как идея: можно здесь сделать все None по дефолту и убрать кучу проверок на is None выше

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Тогда надо проверять содержимое User. Не, не хочу

@KraPete KraPete requested a review from vdromanov January 16, 2025 06:28
@KraPete KraPete requested a review from sikmir January 16, 2025 10:42
@@ -0,0 +1,4 @@
buildDebSbuild defaultTargets: 'bullseye-armhf',
defaultRunPythonChecks: true,
Copy link

Choose a reason for hiding this comment

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

Suggested change
defaultRunPythonChecks: true,
defaultRunPythonChecks: true,
defaultRunLintian: true,

Copy link

@vdromanov vdromanov left a comment

Choose a reason for hiding this comment

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

у матросов
иссяк список вопросов

* change error response body format
* do not send user type in response to check_auth request, as it is not used any more
* send user type in response to login request
* remove check_config endpoint
* add who_am_i endpoint to get own user type
from .users_storage import User, UsersStorage, UserType, is_admin

DEFAULT_PORT = 8180
DEFAULT_DB_FILE = "/var/lib/wb-webui/users.db"
Copy link
Member

Choose a reason for hiding this comment

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

надо добавить в wb-configs.d чтобы переживало обновление прошивки

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Добавил

from .keys_storage import KeysStorage
from .users_storage import User, UsersStorage, UserType, is_admin

DEFAULT_PORT = 8180
Copy link
Member

Choose a reason for hiding this comment

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

может юникс сокет, а? А то пересечётся ещё с кем-нибудь по порту, будет печально

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Сделал юникс сокет

Copy link

coderabbitai bot commented Jan 24, 2025

Walkthrough

This pull request introduces a comprehensive authentication service for the Wiren Board web interface. The project includes a Python-based authentication system with user management, JWT-based authentication, and secure key storage. The implementation covers various aspects such as user login, logout, user type management, and access control. The changes span multiple files, introducing new modules for handling HTTP responses, user storage, keys management, and the main authentication logic. Additionally, it updates the .gitignore, adds a new command, and includes comprehensive unit tests.

Changes

File Change Summary
.gitignore Added entries for build artifacts, VS Code settings, direnv, coverage reports, cache files
21wb-homeui-auth Added wb_move command for directory relocation
Jenkinsfile Introduced buildDebSbuild method with comprehensive build parameters
LICENSE Added MIT License for 2025
README.md Removed authentication service description
debian/... Added comprehensive Debian packaging files for wb-homeui-auth, including control, changelog, compat, rules, and install files
setup.py Created setup configuration for package installation
tests/main_test.py Added comprehensive unit tests for authentication handlers
wb/homeui_auth/... Created multiple modules for authentication service implementation, including HTTP response handling, user storage, and main server logic

Sequence Diagram

sequenceDiagram
    participant Client
    participant AuthServer
    participant UserStorage
    participant KeyStorage

    Client->>AuthServer: Login Request
    AuthServer->>UserStorage: Validate User Credentials
    UserStorage-->>AuthServer: User Validation Result
    alt Valid Credentials
        AuthServer->>KeyStorage: Generate JWT Token
        KeyStorage-->>AuthServer: Signed Token
        AuthServer->>Client: Authentication Success + Cookie
    else Invalid Credentials
        AuthServer->>Client: Authentication Failure
    end
Loading

Poem

🐰 Hop, hop, authentication's here to stay,
Secure our web with tokens today!
Users login, admins control,
JWT magic makes us whole!
Security rabbit says: "Access granted!" 🔐

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (19)
wb/homeui_auth/main.py (5)

84-84: Use more Pythonic membership checks.
Static analysis suggests employing if "key" not in form: instead of if "key" not in form.keys():. This provides cleaner, more natural Python code.

-    if "password" not in form.keys():
+    if "password" not in form:

-    if "login" not in form.keys():
+    if "login" not in form:

-    if "password" not in request.keys():
+    if "password" not in request:

-    if "login" not in request.keys():
+    if "login" not in request:

Also applies to: 90-90, 101-101, 107-107

🧰 Tools
🪛 Ruff (0.8.2)

84-84: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


399-402: Leverage contextlib.suppress for concise error handling.
Instead of a try-except-pass block, consider using contextlib.suppress(OSError) to improve readability and avoid empty exception handlers.

+import contextlib

 try:
     os.remove(args.socket_file)
 except OSError:
     pass

+with contextlib.suppress(OSError):
+    os.remove(args.socket_file)
🧰 Tools
🪛 Ruff (0.8.2)

399-402: Use contextlib.suppress(OSError) instead of try-except-pass

Replace with contextlib.suppress(OSError)

(SIM105)


185-208: Refactor user creation flow to unify validations.
Both this handler and other user handlers (e.g., “update_user_handler”) contain similar validation and user-handling logic. Consider refactoring them into a unified helper to keep creation and update flows consistent and reduce code duplication.


122-124: Double-check fallback to UserType.ADMIN for invalid or missing headers.
Defaulting to UserType.ADMIN might cause unexpected escalations if the header is malformed or missing. Consider verifying that the header’s value is valid and returning a proper error if it’s not.


339-363: Aim for DRY request handling methods.
The “do_GET,” “do_POST,” “do_PUT,” and “do_DELETE” methods all funnel into self.process_request(...). This is good, but if additional logic accumulates in these methods, consider a more unified approach (e.g., a single method dispatch). This reduces duplication and simplifies maintenance.

wb-homeui-auth (1)

8-12: Handle exceptions comprehensively in the main entry point.
Catching KeyboardInterrupt is good, but more comprehensive handling or logging of other exceptions could help diagnose issues if the script encounters unexpected problems.

debian/wb-homeui-auth.service (2)

7-7: Avoid running the service as root if possible.
Running as root may pose security risks. If feasible, run the service under a dedicated low-privilege user.


8-11: Fine-tune restart behavior.
Using Restart=on-failure is good, but consider what should happen on non-failure exit codes other than the ones specified in RestartPreventExitStatus. You may also want to ensure logs are collected to diagnose repeated failures.

debian/control (1)

7-7: Assess Python version requirement.
Requiring Python 3.9 or higher could be restrictive for certain environments. Ensure that your service indeed needs Python 3.9 features and won't run on older Python 3.x releases.

wb/homeui_auth/http_response.py (1)

14-35: Provide consistent response format.
Currently, 4xx and 5xx responses can include a string body, but 2xx responses might or might not include a body. Consider standardizing your responses—possibly returning JSON or at least consistent text messaging—to help clients parse errors or successes more reliably.

wb/homeui_auth/keys_storage.py (2)

8-12: Consider unique constraints or a single-key approach
Currently, the code allows multiple key rows within the keys table and does not enforce any uniqueness constraints. If the intention is to store only a single key at a time, it might be safer to either enforce a unique constraint or store only one row. Otherwise, you risk accumulating unused or inconsistent keys over time.


24-32: Ensure consistent key retrieval for multi-row scenarios
get_key() fetches the first available row, but if multiple rows exist, it may return outdated data. Consider using an explicit ordering (e.g., timestamp) or restricting the table to a single row to avoid retrieving the wrong key.

wb/homeui_auth/users_storage.py (4)

15-28: Validate new user types in has_access_to()
If new user types are introduced later, this method may need updating to handle them. Consider refactoring to avoid enumerating all roles in conditionals (e.g., a role hierarchy or a dictionary-based permission scheme) for easier scalability.


53-57: Properly handle user references before deletion
When deleting users, ensure no orphan references exist. For instance, if user data is referenced in other tables or processes, consider either a cascading delete or a check for dependent data.


58-65: Minimal update logic
The update statement cleanly updates any combination of login, pwd_hash, and type. You might consider verifying that changes do not conflict with existing constraints (e.g., duplicate logins) if you allow changing the username.


81-90: Return None for invalid IDs
Similarly, returning None for missing user_id is consistent with the login lookup. Consider logging potential suspicious requests to track repeated attempts of invalid user IDs.

tests/main_test.py (2)

73-100: Comprehensive coverage for user listing
Testing both the admin and non-admin scenarios ensures robust coverage. The approach to verifying the JSON response is correct. Consider adding edge cases like an empty user list to ensure full coverage.


153-181: Complete tests for “Who Am I?”
Verifies scenarios with no users, an authenticated user, and an unauthenticated user. This is a solid test of possible states. You may want to add additional checks to confirm the JSON structure beyond just user type, if future logic requires more user metadata.

Jenkinsfile (1)

1-5: Ensure coverage criteria matches project goals
A defaultCoverageMin of 50 might be too low if you aim for higher reliability. Consider incrementally raising this threshold to encourage more thorough test coverage over time.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5949340 and 9ae939d.

📒 Files selected for processing (19)
  • .gitignore (1 hunks)
  • 21wb-homeui-auth (1 hunks)
  • Jenkinsfile (1 hunks)
  • LICENSE (1 hunks)
  • README.md (0 hunks)
  • debian/changelog (1 hunks)
  • debian/compat (1 hunks)
  • debian/control (1 hunks)
  • debian/rules (1 hunks)
  • debian/wb-homeui-auth.install (1 hunks)
  • debian/wb-homeui-auth.service (1 hunks)
  • setup.py (1 hunks)
  • tests/main_test.py (1 hunks)
  • wb-homeui-auth (1 hunks)
  • wb/homeui_auth/__init__.py (1 hunks)
  • wb/homeui_auth/http_response.py (1 hunks)
  • wb/homeui_auth/keys_storage.py (1 hunks)
  • wb/homeui_auth/main.py (1 hunks)
  • wb/homeui_auth/users_storage.py (1 hunks)
💤 Files with no reviewable changes (1)
  • README.md
✅ Files skipped from review due to trivial changes (7)
  • debian/compat
  • 21wb-homeui-auth
  • debian/changelog
  • wb/homeui_auth/init.py
  • LICENSE
  • debian/wb-homeui-auth.install
  • .gitignore
🧰 Additional context used
🪛 Ruff (0.8.2)
wb/homeui_auth/main.py

84-84: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


90-90: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


101-101: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


107-107: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


399-402: Use contextlib.suppress(OSError) instead of try-except-pass

Replace with contextlib.suppress(OSError)

(SIM105)

🔇 Additional comments (17)
wb/homeui_auth/main.py (1)

130-131: Verify security implications of “allow user only if no users exist.”
When no users exist, the code automatically permits UserType.USER. Ensure this behavior truly reflects the intended security model, since it might allow unauthorized usage if your environment expects an admin from the get-go.

debian/rules (2)

6-7: Confirm Python 3 build compatibility.
The rules specify Python 3 via --with python3 --buildsystem=pybuild. Ensure the code supports your targeted Python 3 versions, especially if you rely on features from a more recent Python release.


9-10: Assess the impact of skipping installation scripts.
Passing --noscripts to dh_installinit may bypass essential init scripts. Confirm that your service is being set up properly (systemd unit or otherwise) and that no post-install or pre-remove steps are required.

debian/wb-homeui-auth.service (1)

1-4: Consider adding network.target dependency.
If the authentication service depends on network connectivity (e.g., to validate credentials or connect to external services), ensure that the service starts after the network is up by adding:

After=network.target
Wants=network.target

Re-check if this is strictly necessary for your use case.

debian/control (1)

13-13: Check recommended package version alignment.
Confirm that wb-mqtt-homeui (>= 2.108.0) is actually needed and works as intended with your new authentication service. If there's a known minimum version, ensure it's thoroughly tested.

setup.py (1)

13-27: Overall project metadata looks good.
The setup configuration and metadata appear well-structured. Good job documenting author and maintainer details.

wb/homeui_auth/keys_storage.py (1)

19-23: Handle potential database write errors
Storing the key without any error handling could lead to unexpected states if the database write fails. Consider wrapping database operations in try-except blocks for more robust error handling.

wb/homeui_auth/users_storage.py (8)

9-12: Enum usage is clean and extensible
Defining the UserType enum is a neat and explicit approach, helping maintain clarity in user-role logic.


30-32: Shortcut for admin checks
The is_admin() helper is succinct and clear. This keeps the code readable for admin-specific logic.


34-42: Store password hashes securely
Storing password hashes in a TEXT column is acceptable if they are sufficiently hashed and salted. However, consider utilizing a well-tested hashing library (e.g., bcrypt, Argon2) to ensure security.
[security]


43-52: Handle uniqueness violations during user creation
login is marked as UNIQUE. If an insert fails due to a duplicate login, it will raise an exception. You may want to catch such exceptions and return a user-friendly error or handle it gracefully in the calling logic.


66-70: Efficient existence check
SELECT EXISTS (...) is a solid approach for quickly verifying if at least one user exists.


71-80: Return None for invalid logins
Returning None from get_user_by_login is a straightforward pattern for non-existent users. Ensure your calling code handles these None cases gracefully to prevent uncaught errors.


91-95: Counting roles
count_users_by_type() is clearly structured. You might want to check if the type is valid or convert to enum up front, but as is, it’s succinct.


96-99: Avoid exposing hashed passwords
get_users excludes password hashes from the result, which is a good security practice. Great job protecting sensitive data.

tests/main_test.py (2)

23-71: Verify the correctness of status code usages
Among these tests, non-admin, anonymous users, or invalid paths frequently return 403 or 404. Double-check that returning 403 is correct in each scenario, versus a 401 (unauthorized) if authentication was absent. This can impact user experience by providing more accurate HTTP status reasons.


102-151: Appropriate coverage for auth checks
The variety of scenarios (no required user type, no user, user present, required admin, etc.) demonstrates strong coverage. Good job enumerating these different states.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (7)
tests/main_test.py (3)

24-72: Add test case for deleting the last admin user.

The test suite thoroughly covers most scenarios but is missing a test case for attempting to delete the last admin user, which should be prevented to maintain system access.

Add this test case:

def test_delete_last_admin(self):
    self.request.path = "/users/123"
    self.context.user = User("1", "admin1", "password1", UserType.ADMIN)
    user_id = "123"
    user = User(user_id, "admin2", "password2", UserType.ADMIN)
    self.context.users_storage.get_user_by_id.return_value = user
    self.context.users_storage.count_users_by_type.return_value = 1

    response = delete_user_handler(self.request, self.context)

    self.context.users_storage.get_user_by_id.assert_called_once_with(user_id)
    self.context.users_storage.count_users_by_type.assert_called_once_with(UserType.ADMIN)
    self.assertEqual(response, response_400("Can't delete the last admin"))

74-101: Add test case for empty users list.

The test suite should include a test case for when the users list is empty to ensure proper handling of this edge case.

Add this test case:

def test_admin_empty_users(self):
    users_storage = MagicMock(spec=UsersStorage)
    user = User("1", "user1", "password1", UserType.ADMIN)
    users_storage.get_users.return_value = []

    response = get_users_handler(None, WebRequestHandlerContext(user=user, users_storage=users_storage))

    self.assertEqual(
        response, response_200([["Content-type", "application/json"]], json.dumps([]))
    )

185-204: Add test cases for missing or empty headers.

The test suite only covers the happy path. Add test cases for:

  1. Missing X-Forwarded-Host-Ip header
  2. Empty X-Forwarded-Host-Ip header

Add these test cases:

def test_device_info_handler_missing_header(self):
    self.request.headers = {}
    self.context.sn = "ABC123"
    response = device_info_handler(self.request, self.context)

    self.assertEqual(response.status, 200)
    self.assertEqual(
        response.headers,
        [
            ["Content-type", "application/json"],
            ["Access-Control-Allow-Origin", "*"],
        ],
    )
    self.assertEqual(json.loads(response.body), {"sn": "ABC123", "ip": ""})

def test_device_info_handler_empty_header(self):
    self.request.headers = {"X-Forwarded-Host-Ip": ""}
    self.context.sn = "ABC123"
    response = device_info_handler(self.request, self.context)

    self.assertEqual(response.status, 200)
    self.assertEqual(
        response.headers,
        [
            ["Content-type", "application/json"],
            ["Access-Control-Allow-Origin", "*"],
        ],
    )
    self.assertEqual(json.loads(response.body), {"sn": "ABC123", "ip": ""})
wb/homeui_auth/main.py (4)

38-40: Enhance JWT token security.

Consider adding the following improvements to the JWT token:

  1. Add token expiration (exp claim)
  2. Add token issuance time (iat claim)
  3. Add token issuer (iss claim)

Apply this diff:

def make_cookie_value(user_id: str, keys_storage: KeysStorage) -> str:
-    return jwt.encode({"id": user_id}, keys_storage.get_key(), algorithm="HS256").decode("utf-8")
+    now = datetime.datetime.utcnow()
+    payload = {
+        "id": user_id,
+        "iat": now,
+        "exp": now + datetime.timedelta(hours=24),
+        "iss": "wb-homeui-auth"
+    }
+    return jwt.encode(payload, keys_storage.get_key(), algorithm="HS256").decode("utf-8")

84-96: Simplify dictionary key checks.

Remove unnecessary .keys() calls when checking for key presence in dictionaries.

Apply this diff:

def validate_login_request(form: dict) -> None:
-    if "password" not in form.keys():
+    if "password" not in form:
        raise TypeError("No password field in POST arguments")

    if not isinstance(form.get("password"), str) or not form.get("password"):
        raise TypeError("Invalid password field in POST arguments")

-    if "login" not in form.keys():
+    if "login" not in form:
        raise TypeError("No login field in POST arguments")

    if not isinstance(form.get("login"), str) or not form.get("login"):
        raise TypeError("Invalid login field in POST arguments")
🧰 Tools
🪛 Ruff (0.8.2)

85-85: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


91-91: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


98-113: Simplify dictionary key checks.

Remove unnecessary .keys() calls when checking for key presence in dictionaries.

Apply this diff:

def validate_add_user_request(request: dict) -> None:
    if request.get("type") not in [e.value for e in UserType]:
        raise TypeError("Invalid type field in POST arguments")

-    if "password" not in request.keys():
+    if "password" not in request:
        raise TypeError("No password field in POST arguments")

    if not isinstance(request.get("password"), str) or not request.get("password"):
        raise TypeError("Invalid password field in POST arguments")

-    if "login" not in request.keys():
+    if "login" not in request:
        raise TypeError("No login field in POST arguments")

    if not isinstance(request.get("login"), str) or not request.get("login"):
        raise TypeError("Invalid login field in POST arguments")
🧰 Tools
🪛 Ruff (0.8.2)

102-102: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


108-108: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


405-425: Simplify error handling using contextlib.suppress.

Replace the try-except-pass block with contextlib.suppress for cleaner code.

Apply this diff:

+from contextlib import suppress

def main():
    parser = argparse.ArgumentParser(prog=argv[0], description="Home UI authentication service")
    parser.add_argument("--debug", action="store_true", help="Enable debug mode")
    parser.add_argument("--socket-file", default=DEFAULT_SOCKET_FILE, help="Socket file")
    parser.add_argument("--db-file", default=DEFAULT_DB_FILE, help="Database file path")
    args = parser.parse_args()

    con = open_db(args.db_file, DB_SCHEMA_VERSION)

-    try:
-        os.remove(args.socket_file)
-    except OSError:
-        pass
+    with suppress(OSError):
+        os.remove(args.socket_file)
    WebRequestHandler.users_storage = UsersStorage(con)
    WebRequestHandler.keys_storage = KeysStorage(con)
    WebRequestHandler.enable_debug = args.debug
    WebRequestHandler.sn = get_sn()
    server = UnixSocketHttpServer((args.socket_file), WebRequestHandler)
    os.chmod(args.socket_file, 0o662)
    server.serve_forever()
🧰 Tools
🪛 Ruff (0.8.2)

414-417: Use contextlib.suppress(OSError) instead of try-except-pass

Replace with contextlib.suppress(OSError)

(SIM105)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5807143 and 285e2a8.

📒 Files selected for processing (4)
  • .gitignore (1 hunks)
  • debian/changelog (1 hunks)
  • tests/main_test.py (1 hunks)
  • wb/homeui_auth/main.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • debian/changelog
  • .gitignore
🧰 Additional context used
🪛 Ruff (0.8.2)
wb/homeui_auth/main.py

85-85: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


91-91: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


102-102: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


108-108: Use key not in dict instead of key not in dict.keys()

Remove .keys()

(SIM118)


414-417: Use contextlib.suppress(OSError) instead of try-except-pass

Replace with contextlib.suppress(OSError)

(SIM105)

🔇 Additional comments (2)
tests/main_test.py (2)

103-152: LGTM!

The test suite is comprehensive and well-structured, covering all edge cases and error conditions for the authentication check handler.


154-183: LGTM!

The test suite thoroughly covers all scenarios for the who-am-i handler, including authenticated users, unauthenticated users, and cases with no configured users.

Comment on lines +255 to +268
def delete_user_handler(request: BaseHTTPRequestHandler, context: WebRequestHandlerContext) -> HttpResponse:
if not is_admin(context.user):
return response_403()
user_id = get_user_id_from_query(request)
if user_id is None:
return response_404()
user = context.users_storage.get_user_by_id(user_id)
if user is None:
return response_404()
if user.user_id == context.user.user_id:
return response_400("Can't delete yourself")
context.users_storage.delete_user(user_id)
return response_200()

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Prevent deletion of the last admin user.

Add a check to prevent deleting the last admin user to maintain system access.

Apply this diff:

def delete_user_handler(request: BaseHTTPRequestHandler, context: WebRequestHandlerContext) -> HttpResponse:
    if not is_admin(context.user):
        return response_403()
    user_id = get_user_id_from_query(request)
    if user_id is None:
        return response_404()
    user = context.users_storage.get_user_by_id(user_id)
    if user is None:
        return response_404()
    if user.user_id == context.user.user_id:
        return response_400("Can't delete yourself")
+    if user.type == UserType.ADMIN and context.users_storage.count_users_by_type(UserType.ADMIN) == 1:
+        return response_400("Can't delete the last admin")
    context.users_storage.delete_user(user_id)
    return response_200()
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def delete_user_handler(request: BaseHTTPRequestHandler, context: WebRequestHandlerContext) -> HttpResponse:
if not is_admin(context.user):
return response_403()
user_id = get_user_id_from_query(request)
if user_id is None:
return response_404()
user = context.users_storage.get_user_by_id(user_id)
if user is None:
return response_404()
if user.user_id == context.user.user_id:
return response_400("Can't delete yourself")
context.users_storage.delete_user(user_id)
return response_200()
def delete_user_handler(request: BaseHTTPRequestHandler, context: WebRequestHandlerContext) -> HttpResponse:
if not is_admin(context.user):
return response_403()
user_id = get_user_id_from_query(request)
if user_id is None:
return response_404()
user = context.users_storage.get_user_by_id(user_id)
if user is None:
return response_404()
if user.user_id == context.user.user_id:
return response_400("Can't delete yourself")
if user.type == UserType.ADMIN and context.users_storage.count_users_by_type(UserType.ADMIN) == 1:
return response_400("Can't delete the last admin")
context.users_storage.delete_user(user_id)
return response_200()

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.

4 participants