Bug fixes#833
Open
mshriver wants to merge 7 commits into
Open
Conversation
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Contributor
Reviewer's GuideRefines project-based access control for artifacts and imports, hardens project_has_user against None and invalid IDs, and ensures password recovery activation codes are stored as strings instead of bytes, with regression tests added for the new project_has_user behavior. Sequence diagram for updated artifact access controlsequenceDiagram
actor Client
participant ArtifactController as ArtifactController
participant DB as Database
participant Util as Util_projects
participant Artifact as Artifact
participant Result as Result
participant Run as Run
participant Project as Project
participant User as User
Client->>ArtifactController: view_artifact(id_, token_info, user)
ArtifactController->>DB: _build_artifact_response(id_)
DB-->>ArtifactController: artifact_or_error, response
alt artifact is not instance of Artifact
ArtifactController-->>Client: error_body, error_status
else artifact is Artifact
ArtifactController->>Util: _user_has_artifact_access(artifact, user)
alt artifact has result
Util->>Result: get project from artifact.result
Result-->>Util: project
Util->>Util: project_has_user(project, user)
else artifact has run
Util->>Run: get project from artifact.run
Run-->>Util: project
Util->>Util: project_has_user(project, user)
else artifact has neither result nor run
Util-->>ArtifactController: False
end
alt access denied
ArtifactController-->>Client: 403 FORBIDDEN
else access granted
ArtifactController-->>Client: artifact response (file or JSON)
end
end
Sequence diagram for password recovery activation code generationsequenceDiagram
actor UserActor as User
participant Client as Client
participant LoginController as LoginController
participant DB as Database
participant UserModel as UserModel
UserActor->>Client: Submit recover request (email)
Client->>LoginController: POST /recover (body)
LoginController->>DB: lookup user by email
DB-->>LoginController: UserModel or None
alt user not found
LoginController-->>Client: 400 BAD_REQUEST
else user found
LoginController->>LoginController: generate uuid4()
LoginController->>LoginController: urlsafe_b64encode(uuid_bytes).strip(b"=")
LoginController->>LoginController: decode() to str
LoginController->>UserModel: set activation_code (string)
LoginController->>DB: session.add(user)
LoginController->>DB: session.commit()
LoginController-->>Client: 201 CREATED, {}
end
Class diagram for updated artifact access and project_has_userclassDiagram
class Artifact {
+id
+result_id
+run_id
+filename
+result Result
+run Run
+to_dict()
}
class Result {
+id
+project Project
}
class Run {
+id
+project Project
}
class Project {
+id
+owner User
+users list~User~
}
class User {
+id
+email
+activation_code str
}
class ArtifactController {
+view_artifact(id_, token_info, user)
+download_artifact(id_, token_info, user)
+get_artifact(id_, token_info, user)
+get_artifact_list(page, page_size, filter_, sort, result_id, run_id, user)
+delete_artifact(id_, token_info, user)
+_build_artifact_response(id_)
}
class ImportController {
+add_import(body, token_info, user)
}
class LoginController {
+recover(body)
}
class UtilProjects {
+project_has_user(project, user) bool
+get_project(project_id_or_name) Project
+_user_has_artifact_access(artifact, user) bool
}
ArtifactController --> Artifact : uses
Artifact --> Result : optional
Artifact --> Run : optional
Result --> Project : belongs_to
Run --> Project : belongs_to
Project --> User : owner
Project "1" --> "*" User : users
ImportController --> UtilProjects : calls project_has_user
ArtifactController --> UtilProjects : calls _user_has_artifact_access
UtilProjects --> Project : queries
UtilProjects --> User : checks membership
LoginController --> User : sets activation_code
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The repeated access-control condition for artifacts (checking both result and run projects) appears three times; consider extracting this into a small helper (e.g.,
user_has_artifact_access(artifact, user)) to avoid duplication and keep the authorization logic consistent. - For artifacts that are not associated with either a result or a run, the new condition will skip the access check and implicitly allow access; if that is unintended, consider explicitly handling the "no project" case (e.g., by returning FORBIDDEN or NOT_FOUND).
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated access-control condition for artifacts (checking both result and run projects) appears three times; consider extracting this into a small helper (e.g., `user_has_artifact_access(artifact, user)`) to avoid duplication and keep the authorization logic consistent.
- For artifacts that are not associated with either a result or a run, the new condition will skip the access check and implicitly allow access; if that is unintended, consider explicitly handling the "no project" case (e.g., by returning FORBIDDEN or NOT_FOUND).
## Individual Comments
### Comment 1
<location path="backend/tests/test_util.py" line_range="563-566" />
<code_context>
assert result is expected_result
+def test_project_has_user_none_project(make_user):
+ """Test project_has_user returns False when project is None (data integrity issue)."""
+ user = make_user(email="user@test.com")
+ assert project_has_user(None, user) is False
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for `project_has_user` when a string project ID resolves to `None`
The updated logic also returns `False` when `project` is a string and `get_project(project)` returns `None` (e.g., invalid or deleted ID) instead of raising. Please add a regression test for this path, for example:
```python
def test_project_has_user_invalid_string_project_id(make_user):
user = make_user(email="user@test.com")
assert project_has_user("non-existent-project-id", user) is False
```
This will cover the string-ID case and protect against regressions in the `get_project`/`project_has_user` interaction.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: Claude <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR delivers several backend bug fixes related to project permission checks, artifact authorization behavior, and login recovery activation code handling.
Changes:
- Add a
Noneguard toproject_has_user()and a corresponding unit test. - Ensure
recover()storesactivation_codeas a string (not bytes). - Update artifact authorization checks to support artifacts attached to either a result or a run.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| backend/tests/test_util.py | Adds a unit test asserting project_has_user(None, user) returns False. |
| backend/ibutsu_server/util/projects.py | Returns False when the resolved project is None in project_has_user(). |
| backend/ibutsu_server/controllers/login_controller.py | Decodes generated recovery activation_code to str before persisting. |
| backend/ibutsu_server/controllers/artifact_controller.py | Expands authorization checks to handle run-attached artifacts (and avoid None dereferences). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (38.88%) is below the target coverage (85.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #833 +/- ##
==========================================
- Coverage 73.44% 73.42% -0.02%
==========================================
Files 154 154
Lines 7562 7572 +10
Branches 662 666 +4
==========================================
+ Hits 5554 5560 +6
- Misses 1788 1790 +2
- Partials 220 222 +2
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com>
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The repeated
isinstance(artifact, Artifact)+ early return pattern inview_artifactanddownload_artifactsuggests_build_artifact_response’s return contract is unclear; consider tightening that helper’s interface (e.g., always returning a validArtifactor raising/returning a standardized error type) so the controller functions can be simpler and not need type checks.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated `isinstance(artifact, Artifact)` + early return pattern in `view_artifact` and `download_artifact` suggests `_build_artifact_response`’s return contract is unclear; consider tightening that helper’s interface (e.g., always returning a valid `Artifact` or raising/returning a standardized error type) so the controller functions can be simpler and not need type checks.
## Individual Comments
### Comment 1
<location path="backend/ibutsu_server/controllers/artifact_controller.py" line_range="30-39" />
<code_context>
self.status = status
+def _user_has_artifact_access(artifact, user):
+ """Check whether the user has access to the artifact's project.
+
+ Access is determined by the project associated with the artifact's result or run.
+ If the artifact is not linked to either a result or a run, access is denied
+ to avoid silently granting access to orphaned artifacts.
+ """
+ if artifact.result:
+ return project_has_user(artifact.result.project, user)
+ if artifact.run:
+ return project_has_user(artifact.run.project, user)
+ return False
</code_context>
<issue_to_address>
**🚨 question (security):** Clarify behavior when an artifact is linked to both a result and a run with potentially different projects.
Because the helper prefers `artifact.result` over `artifact.run`, if both are set and belong to different projects, access is determined only by the result’s project. If that precedence isn’t intentional, consider either enforcing that both projects match or explicitly denying access when they differ to avoid authorization inconsistencies.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+30
to
+39
| def _user_has_artifact_access(artifact, user): | ||
| """Check whether the user has access to the artifact's project. | ||
|
|
||
| Access is determined by the project associated with the artifact's result or run. | ||
| If the artifact is not linked to either a result or a run, access is denied | ||
| to avoid silently granting access to orphaned artifacts. | ||
| """ | ||
| if artifact.result: | ||
| return project_has_user(artifact.result.project, user) | ||
| if artifact.run: |
Contributor
There was a problem hiding this comment.
🚨 question (security): Clarify behavior when an artifact is linked to both a result and a run with potentially different projects.
Because the helper prefers artifact.result over artifact.run, if both are set and belong to different projects, access is determined only by the result’s project. If that precedence isn’t intentional, consider either enforcing that both projects match or explicitly denying access when they differ to avoid authorization inconsistencies.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens backend authorization and robustness around project membership checks, artifact access control, and password recovery activation code storage.
Changes:
- Make
project_has_user()returnFalsewhen the project lookup yieldsNone, and add regression tests forNone/invalid project inputs. - Centralize artifact access authorization so artifact view/download/get/delete deny access unless the caller has project access via the associated result or run.
- Store password recovery activation codes as
str(notbytes) to avoid type mismatches during lookup.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
backend/tests/test_util.py |
Adds regression tests for project_has_user() handling of None/invalid project inputs. |
backend/ibutsu_server/util/projects.py |
Makes project_has_user() safely return False when project is None. |
backend/ibutsu_server/controllers/login_controller.py |
Ensures recovery activation codes are stored as strings (decoded base64). |
backend/ibutsu_server/controllers/import_controller.py |
Avoids redundant project lookup by passing project_obj into project_has_user(). |
backend/ibutsu_server/controllers/artifact_controller.py |
Adds centralized artifact access logic and applies it across artifact endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+87
to
88
| if not _user_has_artifact_access(artifact, user): | ||
| return HTTPStatus.FORBIDDEN.phrase, HTTPStatus.FORBIDDEN |
Comment on lines
+34
to
+40
| If the artifact is not linked to either a result or a run, access is denied | ||
| to avoid silently granting access to orphaned artifacts. | ||
| """ | ||
| if artifact.result: | ||
| return project_has_user(artifact.result.project, user) | ||
| if artifact.run: | ||
| return project_has_user(artifact.run.project, user) |
Comment on lines
25
to
28
| if isinstance(user, str): | ||
| user = db.session.get(User, user) | ||
| if user.is_superadmin: | ||
| return True |
Comment on lines
+570
to
+572
| """Test project_has_user returns False when a string project ID resolves to None.""" | ||
| user = make_user(email="user@test.com") | ||
| assert project_has_user("non-existent-project-id", user) is False |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Fix artifact and project access control edge cases and improve robustness around user/project lookups.
Bug Fixes:
Enhancements:
Tests: