Add EC2 autograder features #2281
Conversation
…and disable access key fields if not enabled
This reverts commit fc35ace.
📝 WalkthroughWalkthroughAdds EC2 SSH support for autograders: DB migrations and schema updates, encrypted AWS credential fields and validations, controller param handling, helper job payload changes to include EC2 options, new view partials and JS for EC2 settings, and environment feature flags. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,230,255,0.18)
participant U as User
participant B as Browser/DOM
participant JS as autograder.js
end
U->>B: Load autograder settings page
B->>JS: document.ready
JS->>B: Read #autograder_use_access_key checkbox
JS->>B: Enable/disable and clear `#autograder_access_key` & `#autograder_access_key_id`
U->>B: Toggle checkbox
B->>JS: change event
JS->>B: Update input fields accordingly
sequenceDiagram
participant Ctrl as AutogradersController
participant M as Autograder (model)
participant H as AssessmentAutogradeCore
participant Cfg as Rails.config.x.ec2_ssh
participant T as TangoClient
Ctrl->>M: build/create with params (instance_type, use_access_key, access_key*, access_key_id*)
Ctrl->>M: on update, clear plaintext secrets if use_access_key unset
Ctrl->>H: tango_add_job(..., @autograde_prop, ...)
H->>Cfg: check ec2_ssh
alt ec2_ssh enabled
H->>M: include instanceType from @autograde_prop
opt use_access_key true
H->>M: include accessKey and accessKeyId in job props
end
H->>T: submit job with ec2Vmms=true and instanceType
else
H->>T: submit job with existing non-EC2 properties
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb (1)
1-7:❓ Verification inconclusive
Consider encryption for sensitive AWS credentials.
The migration adds string columns for AWS credentials (access_key and access_key_id). Consider using encryption or a secure credentials management strategy to avoid storing sensitive AWS credentials in plaintext.
Consider using Rails' built-in encryption capabilities or a gem like attr_encrypted:
- add_column :autograders, :access_key, :string, default: "" - add_column :autograders, :access_key_id, :string, default: "" + add_column :autograders, :encrypted_access_key, :string, default: "" + add_column :autograders, :encrypted_access_key_iv, :string, default: "" + add_column :autograders, :encrypted_access_key_id, :string, default: "" + add_column :autograders, :encrypted_access_key_id_iv, :string, default: ""With model changes:
class Autograder < ApplicationRecord attr_encrypted :access_key, key: Rails.application.credentials.secret_key_base[0..31] attr_encrypted :access_key_id, key: Rails.application.credentials.secret_key_base[0..31] endLet's check if similar sensitive data is encrypted elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Search for encryption patterns in models echo "Searching for encryption patterns in models..." rg -l "attr_encrypted" --type ruby app/models/ rg -l "encrypted_" --type ruby app/models/Length of output: 192
Encryption for AWS Credentials: Manual Verification Required
After searching for encryption usage in the models (using patterns like
"attr_encrypted"and"encrypted_"), no evidence was found that AWS credentials are currently secured. This suggests the migration is adding plaintext credentials. It’s recommended to update the migration and the related model as follows:
Migration Update: Replace the plaintext columns with encrypted counterparts:
- add_column :autograders, :access_key, :string, default: "" - add_column :autograders, :access_key_id, :string, default: "" + add_column :autograders, :encrypted_access_key, :string, default: "" + add_column :autograders, :encrypted_access_key_iv, :string, default: "" + add_column :autograders, :encrypted_access_key_id, :string, default: "" + add_column :autograders, :encrypted_access_key_id_iv, :string, default: ""Model Update: Incorporate encryption using Rails’ built-in capabilities or a gem like
attr_encrypted:class Autograder < ApplicationRecord attr_encrypted :access_key, key: Rails.application.credentials.secret_key_base[0..31] attr_encrypted :access_key_id, key: Rails.application.credentials.secret_key_base[0..31] endPlease manually verify that no other parts of the codebase secure similar sensitive data before proceeding.
app/views/autograders/_ec2_settings.html.erb (1)
15-17: Fix typo in confirmation messageThere's a typo in the delete confirmation message.
<%= link_to "Delete Autograder", course_assessment_autograder_path(@course, @assessment), method: :delete, class: "btn danger", - data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %> + data: { confirm: "Are you sure you want to delete the Autograder for this assessment?" } %>app/views/autograders/_basic_settings.html.erb (1)
26-28: Confirm deletion text contains a typo.There's a typo in the delete confirmation message.
- data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %> + data: { confirm: "Are you sure you want to delete the Autograder for this assessment?" } %>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
app/assets/javascripts/autograder.js(1 hunks)app/controllers/autograders_controller.rb(2 hunks)app/helpers/assessment_autograde_core.rb(1 hunks)app/views/autograders/_basic_settings.html.erb(1 hunks)app/views/autograders/_ec2_settings.html.erb(1 hunks)app/views/autograders/_form.html.erb(1 hunks)config/environments/development.rb(1 hunks)config/environments/production.rb.template(1 hunks)db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb(1 hunks)db/migrate/20241211042124_add_use_access_key_to_autograder.rb(1 hunks)db/schema.rb(10 hunks)
🧰 Additional context used
🪛 RuboCop (1.73)
db/schema.rb
[convention] 14-15: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
🪛 GitHub Actions: Ruby on Rails CI
db/schema.rb
[error] 443-443: ActiveRecord::MismatchedForeignKey: Column blob_id on table active_storage_attachments does not match column id on active_storage_blobs, which has type bigint. To resolve this issue, change the type of the blob_id column on active_storage_attachments to be :bigint.
🔇 Additional comments (16)
db/migrate/20241211042124_add_use_access_key_to_autograder.rb (1)
1-5: LGTM! Migration is well structured.The migration adds a boolean column with an appropriate default value of false, which is good for backward compatibility.
config/environments/development.rb (1)
77-78: Feature flag looks good.The EC2 autograder feature flag follows the established pattern in the file and is well positioned with other feature flags.
app/assets/javascripts/autograder.js (1)
15-17: Good practice with immediate initialization.Good approach to call the callback immediately after binding to ensure the initial state of the fields is correct based on the checkbox.
config/environments/production.rb.template (1)
91-93: Feature flag addition looks goodThe new feature flag
config.x.ec2_sshis added for controlling the EC2 autograder functionality. The flag follows the existing pattern of feature flags in the codebase and is set tofalseby default, which is appropriate for a template file.app/views/autograders/_ec2_settings.html.erb (1)
1-3: JavaScript inclusion looks goodThe JavaScript inclusion for the autograder functionality is appropriately placed in the content_for block for javascripts.
app/controllers/autograders_controller.rb (1)
19-21: Initialize EC2-specific properties correctlyGood addition of EC2-specific properties with sensible defaults. The "t2.micro" choice for the default instance type is appropriate as it's in the AWS free tier.
app/views/autograders/_form.html.erb (2)
1-17: Tabbed interface implementation looks goodThe implementation of a tabbed interface for the autograder settings is a nice UI improvement. The conditional rendering of the EC2 tab based on the feature flag is properly implemented.
19-34: Form structure and partial rendering is well organizedThe form structure with partials for different setting categories is well organized and follows good separation of concerns principles.
app/views/autograders/_basic_settings.html.erb (3)
1-4: Good VM image input implementation with helpful context links.The text field for VM image includes contextual help with links to view existing VM images and optionally upload Docker images. This improves user experience by providing direct access to related functionality.
6-10: Well-constrained timeout input with appropriate help text.The number field for timeout has appropriate min/max constraints (10-900 seconds) with clear help text explaining the limits. The release scores checkbox includes a helpful recommendation.
12-18: Clear file upload interface with existing file indication.The file upload implementation correctly indicates when files already exist and will be overridden. The help block about file renaming helps set appropriate user expectations.
app/helpers/assessment_autograde_core.rb (2)
182-182: Job properties serialization moved after EC2 properties are added.The job_properties are correctly converted to JSON after all properties have been added, including the EC2-specific ones.
169-181:✅ Verification successful
Well-implemented conditional logic for EC2 configuration.
The implementation correctly checks for the presence of the EC2 SSH configuration flag before adding EC2-specific properties to the job. The conditional handling of access credentials based on the
use_access_key?method is a good practice, providing empty strings as fallbacks when custom credentials aren't used.
🏁 Script executed:
#!/bin/bash # Description: Verify that the EC2 configuration is correctly set in the application configuration # Check if EC2 SSH configuration is present in the application's configuration files echo "Checking for EC2 SSH configuration..." grep -r "config.x.ec2_ssh" --include="*.rb" config/ # Check for any related EC2 configuration in the application echo "Checking for any other EC2-related configurations..." grep -r "ec2" --include="*.rb" --include="*.yml" config/Length of output: 611
EC2 Configuration Condition Verified
The conditional logic in
app/helpers/assessment_autograde_core.rbis correctly implemented. Verification of the configuration (withconfig.x.ec2_sshset inconfig/environments/development.rb) confirms that EC2-specific properties are only added when expected. The handling of access keys—using values from@autograde_propwhen available and falling back to empty strings otherwise—is appropriate.db/schema.rb (3)
152-155: New autograders table fields for EC2 support.The schema includes new fields in the autograders table to support EC2 configuration:
instance_type: For specifying the EC2 instance typeaccess_keyandaccess_key_id: For AWS authenticationuse_access_key: Boolean flag to determine whether custom keys should be usedThese fields align well with the EC2 feature implementation in the controller code.
13-13: Schema version updated.The schema version has been updated to reflect the new migrations that add EC2 support.
445-448: New foreign key constraints added.The schema adds foreign key constraints for GitHub integrations and OAuth-related tables, improving database integrity.
app/assets/javascripts/autograder.js
Outdated
| ;(function() { | ||
| $(document).ready(function () { | ||
| function access_key_callback() { | ||
| const checked = $(this).prop('checked'); | ||
| const $access_key_field = $('#autograder_access_key'); | ||
| const $access_key_id_field = $('#autograder_access_key_id'); | ||
| $access_key_field.prop('disabled', !checked); | ||
| $access_key_id_field.prop('disabled', !checked); | ||
| if (!checked) { | ||
| $access_key_field.val('', checked); | ||
| $access_key_id_field.val('', checked); | ||
| } | ||
| } | ||
|
|
||
| $('#autograder_use_access_key').on('change', access_key_callback); | ||
| access_key_callback.call($('#autograder_use_access_key')); | ||
| }); | ||
| })(); No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix val() method usage and consider additional validation.
The JavaScript function properly manages the state of the input fields based on the checkbox, but there's an issue with the val() method usage.
- The val() method doesn't take a second argument. Fix lines 10-11:
- $access_key_field.val('', checked);
- $access_key_id_field.val('', checked);
+ $access_key_field.val('');
+ $access_key_id_field.val('');- Consider adding validation for the AWS credentials format when enabled to provide immediate feedback to users.
📝 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.
| ;(function() { | |
| $(document).ready(function () { | |
| function access_key_callback() { | |
| const checked = $(this).prop('checked'); | |
| const $access_key_field = $('#autograder_access_key'); | |
| const $access_key_id_field = $('#autograder_access_key_id'); | |
| $access_key_field.prop('disabled', !checked); | |
| $access_key_id_field.prop('disabled', !checked); | |
| if (!checked) { | |
| $access_key_field.val('', checked); | |
| $access_key_id_field.val('', checked); | |
| } | |
| } | |
| $('#autograder_use_access_key').on('change', access_key_callback); | |
| access_key_callback.call($('#autograder_use_access_key')); | |
| }); | |
| })(); | |
| ;(function() { | |
| $(document).ready(function () { | |
| function access_key_callback() { | |
| const checked = $(this).prop('checked'); | |
| const $access_key_field = $('#autograder_access_key'); | |
| const $access_key_id_field = $('#autograder_access_key_id'); | |
| $access_key_field.prop('disabled', !checked); | |
| $access_key_id_field.prop('disabled', !checked); | |
| if (!checked) { | |
| $access_key_field.val(''); | |
| $access_key_id_field.val(''); | |
| } | |
| } | |
| $('#autograder_use_access_key').on('change', access_key_callback); | |
| access_key_callback.call($('#autograder_use_access_key')); | |
| }); | |
| })(); |
| params[:autograder].permit(:autograde_timeout, :autograde_image, :release_score, :access_key, | ||
| :access_key_id, :instance_type) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure proper validation for EC2 parameters
The controller now correctly permits the new EC2 parameters. However, consider adding validation for the instance type and secure handling of AWS credentials.
🏁 Script executed:
#!/bin/bash
# Check if there's validation for instance type and access keys in the autograder model
# Look for validation in the autograder model
echo "Checking for validations in the Autograder model:"
rg -A 5 "validates.*instance_type" app/models/
rg -A 5 "validates.*access_key" app/models/
# Check if there's any encryption for the AWS credentials
echo "Checking for potential credential encryption:"
rg -A 5 "encrypt.*access_key" app/models/Length of output: 348
Action Required: Enforce Validations on EC2 Parameters & Secure AWS Credentials
The controller now permits the new EC2 parameters, but our search in the Autograder model did not reveal any validations for instance_type or any encryption for AWS credentials. Please ensure that:
- The model includes proper validations for
instance_type(e.g., checking that it matches a set of allowed values). - AWS credentials such as
access_key(and potentiallyaccess_key_id) are processed securely (e.g., through encryption or another secure handling mechanism).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
db/schema.rb (1)
13-451: 🛠️ Refactor suggestionConsider encrypting sensitive AWS credentials
While the schema changes look good, AWS access keys are sensitive credentials that should be encrypted when stored in the database. Consider using Rails' built-in encryption capabilities or a gem like attr_encrypted to protect these values.
You could implement encryption for these fields by:
- Adding encrypted_attribute columns instead of plaintext
- Using Rails' Active Record Encryption (for Rails 7+) or a gem like attr_encrypted
- Ensuring the keys are properly rotated and secured
This would significantly improve the security posture of your application when handling AWS credentials.
🧰 Tools
🪛 RuboCop (1.73)
[convention] 14-15: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
♻️ Duplicate comments (1)
app/assets/javascripts/autograder.js (1)
10-11:⚠️ Potential issueFix val() method usage
The jQuery val() method doesn't accept a second argument. It should be called without the second parameter.
- $access_key_field.val('', checked); - $access_key_id_field.val('', checked); + $access_key_field.val(''); + $access_key_id_field.val('');
🧹 Nitpick comments (6)
app/views/autograders/_ec2_settings.html.erb (4)
25-25: Remove trailing whitespaceThere is trailing whitespace at the end of this line.
- +🧰 Tools
🪛 GitHub Actions: Ruby on Rails CI
[warning] 25-25: Extra whitespace detected at end of line.
33-33: Remove trailing whitespaceThere is trailing whitespace at the end of this line.
- +🧰 Tools
🪛 GitHub Actions: Ruby on Rails CI
[warning] 33-33: Extra whitespace detected at end of line.
40-40: Remove trailing whitespaceThere is trailing whitespace at the end of this line.
- +🧰 Tools
🪛 GitHub Actions: Ruby on Rails CI
[warning] 40-40: Extra whitespace detected at end of line.
1-87: Add trailing newline at the end of the fileFiles should end with a newline character.
method: :delete, class: "btn danger", data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %> +🧰 Tools
🪛 GitHub Actions: Ruby on Rails CI
[warning] 25-25: Extra whitespace detected at end of line.
[warning] 33-33: Extra whitespace detected at end of line.
[warning] 40-40: Extra whitespace detected at end of line.
[warning] 87-87: Missing a trailing newline at the end of the file.
app/assets/javascripts/autograder.js (1)
32-35: Remove console.log in production codeConsole logs should be removed from production code. They may expose sensitive information and have no value in a production environment.
$('#autograder_instance_type').on('change.ec2-instance', function() { const selectedInstance = $(this).val(); - console.log('Selected EC2 instance type:', selectedInstance); - }); + });db/schema.rb (1)
14-15: Remove extra empty lineThere's an extra empty line at the beginning of the block body that should be removed according to RuboCop.
ActiveRecord::Schema.define(version: 2024_12_11_042124) do - create_table "active_storage_attachments", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|🧰 Tools
🪛 RuboCop (1.73)
[convention] 14-15: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/assets/javascripts/autograder.js(1 hunks)app/views/autograders/_ec2_settings.html.erb(1 hunks)db/schema.rb(23 hunks)
🧰 Additional context used
🪛 GitHub Actions: Ruby on Rails CI
app/views/autograders/_ec2_settings.html.erb
[warning] 25-25: Extra whitespace detected at end of line.
[warning] 33-33: Extra whitespace detected at end of line.
[warning] 40-40: Extra whitespace detected at end of line.
[warning] 87-87: Missing a trailing newline at the end of the file.
🪛 RuboCop (1.73)
db/schema.rb
[convention] 14-15: Extra empty line detected at block body beginning.
(Layout/EmptyLinesAroundBlockBody)
🔇 Additional comments (5)
app/views/autograders/_ec2_settings.html.erb (2)
9-10: Sensitive credentials should use password fieldsThe AWS access key fields contain sensitive credentials that should be masked when displayed.
-<%= f.text_field :access_key, display_name: "Access Key" %> -<%= f.text_field :access_key_id, display_name: "Access Key ID" %> +<%= f.password_field :access_key, display_name: "Access Key" %> +<%= f.password_field :access_key_id, display_name: "Access Key ID" %>
12-81: Well-structured and informative EC2 instance selectionThe EC2 instance options are well-organized by category with detailed descriptions, making it easy for users to understand the tradeoffs between different instance types. The recommendation box is particularly helpful for guiding users to choose appropriate instances for their specific use cases.
🧰 Tools
🪛 GitHub Actions: Ruby on Rails CI
[warning] 25-25: Extra whitespace detected at end of line.
[warning] 33-33: Extra whitespace detected at end of line.
[warning] 40-40: Extra whitespace detected at end of line.
app/assets/javascripts/autograder.js (2)
3-16: Good implementation of access key field togglingThe access key callback function properly disables/enables the access key fields based on the checkbox state, which matches the UI requirement for optional access key input.
18-30: Well-implemented tooltip and hover functionalityThe initialization of tooltips and hover effects for the EC2 dropdown enhances the user experience by providing visual feedback.
db/schema.rb (1)
152-155: Well-structured database fields for EC2 configurationThe new fields added to the autograders table properly support the EC2 autograder features:
instance_typefor storing the selected EC2 instance typeaccess_keyandaccess_key_idfor custom AWS credentialsuse_access_keyflag to toggle custom credentialsThese fields align well with the UI elements in the EC2 settings form and have appropriate default values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
app/views/autograders/_ec2_settings.html.erb (6)
9-10: Implement conditional display of access key fieldsThe access key fields should be conditionally shown based on the "Enable Access Key" checkbox status to improve usability. These fields should be disabled or hidden when the checkbox is unchecked.
You could add an
idto the checkbox and control the visibility with JavaScript:<%= f.check_box :use_access_key, display_name: "Enable Access Key", + id: "use_access_key_checkbox", help_text: "(Optional) Use your own provided access key to authenticate to different EC2 instances than the default one on Tango" %> -<%= f.text_field :access_key, display_name: "Access Key" %> -<%= f.text_field :access_key_id, display_name: "Access Key ID" %> +<%= f.text_field :access_key, display_name: "Access Key", id: "access_key_field", disabled: [email protected]_access_key %> +<%= f.text_field :access_key_id, display_name: "Access Key ID", id: "access_key_id_field", disabled: [email protected]_access_key %>Then in your autograder.js file:
document.addEventListener('DOMContentLoaded', function() { const checkbox = document.getElementById('use_access_key_checkbox'); const keyField = document.getElementById('access_key_field'); const keyIdField = document.getElementById('access_key_id_field'); function updateFieldState() { const isEnabled = checkbox.checked; keyField.disabled = !isEnabled; keyIdField.disabled = !isEnabled; } checkbox.addEventListener('change', updateFieldState); updateFieldState(); // Initial state });
14-44: Consider moving EC2 instance options to a helper methodThe large array of EC2 instance options makes this template harder to read and maintain. Consider moving this configuration to a helper method or a constant in a separate file.
You could create a helper method in
app/helpers/autograders_helper.rb:def ec2_instance_options [ ['T2 - General Purpose (Burstable)', [ ['t2.nano - 1 vCPU, 0.5 GiB RAM (minimal, lowest cost)', 't2.nano'], # ... rest of the options ]], # ... other categories ] endThen in your template:
<%= f.select :instance_type, grouped_options_for_select(ec2_instance_options, @autograder.instance_type), # ... rest of the code %>
50-50: Add fallback for @autograder.instance_typeIf
@autograderis nil or doesn't have aninstance_typeattribute set, this could cause errors. Add a fallback value.-grouped_options_for_select(ec2_instance_options, @autograder.instance_type), +grouped_options_for_select(ec2_instance_options, @autograder&.instance_type || 't2.micro'),
84-84: Fix typo in confirmation messageThere's a typo in the word "assesssment" (extra 's').
- data: { confirm: "Are you sure you want to delete the Autograder for this assesssment?" } %> + data: { confirm: "Are you sure you want to delete the Autograder for this assessment?" } %>
80-84: Consider separating save and delete actionsPlacing the delete button immediately after the save button could lead to accidental deletions. Consider adding more visual separation or moving the delete button to a different location.
<%= f.submit "Save Settings" %> +<div style="margin-top: 20px; border-top: 1px solid #ddd; padding-top: 15px;"> + <h5>Danger Zone</h5> <%= link_to "Delete Autograder", course_assessment_autograder_path(@course, @assessment), method: :delete, class: "btn danger", data: { confirm: "Are you sure you want to delete the Autograder for this assessment?" } %> +</div>
77-77: Consider adding more comprehensive pricing informationThe note about pricing is helpful, but more detailed information about cost implications would be valuable.
Consider adding more specific pricing information or warnings:
- <small>Note: Larger instances incur higher AWS costs. <a href='https://aws.amazon.com/ec2/pricing/on-demand/' target='_blank'>View EC2 pricing</a></small> + <small>Note: Larger instances incur higher AWS costs. For example, a t2.2xlarge can cost up to 8x more than a t2.micro. Check <a href='https://aws.amazon.com/ec2/pricing/on-demand/' target='_blank'>EC2 pricing</a> before selecting larger instances for your assignments.</small>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/views/autograders/_ec2_settings.html.erb(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
app/views/autograders/_ec2_settings.html.erb (1)
9-10: Use password fields for AWS credentialsAWS access keys and IDs are sensitive credentials that should be masked in the UI. Use
password_fieldinstead oftext_fieldto prevent exposure.-<%= f.text_field :access_key, display_name: "Access Key" %> -<%= f.text_field :access_key_id, display_name: "Access Key ID" %> +<%= f.password_field :access_key, display_name: "Access Key" %> +<%= f.password_field :access_key_id, display_name: "Access Key ID" %>
KesterTan
requested review from
a team,
20wildmanj,
KesterTan and
jhs-panda
and removed request for
a team and
KesterTan
KesterTan
requested review from
Futurecomputerfixer
and removed request for
20wildmanj and
jhs-panda
There was a problem hiding this comment.
Security (secrets handling)
Problem: access_key / access_key_id are stored and passed around as plaintext. They must be encrypted at rest, scrubbed from logs, and not echoed in the UI.
Changes:
Prefer IAM roles over keys
File: app/helpers/assessment_autograde_core.rb
In the EC2 block, if use_access_key is false, omit accessKey/accessKeyId entirely from job_properties (don’t send empty strings).
Encrypt at rest (follow GitHub integration precedent)
Add encrypted columns mirroring github_integrations.access_token_ciphertext.
New migration (replace plaintext columns introduced in this PR):
Files:
Replace db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb with one that adds:
access_key_ciphertext :text
access_key_id_ciphertext :text
instance_type :string, default: "" (kept)
Drop the plaintext access_key/access_key_id columns if already migrated.
Model:
File: app/models/autograder.rb
Use the same encryption library already used (e.g., Lockbox) to define:
encrypts :access_key
encrypts :access_key_id
where those virtual attributes map to *_ciphertext.
Gate presence validations with use_access_key (see “Validations” section).
Scrub from logs & params filtering
File: config/initializers/filter_parameter_logging.rb
Add: Rails.application.config.filter_parameters += %i[access_key access_key_id]
File: app/controllers/autograders_controller.rb
Ensure we never flash or log raw values.
UI should not echo secrets
File: app/views/autograders/_ec2_settings.html.erb
Change inputs to password-style and avoid prepopulating:
<%= f.password_field :access_key, display_name: "Access Key", value: nil, autocomplete: "new-password" %>
<%= f.text_field :access_key_id, display_name: "Access Key ID", value: nil, autocomplete: "off" %>
Add a small hint: “Leave blank to keep existing secret.”
Strong params & controller flow
Problem: Params are permitted in two calls; :use_access_key isn’t included; and empty-string secrets are sent downstream.
Changes:
File: app/controllers/autograders_controller.rb
Unify strong params and include all fields:
def autograder_params
params.require(:autograder).permit(
:autograde_timeout, :autograde_image, :release_score,
:use_access_key, :access_key, :access_key_id, :instance_type
)
end
In create defaults block, avoid initializing secrets to "". Prefer nil:
a.access_key_id = nil
a.access_key = nil
a.instance_type = "t3.micro" # see validations below
In update/create, if use_access_key is false, ignore any provided access_key/access_key_id (set to nil before save).
Validations & defensive checks
Problem: instance_type is free-form; secrets aren’t validated when enabled.
Changes:
File: app/models/autograder.rb
Add allow-list for instance types shown in the UI:
INSTANCE_TYPES = %w[
t2.nano t2.micro t2.small t2.medium t2.large t2.xlarge t2.2xlarge
t3.micro t3.small t3.medium t3.large
c5.large c5.xlarge c5.2xlarge
r5.large r5.xlarge r5.2xlarge
].freeze
validates :instance_type, inclusion: { in: INSTANCE_TYPES }, allow_blank: true
Gate secrets validation behind the toggle:
with_options if: :use_access_key do
validates :access_key_id, presence: true, format: { with: /\A[A-Z0-9]{16,24}\z/, message: "looks invalid" }
validates :access_key, presence: true
end
Add bounded timeout validation if not already present:
validates :autograde_timeout, inclusion: { in: 10..900 }
Tango payload assembly
Problem: Empty strings are sent; json built twice; feature flag should be respected server-side.
Changes:
File: app/helpers/assessment_autograde_core.rb
Build a Ruby hash first, then JSON once at the end; respect config.x.ec2_ssh and use_access_key:
job_properties = {
"image" => @autograde_prop.autograde_image,
"timeout" => @autograde_prop.autograde_timeout,
"callback_url" => callback_url,
"jobName" => job_name,
"disable_network" => assessment.disable_network
}
if Rails.configuration.x.ec2_ssh
job_properties["ec2Vmms"] = true
job_properties["instanceType"] = @autograde_prop.instance_type.presence || "t3.micro"
if @autograde_prop.use_access_key?
job_properties["accessKey"] = @autograde_prop.access_key
job_properties["accessKeyId"] = @autograde_prop.access_key_id
end
end
job_properties = job_properties.to_json
This ensures keys are omitted entirely when not used.
Frontend (JS & form UX)
Problem: Minor JS bug; console noise; better UX for disabled/secret fields.
Changes:
File: app/assets/javascripts/autograder.js
Fix .val() misuse and remove console log:
;(function() {
$(document).ready(function () {
function access_key_callback() {
const checked = $(this).prop('checked');
const
const
$access_key_field.prop('disabled', !checked);
$access_key_id_field.prop('disabled', !checked);
if (!checked) {
$access_key_field.val('');
$access_key_id_field.val('');
}
}
$('#autograder_use_access_key').on('change', access_key_callback);
access_key_callback.call($('#autograder_use_access_key'));
// Optional: keep dropdown UI tweaks, but drop console.log
$('#autograder_instance_type option').hover(
function() { $(this).addClass('highlighted-option'); },
function() { $(this).removeClass('highlighted-option'); }
);
});
})();
File: app/views/autograders/_ec2_settings.html.erb
Add pattern to Access Key ID (light client-side feedback):
<%= f.text_field :access_key_id, display_name: "Access Key ID",
value: nil, autocomplete: "off",
pattern: "[A-Z0-9]{16,24}",
title: "Looks like an invalid AWS Access Key ID" %>
Feature flag correctness
Problem: UI is gated by config.x.ec2_ssh, but server must guard too.
Changes:
Files:
config/environments/development.rb – OK to be true.
config/environments/production.rb.template – defaults to false (good).
Server guard:
File: app/helpers/assessment_autograde_core.rb
Ensure the EC2 payload branch only runs if Rails.configuration.x.ec2_ssh is true (already applied above).
Migrations / schema churn
Problem: schema.rb shows charset/timestamp engine churn unrelated to this PR; plaintext columns introduced for secrets.
Changes:
Files: db/schema.rb
Regenerate schema after replacing the plaintext secret columns with encrypted *_ciphertext. Avoid unrelated charset/timestamp diffs (ensure you run the same DB engine/version locally as CI).
Files: db/migrate/20241205233214_add_ec2_ssh_fields_to_autograders.rb, db/migrate/20241211042124_add_use_access_key_to_autograder.rb
Update to remove plaintext columns and add encrypted ones (see “Security” section). Keep use_access_key:boolean migration.
Tests
Problem: No tests cover new payload assembly and validations.
Add:
File: spec/helpers/assessment_autograde_core_spec.rb
Cases:
ec2_ssh=false → no ec2Vmms/instanceType/keys in payload.
ec2_ssh=true, use_access_key=false → ec2Vmms & instanceType present, no keys.
ec2_ssh=true, use_access_key=true → keys present.
File: spec/models/autograder_spec.rb
Validates inclusion for instance_type.
Validates secrets presence only when use_access_key=true.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
app/models/autograder.rb (1)
20-26: Add minimal validation for instance_type; keep key-id regex pragmatic.Add presence (and maybe length) for instance_type to prevent empty payload to Tango. Your access_key_id regex is fine; consider 20 chars typical, but current range is acceptable.
validates :autograde_image, length: { maximum: 64 } - with_options if: :use_access_key do + validates :instance_type, presence: true, length: { maximum: 32 }, allow_nil: true + with_options if: :use_access_key do validates :access_key_id, presence: true, format: { with: /\A[A-Z0-9]{16,24}\z/, message: "looks invalid" } validates :access_key, presence: true endBased on learnings
🧹 Nitpick comments (4)
config/initializers/filter_parameter_logging.rb (1)
4-4: Broaden secret filtering to cover camelCase and ciphertext variants.Add filters for accessKey/accessKeyId and ciphertext columns; include a regex for future keys.
-Rails.application.config.filter_parameters += [:password, :session, :warden, :secret, :salt, :cookie, :csrf, :restful_key, :lockbox_master_key, :lti_tool_private_key, :access_key, :access_key_id] +Rails.application.config.filter_parameters += [ + :password, :session, :warden, :secret, :salt, :cookie, :csrf, :restful_key, + :lockbox_master_key, :lti_tool_private_key, + :access_key, :access_key_id, + :accessKey, :accessKeyId, + :access_key_ciphertext, :access_key_id_ciphertext, + /access.*key/i +]app/controllers/autograders_controller.rb (2)
19-21: Avoid encrypting and storing empty-string secrets on create; align default instance type.Set secrets to nil, and pick one default instance type (helper uses "t3.micro").
- a.access_key_id = "" - a.access_key = "" - a.instance_type = "t2.micro" + a.access_key_id = nil + a.access_key = nil + a.instance_type = "t3.micro"
125-128: Optional: normalize boolean in strong params layer.Casting here reduces surprises downstream.
def autograder_params - params.require(:autograder).permit( + permitted = params.require(:autograder).permit( :autograde_timeout, :autograde_image, :release_score, :use_access_key, :access_key, :access_key_id, :instance_type - ) + ) + permitted[:use_access_key] = ActiveModel::Type::Boolean.new.cast(permitted[:use_access_key]) + permitted endapp/helpers/assessment_autograde_core.rb (1)
173-181: EC2 block: only include secrets when present; keep instanceType default consistent.Guard against empty strings and align default with controller “create”.
- if Rails.configuration.x.ec2_ssh + if Rails.configuration.x.ec2_ssh job_properties["ec2Vmms"] = true - job_properties["instanceType"] = @autograde_prop.instance_type.presence || "t3.micro" + job_properties["instanceType"] = @autograde_prop.instance_type.presence || "t3.micro" - if @autograde_prop.use_access_key? - job_properties["accessKey"] = @autograde_prop.access_key - job_properties["accessKeyId"] = @autograde_prop.access_key_id - end + if @autograde_prop.use_access_key? + key = @autograde_prop.access_key.presence + key_id = @autograde_prop.access_key_id.presence + job_properties["accessKey"] = key if key + job_properties["accessKeyId"] = key_id if key_id + end endNote: If you adopt "t3.micro" above, also update AutogradersController#create to match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/controllers/autograders_controller.rb(3 hunks)app/helpers/assessment_autograde_core.rb(1 hunks)app/models/autograder.rb(1 hunks)app/views/autograders/_ec2_settings.html.erb(1 hunks)config/initializers/filter_parameter_logging.rb(1 hunks)db/migrate/20251021034813_encrypt_autograder_credentials.rb(1 hunks)db/schema.rb(23 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/autograders/_ec2_settings.html.erb
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-09-20T20:46:52.775Z
Learnt from: evanyeyeye
PR: autolab/Autolab#2209
File: app/controllers/autograders_controller.rb:0-0
Timestamp: 2024-09-20T20:46:52.775Z
Learning: In the `AutogradersController`, ensure that the `download_file` method has `action_auth_level :download_file, :instructor` to restrict access to instructors.
Applied to files:
app/controllers/autograders_controller.rb
🧬 Code graph analysis (1)
config/initializers/filter_parameter_logging.rb (1)
config/application.rb (1)
config(10-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (3)
app/models/autograder.rb (1)
8-11: Encryption DSL verified as correct—no changes needed.The
has_encryptedDSL is properly mapped toaccess_key_ciphertextandaccess_key_id_ciphertextcolumns in the schema. Lockbox 1.3.0 (as confirmed in Gemfile.lock) uses this standard syntax for database field encryption. The migration adds both ciphertext columns and correctly encrypts/decrypts through the model attributes. The conditional validations usinguse_access_keyare also properly defined as a database column.db/schema.rb (2)
62-63: Verify datetime to timestamp conversion implications.Several columns have been converted from
datetimetotimestamp:
announcements.start_dateandend_dateassessments.due_at,end_at, andstart_atscheduler.nextEnsure this conversion is intentional, as
timestamphas different characteristics:
- Range limitation (1970-2038 on older systems; extended to 2106 in MySQL 5.6.4+ with fractional seconds)
- Automatic timezone conversion based on server timezone
- Different storage size and precision
This is generally acceptable for assessment/announcement dates but verify that existing data falls within the valid range and timezone handling meets requirements.
Also applies to: 89-91, 333-333
152-157: EC2 credential encryption is properly implemented.The Autograder model uses
has_encrypted(from the lockbox gem) for automatic encryption/decryption ofaccess_keyandaccess_key_id. The migration successfully removes plaintext columns and stores encrypted data in ciphertext fields. Validations enforce required credentials whenuse_access_keyis true, including AWS access key ID format validation.Minor improvement: Add NOT NULL constraints to
access_key_ciphertextandaccess_key_id_ciphertextcolumns to enforce data integrity whenuse_access_keyis true. Currently the schema allows NULL values for these fields despite the model requiring them when the feature is enabled.
| # Clear secrets if use_access_key is disabled | ||
| params_to_update = autograder_params | ||
| unless params_to_update[:use_access_key] | ||
| params_to_update[:access_key] = nil | ||
| params_to_update[:access_key_id] = nil | ||
| end | ||
|
|
||
| if @autograder.update(params_to_update) && @assessment.update(assessment_params) |
There was a problem hiding this comment.
Bug: "0"/"1" strings make unless params_to_update[:use_access_key] always truthy. Cast to boolean.
Without casting, secrets won’t be cleared when the checkbox is unchecked.
- params_to_update = autograder_params
- unless params_to_update[:use_access_key]
+ params_to_update = autograder_params.to_h
+ use_access_key = ActiveModel::Type::Boolean.new.cast(params_to_update[:use_access_key])
+ unless use_access_key
params_to_update[:access_key] = nil
params_to_update[:access_key_id] = nil
end
-
+ if use_access_key
+ # preserve existing secrets when fields are blank
+ params_to_update.delete(:access_key) if params_to_update[:access_key].to_s.strip.empty?
+ params_to_update.delete(:access_key_id) if params_to_update[:access_key_id].to_s.strip.empty?
+ end🤖 Prompt for AI Agents
In app/controllers/autograders_controller.rb around lines 44 to 51,
params_to_update[:use_access_key] comes from form params as "0"/"1" strings so
the unless check is always truthy; convert/cast that param to a boolean before
the unless (for example using Rails boolean casting or comparing to "1") so when
the checkbox is unchecked the condition evaluates false and
access_key/access_key_id are set to nil; update the code to cast use_access_key
to a proper boolean and then perform the conditional clearing of secrets.
| Autograder.reset_column_information | ||
| Autograder.find_each do |autograder| | ||
| if autograder.read_attribute(:access_key).present? | ||
| autograder.access_key = autograder.read_attribute(:access_key) | ||
| end | ||
| if autograder.read_attribute(:access_key_id).present? | ||
| autograder.access_key_id = autograder.read_attribute(:access_key_id) | ||
| end | ||
| autograder.save(validate: false) # Skip validation during migration | ||
| end |
There was a problem hiding this comment.
Avoid triggering model callbacks during data migration.
Using save(validate: false) still runs callbacks (after_commit), risking heavy side effects. Compute ciphertext via writers, then persist with update_columns to bypass callbacks.
- Autograder.reset_column_information
- Autograder.find_each do |autograder|
- if autograder.read_attribute(:access_key).present?
- autograder.access_key = autograder.read_attribute(:access_key)
- end
- if autograder.read_attribute(:access_key_id).present?
- autograder.access_key_id = autograder.read_attribute(:access_key_id)
- end
- autograder.save(validate: false) # Skip validation during migration
- end
+ Autograder.reset_column_information
+ Autograder.find_each do |autograder|
+ updates = {}
+ if (plain = autograder.read_attribute(:access_key)).present?
+ autograder.access_key = plain
+ updates[:access_key_ciphertext] = autograder.access_key_ciphertext
+ end
+ if (plain_id = autograder.read_attribute(:access_key_id)).present?
+ autograder.access_key_id = plain_id
+ updates[:access_key_id_ciphertext] = autograder.access_key_id_ciphertext
+ end
+ autograder.update_columns(updates) if updates.any?
+ end📝 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.
| Autograder.reset_column_information | |
| Autograder.find_each do |autograder| | |
| if autograder.read_attribute(:access_key).present? | |
| autograder.access_key = autograder.read_attribute(:access_key) | |
| end | |
| if autograder.read_attribute(:access_key_id).present? | |
| autograder.access_key_id = autograder.read_attribute(:access_key_id) | |
| end | |
| autograder.save(validate: false) # Skip validation during migration | |
| end | |
| Autograder.reset_column_information | |
| Autograder.find_each do |autograder| | |
| updates = {} | |
| if (plain = autograder.read_attribute(:access_key)).present? | |
| autograder.access_key = plain | |
| updates[:access_key_ciphertext] = autograder.access_key_ciphertext | |
| end | |
| if (plain_id = autograder.read_attribute(:access_key_id)).present? | |
| autograder.access_key_id = plain_id | |
| updates[:access_key_id_ciphertext] = autograder.access_key_id_ciphertext | |
| end | |
| autograder.update_columns(updates) if updates.any? | |
| end |
🤖 Prompt for AI Agents
In db/migrate/20251021034813_encrypt_autograder_credentials.rb around lines 8 to
17, the migration currently uses save(validate: false) which still runs model
callbacks; instead, for each Autograder load the raw values with read_attribute,
assign them through the model writer methods so the encryption writers compute
ciphertext in-memory (e.g., autograder.access_key = raw_key), then persist only
the encrypted DB columns with update_columns (bypassing validations and
callbacks) — update the encrypted column names (and clear plain-text columns if
needed) using update_columns to avoid triggering after_commit/other callbacks.
| add_column :autograders, :access_key, :string, default: "" | ||
| add_column :autograders, :access_key_id, :string, default: "" | ||
|
|
There was a problem hiding this comment.
Don’t reintroduce plaintext with default "".
Restore columns as nullable with no default to reflect prior semantics and avoid silent empty strings.
- add_column :autograders, :access_key, :string, default: ""
- add_column :autograders, :access_key_id, :string, default: ""
+ add_column :autograders, :access_key, :string, null: true
+ add_column :autograders, :access_key_id, :string, null: true📝 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.
| add_column :autograders, :access_key, :string, default: "" | |
| add_column :autograders, :access_key_id, :string, default: "" | |
| add_column :autograders, :access_key, :string, null: true | |
| add_column :autograders, :access_key_id, :string, null: true |
🤖 Prompt for AI Agents
In db/migrate/20251021034813_encrypt_autograder_credentials.rb around lines 26
to 28, the migration adds access_key and access_key_id with default: "" which
reintroduces plaintext empty-string defaults; change the add_column calls to add
nullable string columns with no default (i.e., remove default: "" and do not set
null: false) so the columns are created as NULLable and without a default value,
matching prior semantics.
| t.bigint "course_user_datum_id" | ||
| t.bigint "course_id" | ||
| t.bigint "risk_condition_id" |
There was a problem hiding this comment.
Critical: Foreign key type mismatch in watchlist_instances.
The watchlist_instances table declares course_id and course_user_datum_id as bigint, but the referenced tables use integer primary keys:
coursestable (line 176):id: :integercourse_user_datatable (line 160):id: :integer
This type mismatch will cause database constraint errors and must be corrected before deployment.
Apply this fix by updating the migration to use matching types:
create_table "watchlist_instances", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
- t.bigint "course_user_datum_id"
- t.bigint "course_id"
+ t.integer "course_user_datum_id"
+ t.integer "course_id"
t.bigint "risk_condition_id"Similarly, update watchlist_configurations:
create_table "watchlist_configurations", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
t.json "category_blocklist"
t.json "assessment_blocklist"
- t.bigint "course_id"
+ t.integer "course_id"After updating the migration, regenerate the schema with rails db:schema:dump.
📝 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.
| t.bigint "course_user_datum_id" | |
| t.bigint "course_id" | |
| t.bigint "risk_condition_id" | |
| t.integer "course_user_datum_id" | |
| t.integer "course_id" | |
| t.bigint "risk_condition_id" |
🤖 Prompt for AI Agents
In db/schema.rb around lines 434 to 436, the watchlist_instances columns
course_user_datum_id and course_id are declared as bigint but must match the
referenced tables which use integer primary keys; update the original migration
that creates/changes watchlist_instances to declare these foreign key columns as
:integer (not :bigint) and add/adjust explicit foreign_key constraints if
present, make the same correction in the migration for watchlist_configurations
where course_id (or any course_user_datum_id) was set to bigint so they use
:integer, then run rails db:migrate (if changing a pending migration) or create
a new migration to change_column to :integer for existing deployments and
finally regenerate the schema with rails db:schema:dump.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/models/autograder.rb (1)
20-26: Consider tightening credential format validations.The current validations provide basic checks, but AWS credentials have specific format requirements that could be enforced more strictly:
- Access Key ID: AWS Access Key IDs are typically 20 characters and use prefixes like AKIA (long‑term) or ASIA (temporary). The current regex allows 16-24 characters without enforcing the prefix.
- Secret Access Key: No format validation is applied. AWS Secret Access Keys are typically 40-character values with base64-like encoding.
- Error message: "looks invalid" could be more descriptive.
While AWS will reject invalid credentials at runtime, catching format errors earlier improves user experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/assets/javascripts/autograder.js(1 hunks)app/controllers/autograders_controller.rb(3 hunks)app/models/autograder.rb(1 hunks)app/views/autograders/_ec2_settings.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- app/assets/javascripts/autograder.js
- app/controllers/autograders_controller.rb
- app/views/autograders/_ec2_settings.html.erb
🔇 Additional comments (1)
app/models/autograder.rb (1)
8-10: Good security practice: encrypting AWS credentials at rest.Using Lockbox's
has_encryptedfor AWS credentials is the right approach to protect sensitive data.
4 participants
Description
This PR enables the use and configuration of EC2 autograding instances via Autolab.
Documentation will be added in a future PR.
How Has This Been Tested?
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for linting