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

feat: add iam roles filtering #5

Merged
merged 1 commit into from
Dec 27, 2023
Merged

feat: add iam roles filtering #5

merged 1 commit into from
Dec 27, 2023

Conversation

lirlia
Copy link
Owner

@lirlia lirlia commented Dec 27, 2023

  1. add filter role feature
  2. validation role is valid (in request / in judge process)

Summary by CodeRabbit

  • New Features

    • Launched a new admin page for managing IAM role filtering rules.
    • Introduced API methods for IAM role filtering operations.
    • Added functionality for adding, listing, and deleting IAM role filtering rules dynamically.
  • Improvements

    • Enhanced JSON handling for API requests and responses.
    • Refined API routing logic to accommodate new IAM role filtering endpoints.
    • Extended database schema with a new table for IAM role filtering rules.
  • Bug Fixes

    • Corrected API request and response encoding/decoding functions.
    • Addressed issues with IAM role filtering rules validation.
  • Documentation

    • Updated comments and internal documentation to improve clarity.
  • Testing

    • Added new integration tests for IAM role filtering.
    • Improved test utility functions and test suite setup.
  • User Interface

    • Implemented a search bar in the main app interface.
    • Updated the admin section navigation to include a link to the IAM Role Filtering page.
  • Database

    • Created new SQL queries for IAM role filtering rule management.
    • Defined a new table for storing IAM role filtering rules.

Copy link

coderabbitai bot commented Dec 27, 2023

Walkthrough

The recent updates focus on enhancing IAM role filtering capabilities. A new admin page for rule management has been created, alongside API client methods, request handlers, and response logic. The database schema now includes IAM role filtering rules, with corresponding repository and model layers handling rule operations. Integration and unit tests ensure the functionality's robustness, while the web template reflects these additions, offering an improved user interface.

Changes

File Path Change Summary
admin-page.html Introduced a management page for filtering rules.
api/prel_api/... Expanded the OpenAPI client with new methods for IAM role filtering rules and admin requests.
db/query.sql
db/schema.sql
Updated SQL queries and added a new iam_role_filtering_rules table.
internal/gateway/postgresql/...
Added new types and methods for IAM role filtering rule operations.
internal/handler/... Implemented new handler methods and import changes for IAM roles and filtering rules.
test/integration/...
test/testutil/server.go
Introduced integration tests and test helper modifications for IAM role filtering.
web/template/... Added new template constants and an HTML template for IAM role filtering management.

Assessment against linked issues

Objective Addressed Explanation
Admin Control over Visible IAM Roles for Users (#4)

"In the land of code and wire, 🐇

Changes hop, never tire. 🌟

Filters weave, roles align, 🎩

Admins cheer, all by design." 🎉

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 with CodeRabbit Bot (@coderabbitai)

  • You can directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit-tests for this file.
  • You can tag CodeRabbit on specific lines of code or entire files in the PR by tagging @coderabbitai in a comment. Examples:
    • @coderabbitai generate unit tests for this file.
    • @coderabbitai modularize this function.
  • You can tag @coderabbitai in a PR comment and ask questions about the PR and the codebase. Examples:
    • @coderabbitai generate interesting stats about this repository.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit tests.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid.
    • @coderabbitai read the files in the src/scheduler package and generate README in the markdown format.

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.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • 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/coderabbit-overrides.v2.json

CodeRabbit Discord Community

Join our Discord Community to get help, request features, and share feedback.

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.

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 4ab8bbb and d978ba4.
Files ignored due to filter (1)
  • api/api.yaml
Files selected for processing (37)
  • admin-page.html (1 hunks)
  • api/prel_api/oas_client_gen.go (4 hunks)
  • api/prel_api/oas_handlers_gen.go (2 hunks)
  • api/prel_api/oas_interfaces_gen.go (2 hunks)
  • api/prel_api/oas_json_gen.go (2 hunks)
  • api/prel_api/oas_parameters_gen.go (1 hunks)
  • api/prel_api/oas_request_decoders_gen.go (1 hunks)
  • api/prel_api/oas_request_encoders_gen.go (1 hunks)
  • api/prel_api/oas_response_decoders_gen.go (2 hunks)
  • api/prel_api/oas_response_encoders_gen.go (2 hunks)
  • api/prel_api/oas_router_gen.go (4 hunks)
  • api/prel_api/oas_schemas_gen.go (7 hunks)
  • api/prel_api/oas_server_gen.go (2 hunks)
  • api/prel_api/oas_unimplemented_gen.go (2 hunks)
  • api/prel_api/oas_validators_gen.go (1 hunks)
  • db/query.sql (1 hunks)
  • db/schema.sql (1 hunks)
  • internal/gateway/postgresql/models.gen.go (1 hunks)
  • internal/gateway/postgresql/query.sql.gen.go (3 hunks)
  • internal/gateway/repository/iam_role_filtering.go (1 hunks)
  • internal/handler/api_handler.go (2 hunks)
  • internal/handler/converter.go (1 hunks)
  • internal/handler/page_handler.go (1 hunks)
  • internal/model/iam_role_filtering.go (1 hunks)
  • internal/model/repository.go (1 hunks)
  • internal/usecase/iam_role.go (1 hunks)
  • internal/usecase/iam_role_test.go (1 hunks)
  • internal/usecase/usecase.go (2 hunks)
  • internal/usecase/usecase_suite_test.go (1 hunks)
  • scripts/insert-debug-query.sh (4 hunks)
  • test/integration/iam_role_filtering_test.go (1 hunks)
  • test/integration/iam_role_test.go (1 hunks)
  • test/testutil/clock.go (1 hunks)
  • test/testutil/server.go (2 hunks)
  • web/template/template.go (1 hunks)
  • web/template/templates/_header.tpl (1 hunks)
  • web/template/templates/admin_iam_role_filtering.tpl (1 hunks)
Files skipped from review due to trivial changes (2)
  • scripts/insert-debug-query.sh
  • test/testutil/clock.go
Files skipped from review as they are similar to previous changes (31)
  • admin-page.html
  • api/prel_api/oas_client_gen.go
  • api/prel_api/oas_interfaces_gen.go
  • api/prel_api/oas_request_decoders_gen.go
  • api/prel_api/oas_request_encoders_gen.go
  • api/prel_api/oas_response_decoders_gen.go
  • api/prel_api/oas_router_gen.go
  • api/prel_api/oas_schemas_gen.go
  • api/prel_api/oas_server_gen.go
  • api/prel_api/oas_unimplemented_gen.go
  • api/prel_api/oas_validators_gen.go
  • db/query.sql
  • db/schema.sql
  • internal/gateway/postgresql/models.gen.go
  • internal/gateway/postgresql/query.sql.gen.go
  • internal/gateway/repository/iam_role_filtering.go
  • internal/handler/api_handler.go
  • internal/handler/converter.go
  • internal/handler/page_handler.go
  • internal/model/iam_role_filtering.go
  • internal/model/repository.go
  • internal/usecase/iam_role.go
  • internal/usecase/iam_role_test.go
  • internal/usecase/usecase.go
  • internal/usecase/usecase_suite_test.go
  • test/integration/iam_role_filtering_test.go
  • test/integration/iam_role_test.go
  • test/testutil/server.go
  • web/template/template.go
  • web/template/templates/_header.tpl
  • web/template/templates/admin_iam_role_filtering.tpl
Additional comments: 21
api/prel_api/oas_parameters_gen.go (3)
  • 18-22: The addition of the APIIamRoleFilteringRulesRuleIDDeleteParams struct is consistent with the pattern used for other API operations in the file. It defines a single field RuleID for the path parameter.

  • 24-33: The unpackAPIIamRoleFilteringRulesRuleIDDeleteParams function correctly extracts the RuleID from the packed parameters. It follows the established pattern in the file.

  • 35-81: The decodeAPIIamRoleFilteringRulesRuleIDDeleteParams function properly decodes the RuleID from the path, handles URL escaping, and validates that the parameter is not empty. It uses the uri and conv packages for decoding and conversion, and returns a custom error type for decoding errors.

api/prel_api/oas_response_encoders_gen.go (4)
  • 87-153: The function encodeAPIIamRoleFilteringRulesPostResponse is consistent with the previous function in terms of structure and error handling. The same refinement suggested for error handling in encodeAPIIamRoleFilteringRulesGetResponse should be applied here for consistency and to handle client disconnections gracefully.
  • 155-226: The function encodeAPIIamRoleFilteringRulesRuleIDDeleteResponse follows the same pattern as the previous functions. It would benefit from the same refinement for error handling to manage client disconnections effectively.
  • 780-845: The function encodeAdminIamRoleFilteringGetResponse encodes the admin page response for IAM role filtering. It is consistent with the other functions in terms of structure and error handling. The same refinement for error handling should be applied here as well.
  • 777-849: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [228-846]

All functions for encoding API responses follow a consistent pattern and would benefit from refined error handling to address client disconnections. This change should be applied throughout the file to maintain consistency and improve robustness.

api/prel_api/oas_json_gen.go (9)
  • 17-21: The implementation of the Encode method for APIIamRoleFilteringRulesGetOK is standard and follows the expected pattern for JSON marshaling with the jx library.

  • 40-107: The Decode method for APIIamRoleFilteringRulesGetOK includes proper error handling and validation of required fields. However, ensure that the Decode method is called with a non-nil receiver to avoid potential nil pointer dereferences.

  • 123-127: The Encode method for APIIamRoleFilteringRulesPostOK is implemented correctly, mirroring the structure of the APIIamRoleFilteringRulesGetOK Encode method.

  • 142-201: The Decode method for APIIamRoleFilteringRulesPostOK is correct, with appropriate error wrapping and required field validation. The pattern of checking for a nil receiver is consistent across the methods.

  • 217-221: The Encode method for APIIamRoleFilteringRulesPostReq follows the established pattern for encoding JSON objects and is correctly implemented.

  • 236-297: The Decode method for APIIamRoleFilteringRulesPostReq correctly decodes the JSON object, including the required field validation logic.

  • 1302-1306: The Encode method for IamRoleFilteringRule is implemented correctly, with the encodeFields helper method modularizing the encoding process.

  • 1326-1399: The Decode method for IamRoleFilteringRule includes proper error handling and validation of required fields. The pattern of using a non-nil receiver is consistent and should be ensured when calling this method.

  • 14-315: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [17-1413]

The JSON marshaling and unmarshaling methods for the various structs in this file are implemented consistently and correctly. The use of the jx library is appropriate, and the error handling and validation logic are in line with best practices.

api/prel_api/oas_handlers_gen.go (5)
  • 22-154: The implementation of handleAPIIamRoleFilteringRulesGetRequest seems to follow the correct logic for handling a GET request. However, ensure that the security middleware (securityCookieAuth) is thoroughly tested to prevent unauthorized access.

  • 156-303: The handleAPIIamRoleFilteringRulesPostRequest function adds support for POST requests. It's important to verify that the input validation for the POST data is robust to prevent injection attacks or processing of invalid data.

  • 305-452: In handleAPIIamRoleFilteringRulesRuleIDDeleteRequest, ensure that the ruleID parameter is properly validated and sanitized before use to prevent path traversal or injection attacks.

  • 1684-1816: The handleAdminIamRoleFilteringGetRequest function is responsible for serving the IAM role filtering admin page. It is crucial to confirm that only authorized administrative users can access this endpoint.

  • 1681-1820: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [454-1817]

The general pattern for handling requests, including starting spans, recording metrics, and invoking middleware, is consistent across all functions. Ensure that the middleware and authentication checks are correctly implemented and tested to prevent security issues.

api/prel_api/oas_response_encoders_gen.go Show resolved Hide resolved
@lirlia lirlia merged commit 83642ec into main Dec 27, 2023
4 checks passed
@lirlia lirlia deleted the issues-4 branch December 27, 2023 18:35
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.

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 4ab8bbb and dda5c0e.
Files ignored due to filter (1)
  • api/api.yaml
Files selected for processing (39)
  • admin-page.html (1 hunks)
  • api/prel_api/oas_client_gen.go (4 hunks)
  • api/prel_api/oas_handlers_gen.go (2 hunks)
  • api/prel_api/oas_interfaces_gen.go (2 hunks)
  • api/prel_api/oas_json_gen.go (2 hunks)
  • api/prel_api/oas_parameters_gen.go (1 hunks)
  • api/prel_api/oas_request_decoders_gen.go (1 hunks)
  • api/prel_api/oas_request_encoders_gen.go (1 hunks)
  • api/prel_api/oas_response_decoders_gen.go (2 hunks)
  • api/prel_api/oas_response_encoders_gen.go (2 hunks)
  • api/prel_api/oas_router_gen.go (4 hunks)
  • api/prel_api/oas_schemas_gen.go (7 hunks)
  • api/prel_api/oas_server_gen.go (2 hunks)
  • api/prel_api/oas_unimplemented_gen.go (2 hunks)
  • api/prel_api/oas_validators_gen.go (1 hunks)
  • db/query.sql (1 hunks)
  • db/schema.sql (1 hunks)
  • internal/gateway/postgresql/models.gen.go (1 hunks)
  • internal/gateway/postgresql/query.sql.gen.go (3 hunks)
  • internal/gateway/repository/iam_role_filtering.go (1 hunks)
  • internal/handler/api_handler.go (2 hunks)
  • internal/handler/converter.go (1 hunks)
  • internal/handler/page_handler.go (1 hunks)
  • internal/model/iam_role_filtering.go (1 hunks)
  • internal/model/repository.go (1 hunks)
  • internal/usecase/iam_role.go (1 hunks)
  • internal/usecase/iam_role_test.go (1 hunks)
  • internal/usecase/request.go (4 hunks)
  • internal/usecase/usecase.go (2 hunks)
  • internal/usecase/usecase_suite_test.go (1 hunks)
  • scripts/insert-debug-query.sh (4 hunks)
  • test/integration/iam_role_filtering_test.go (1 hunks)
  • test/integration/iam_role_test.go (1 hunks)
  • test/integration/request_test.go (4 hunks)
  • test/testutil/clock.go (1 hunks)
  • test/testutil/server.go (2 hunks)
  • web/template/template.go (1 hunks)
  • web/template/templates/_header.tpl (1 hunks)
  • web/template/templates/admin_iam_role_filtering.tpl (1 hunks)
Files skipped from review as they are similar to previous changes (34)
  • admin-page.html
  • api/prel_api/oas_client_gen.go
  • api/prel_api/oas_interfaces_gen.go
  • api/prel_api/oas_parameters_gen.go
  • api/prel_api/oas_request_decoders_gen.go
  • api/prel_api/oas_request_encoders_gen.go
  • api/prel_api/oas_response_decoders_gen.go
  • api/prel_api/oas_response_encoders_gen.go
  • api/prel_api/oas_router_gen.go
  • api/prel_api/oas_schemas_gen.go
  • api/prel_api/oas_server_gen.go
  • api/prel_api/oas_unimplemented_gen.go
  • api/prel_api/oas_validators_gen.go
  • db/query.sql
  • db/schema.sql
  • internal/gateway/postgresql/models.gen.go
  • internal/gateway/postgresql/query.sql.gen.go
  • internal/gateway/repository/iam_role_filtering.go
  • internal/handler/api_handler.go
  • internal/handler/page_handler.go
  • internal/model/iam_role_filtering.go
  • internal/model/repository.go
  • internal/usecase/iam_role.go
  • internal/usecase/iam_role_test.go
  • internal/usecase/usecase.go
  • internal/usecase/usecase_suite_test.go
  • scripts/insert-debug-query.sh
  • test/integration/iam_role_filtering_test.go
  • test/integration/iam_role_test.go
  • test/testutil/clock.go
  • test/testutil/server.go
  • web/template/template.go
  • web/template/templates/_header.tpl
  • web/template/templates/admin_iam_role_filtering.tpl
Additional comments: 23
internal/handler/converter.go (2)
  • 154-158: The function convertIamRoleFilteringRule correctly converts a model.IamRoleFilteringRule to an api.IamRoleFilteringRule. The fields are appropriately mapped, and the function is straightforward.

  • 161-166: The function convertIamRoleFilteringRules correctly iterates over a slice of model.IamRoleFilteringRules and converts each element using the convertIamRoleFilteringRule function. The use of make with a capacity hint is a good practice for performance.

internal/usecase/request.go (3)
  • 50-66: The validateRoles function checks if the provided roles are within the set of roles returned by GetIamRoles. The use of the slices package for the Contains method is appropriate. However, it's important to ensure that GetIamRoles returns a list of role names that are expected to be in the same format as the roles provided to validateRoles.
Verification successful

The GetIamRoles function in iam_role.go retrieves IAM roles, sorts them, and potentially filters them based on rule patterns. It returns the names of the roles, which are then used in the validateRoles function to check against the provided roles. The roles are returned as a sorted slice of strings, which is compatible with the validateRoles function's expectations.

  • The roles are obtained from a call to uc.c.GetIamRoles(now, projectID, user), which suggests that the roles are associated with a user and a project.
  • The roles are then appended to a slice roleIDs after accessing the Name field of each role.
  • The roleIDs slice is sorted, which would facilitate any operations that rely on order, such as binary search, though this is not directly relevant to the validateRoles function.
  • The function also involves a transaction that retrieves IAM role filtering rules and applies them to the roleIDs.
  • The final list of role names is either the filtered list or the original roleIDs if no filtering rules are applied.

Given this information, it appears that the GetIamRoles function returns role names in a format that should be directly comparable to the roles provided to validateRoles, as it uses the Name field of the roles, which is likely the expected identifier for comparison.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Verify that GetIamRoles returns role names in the expected format.
ast-grep --lang go --pattern $'func ($_ *Usecase) GetIamRoles($_ context.Context, $_ string) ([]string, error) {
  $$$
}'

Length of output: 2411

* 78-81: The call to `validateRoles` within `CreateRequest` is correctly placed within the transaction. This ensures that the role validation is part of the atomic operation, which is crucial for maintaining data integrity.
  • 119-122: The call to validateRoles within JudgeRequest is also correctly placed within the transaction. This ensures that the request is not judged unless the roles are valid, which is a good security practice.
test/integration/request_test.go (1)
  • 32-39: The mock setup for QueryGrantableRoles is correctly configured to return a fixed set of roles. This setup is used across multiple tests, so using AnyTimes() is appropriate to avoid repeating the setup.
api/prel_api/oas_json_gen.go (13)
  • 17-34: The implementation of Encode and encodeFields methods for APIIamRoleFilteringRulesGetOK struct is correct and follows the standard pattern for JSON encoding in Go using the jx library.

  • 40-107: The Decode method for APIIamRoleFilteringRulesGetOK struct properly checks for nil receiver, iterates over the object fields, and uses a bit set to track required fields. However, ensure that the IamRoleFilteringRules slice is not being reset unintentionally if it's meant to be appended to in other parts of the code.

  • 110-121: The MarshalJSON and UnmarshalJSON methods for APIIamRoleFilteringRulesGetOK struct are correctly implemented as wrappers around the Encode and Decode methods.

  • 123-136: The Encode method for APIIamRoleFilteringRulesPostOK struct is implemented correctly, following the established pattern for JSON encoding.

  • 142-201: The Decode method for APIIamRoleFilteringRulesPostOK struct is implemented correctly. It checks for nil receiver, iterates over the object fields, and uses a bit set to track required fields. Ensure that the IamRoleFilteringRule field is intended to be overwritten and not appended to.

  • 204-215: The MarshalJSON and UnmarshalJSON methods for APIIamRoleFilteringRulesPostOK struct are correctly implemented as wrappers around the Encode and Decode methods.

  • 217-230: The Encode method for APIIamRoleFilteringRulesPostReq struct is implemented correctly, following the established pattern for JSON encoding.

  • 236-297: The Decode method for APIIamRoleFilteringRulesPostReq struct is implemented correctly. It checks for nil receiver, iterates over the object fields, and uses a bit set to track required fields. Ensure that the Pattern field is intended to be overwritten and not appended to.

  • 300-311: The MarshalJSON and UnmarshalJSON methods for APIIamRoleFilteringRulesPostReq struct are correctly implemented as wrappers around the Encode and Decode methods.

  • 1302-1319: The Encode method for IamRoleFilteringRule struct is implemented correctly, following the established pattern for JSON encoding.

  • 1326-1399: The Decode method for IamRoleFilteringRule struct is implemented correctly. It checks for nil receiver, iterates over the object fields, and uses a bit set to track required fields. Ensure that the ID and Pattern fields are intended to be overwritten and not appended to.

  • 1402-1413: The MarshalJSON and UnmarshalJSON methods for IamRoleFilteringRule struct are correctly implemented as wrappers around the Encode and Decode methods.

  • 14-315: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [14-1413]

All other JSON marshaling and unmarshaling methods for the various structs in this file follow the established patterns and are correctly implemented. Ensure that all fields that are overwritten during the Decode process are intended to behave as such and are not meant to be appended to existing data.

api/prel_api/oas_handlers_gen.go (4)
  • 22-154: The implementation of handleAPIIamRoleFilteringRulesGetRequest seems to follow the standard pattern for handling a GET request in this codebase. It includes telemetry, security checks, error handling, and response encoding. Ensure that the security checks (securityCookieAuth) and the actual business logic call (APIIamRoleFilteringRulesGet) are correctly implemented and tested.

  • 156-303: The handleAPIIamRoleFilteringRulesPostRequest function follows a similar pattern to the GET request handler. It includes decoding the request, running security checks, invoking the business logic, and encoding the response. Verify that the request decoding (decodeAPIIamRoleFilteringRulesPostRequest) and the business logic (APIIamRoleFilteringRulesPost) are correctly implemented and tested.

  • 305-452: The handleAPIIamRoleFilteringRulesRuleIDDeleteRequest function is responsible for handling DELETE requests. It includes parameter decoding, security checks, business logic invocation, and response encoding. Verify that the parameter decoding (decodeAPIIamRoleFilteringRulesRuleIDDeleteParams) and the business logic (APIIamRoleFilteringRulesRuleIDDelete) are correctly implemented and tested.

  • 1684-1816: The handleAdminIamRoleFilteringGetRequest function is designed to handle GET requests for the admin IAM role filtering page. It follows the established pattern for request handling, including security checks and response encoding. Verify that the business logic (AdminIamRoleFilteringGet) is correctly implemented and tested, and ensure that the admin page is properly secured and only accessible to authorized users.

Comment on lines +108 to +127
Context("when request' role is not permitted", func() {
It("should return error", func() {
res, err := helper.ApiClient.APIRequestsPost(ctx, &api.APIRequestsPostReq{
ProjectID: "project-id",
IamRoles: []string{
"roles/bigquery.admin",
},
Period: api.APIRequestsPostReqPeriod10,
Reason: "this is a test",
})

Expect(err).NotTo(HaveOccurred())
Expect(res).NotTo(BeNil())
Expect(res.(*api.BadRequest)).NotTo(BeNil())

b, err := io.ReadAll(res.(*api.BadRequest).Data)
Expect(err).NotTo(HaveOccurred())
Expect(string(b)).To(ContainSubstring("Invalid Argument"))
})
})
Copy link

Choose a reason for hiding this comment

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

The new test case "when request' role is not permitted" should verify that the error returned when an invalid role is used is as expected. However, the test expects a BadRequest response, which is not consistent with the expected behavior of returning an error when roles are not permitted. The test should assert the specific error type and message related to invalid roles.

- Expect(res.(*api.BadRequest)).NotTo(BeNil())
+ Expect(res).To(BeAssignableToTypeOf(api.ErrorResponse{}))
+ Expect(res.(api.ErrorResponse).GetMessage()).To(ContainSubstring("invalid iam role"))

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.

Suggested change
Context("when request' role is not permitted", func() {
It("should return error", func() {
res, err := helper.ApiClient.APIRequestsPost(ctx, &api.APIRequestsPostReq{
ProjectID: "project-id",
IamRoles: []string{
"roles/bigquery.admin",
},
Period: api.APIRequestsPostReqPeriod10,
Reason: "this is a test",
})
Expect(err).NotTo(HaveOccurred())
Expect(res).NotTo(BeNil())
Expect(res.(*api.BadRequest)).NotTo(BeNil())
b, err := io.ReadAll(res.(*api.BadRequest).Data)
Expect(err).NotTo(HaveOccurred())
Expect(string(b)).To(ContainSubstring("Invalid Argument"))
})
})
Context("when request' role is not permitted", func() {
It("should return error", func() {
res, err := helper.ApiClient.APIRequestsPost(ctx, &api.APIRequestsPostReq{
ProjectID: "project-id",
IamRoles: []string{
"roles/bigquery.admin",
},
Period: api.APIRequestsPostReqPeriod10,
Reason: "this is a test",
})
Expect(err).NotTo(HaveOccurred())
Expect(res).NotTo(BeNil())
Expect(res).To(BeAssignableToTypeOf(api.ErrorResponse{}))
Expect(res.(api.ErrorResponse).GetMessage()).To(ContainSubstring("invalid iam role"))
b, err := io.ReadAll(res.(*api.BadRequest).Data)
Expect(err).NotTo(HaveOccurred())
Expect(string(b)).To(ContainSubstring("Invalid Argument"))
})
})

Comment on lines +263 to +281
Context("when request has invalid role", func() {
It("should return error", func() {
newReq := testutil.NewRequest(testutil.WithRequesterUserID(helper.User.ID()), testutil.WithIamRoles([]string{"invalid-role"}))
Expect(helper.RequestRepo.Save(ctx, newReq)).To(Succeed())

res, err := judgerClient.APIRequestsRequestIDPatch(ctx, &api.APIRequestsRequestIDPatchReq{
Status: api.JudgeStatusApprove,
}, api.APIRequestsRequestIDPatchParams{
RequestID: newReq.ID(),
})

Expect(err).NotTo(HaveOccurred())
Expect(res).NotTo(BeNil())

b, err := io.ReadAll(res.(*api.BadRequest).Data)
Expect(err).NotTo(HaveOccurred())
Expect(string(b)).To(ContainSubstring("Invalid Argument"))
})
})
Copy link

Choose a reason for hiding this comment

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

The test case "when request has invalid role" is similar to the previous one and should also assert the specific error type and message related to invalid roles. The same issue applies here.

- Expect(res.(*api.BadRequest)).NotTo(BeNil())
+ Expect(res).To(BeAssignableToTypeOf(api.ErrorResponse{}))
+ Expect(res.(api.ErrorResponse).GetMessage()).To(ContainSubstring("invalid iam role"))

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.

Suggested change
Context("when request has invalid role", func() {
It("should return error", func() {
newReq := testutil.NewRequest(testutil.WithRequesterUserID(helper.User.ID()), testutil.WithIamRoles([]string{"invalid-role"}))
Expect(helper.RequestRepo.Save(ctx, newReq)).To(Succeed())
res, err := judgerClient.APIRequestsRequestIDPatch(ctx, &api.APIRequestsRequestIDPatchReq{
Status: api.JudgeStatusApprove,
}, api.APIRequestsRequestIDPatchParams{
RequestID: newReq.ID(),
})
Expect(err).NotTo(HaveOccurred())
Expect(res).NotTo(BeNil())
b, err := io.ReadAll(res.(*api.BadRequest).Data)
Expect(err).NotTo(HaveOccurred())
Expect(string(b)).To(ContainSubstring("Invalid Argument"))
})
})
Context("when request has invalid role", func() {
It("should return error", func() {
newReq := testutil.NewRequest(testutil.WithRequesterUserID(helper.User.ID()), testutil.WithIamRoles([]string{"invalid-role"}))
Expect(helper.RequestRepo.Save(ctx, newReq)).To(Succeed())
res, err := judgerClient.APIRequestsRequestIDPatch(ctx, &api.APIRequestsRequestIDPatchReq{
Status: api.JudgeStatusApprove,
}, api.APIRequestsRequestIDPatchParams{
RequestID: newReq.ID(),
})
Expect(err).NotTo(HaveOccurred())
Expect(res).To(BeAssignableToTypeOf(api.ErrorResponse{}))
Expect(res.(api.ErrorResponse).GetMessage()).To(ContainSubstring("invalid iam role"))
})
})

@lirlia lirlia added the minor minor release label Dec 30, 2023
@github-actions github-actions bot mentioned this pull request Dec 30, 2023
@prel-bot prel-bot bot mentioned this pull request Dec 30, 2023
@coderabbitai coderabbitai bot mentioned this pull request Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Suggestion: Admin Control over Visible IAM Roles for Users
1 participant