Skip to content
Permalink
Browse files Browse the repository at this point in the history
Progresses on grades change view and events. Discovered security vuln…
…erability that allows a course admin to change whatever grade they want if they know the ID, even in other classes
  • Loading branch information
App Academy Student committed Mar 19, 2015
1 parent dc422a1 commit 134f548
Show file tree
Hide file tree
Showing 16 changed files with 89 additions and 93 deletions.
18 changes: 14 additions & 4 deletions app/assets/javascripts/collections/grades.js
Expand Up @@ -7,7 +7,7 @@ MerlinsBoard.Collections.Grades = Backbone.Collection.extend({

model: MerlinsBoard.Models.Grade,

getOrFetch: function (id) {
getOrFetch: function (id, course_id) {
var grade = this.get(id);
var grades = this;

Expand All @@ -31,16 +31,26 @@ MerlinsBoard.Collections.Grades = Backbone.Collection.extend({

//some logic here to check if "data" was already passed in, and fusing that to the data parameter...

_.extend(options,{ data: $.param({ course_id: this.course_id}) }); //options is changed
//with this, I might always have to bind fetch - be mindful of this in case I need to fetch more data
_.extend(options,{ data: $.param({ course_id: this.course_id}) });
return Backbone.Collection.prototype.fetch.call(this, options);
},

student: function () {
if (!this._student) {
this._student = new MerlinsBoard.Models.User();
}

return this._student
},

parse: function (resp) {
this.student = new MerlinsBoard.Models.User({fname: resp.student_fname,lname: resp.student_lname});
this.student().set({fname: resp.student_fname,lname: resp.student_lname});
// this.course_id = resp.course_id should only set once and then never again

resp.student_fname.delete //is there a better way to clean this up?
resp.student_fname.delete
resp.course_id.delete

return resp.grades
}

Expand Down
2 changes: 1 addition & 1 deletion app/assets/javascripts/models/grade.js
@@ -1,4 +1,4 @@
MerlinsBoard.Models.Grade = Backbone.Model.extend({
urlRoot: 'api/grades',
validate: function () {}
})
})
7 changes: 4 additions & 3 deletions app/assets/javascripts/routers/router.js
Expand Up @@ -32,7 +32,7 @@ MerlinsBoard.Routers.Router = Backbone.Router.extend({
"course/:course_id/assignments/:id/edit" : "editAssignment",
//grades
"course/:id/grades/student-search" : "gradeSearch",
"course/:course_id/grades/user/:user_id" : "gradesShow"
"course/:course_id/grades/user/:user_id" : "gradeShow"
//misc
// "user/:id": "showuser"
//":wildcard": "does not exist" --self explanatory
Expand All @@ -43,9 +43,9 @@ MerlinsBoard.Routers.Router = Backbone.Router.extend({
enrollcourses: function () {
var allcourses = new MerlinsBoard.Collections.Courses([],{owner: this.currentUser});

allcourses.fetch();
this.currentUser.fetch()

var enrollView = new MerlinsBoard.Views.CoursesEnroll({collection: allcourses,model: this.currentUser});
var enrollView = new MerlinsBoard.Views.CoursesEnroll({model: this.currentUser});
this.swapView(enrollView);
},

Expand Down Expand Up @@ -154,6 +154,7 @@ MerlinsBoard.Routers.Router = Backbone.Router.extend({
gradeShow: function (course_id, user_id) {
// var course = MerlinsBoard.Courses.getOrFetch(id);
var grades = new MerlinsBoard.Collections.Grades({course_id: course_id, user_id: user_id});

grades.fetch();

var gradesList = new MerlinsBoard.Views.GradesStudent({collection: grades});
Expand Down
2 changes: 1 addition & 1 deletion app/assets/javascripts/views/courses/course-form.js
Expand Up @@ -23,7 +23,7 @@ MerlinsBoard.Views.CourseForm = Backbone.View.extend({
this.model.save(attrs, {
success: function () {
MerlinsBoard.Courses.add(this.model,{merge: true})
Backbone.history.navigate("",{trigger: true}) //instead do a "course created/saved"
Backbone.history.navigate("course/search",{trigger: true})
}.bind(this),
error: function (model,resp) {
var errorArray = resp.responseJSON;
Expand Down
3 changes: 1 addition & 2 deletions app/assets/javascripts/views/courses/course-list.js
@@ -1,9 +1,8 @@
MerlinsBoard.Views.CoursesList = Backbone.View.extend({
initialize: function () {
this.listenTo(this.collection, "add remove sync", this.render);
this.listenTo(this.collection, "reset add sync", this.render);
},


template: JST["courses/list"],

tagName: "ul",
Expand Down
33 changes: 12 additions & 21 deletions app/assets/javascripts/views/courses/course_search.js
@@ -1,6 +1,5 @@
MerlinsBoard.Views.CoursesSearch = Backbone.View.extend({
initialize: function () {
this.searchCollection = new MerlinsBoard.Collections.CoursesSearch();
},

template: JST["courses/coursesearch"],
Expand All @@ -11,6 +10,14 @@ MerlinsBoard.Views.CoursesSearch = Backbone.View.extend({
return this
},

searchCollection: function () {
if (!this._searchCollection) {
this._searchCollection = new MerlinsBoard.Collections.CoursesSearch();
}

return this._searchCollection
},

tagName: "section",

className: "course-search",
Expand All @@ -19,30 +26,14 @@ MerlinsBoard.Views.CoursesSearch = Backbone.View.extend({
"submit form.course-find":"search"
},

// search: function (event) {
// event.preventDefault();
// var query = $("input.course-find-input").val();
//
// var filtered = this.collection.filter(function (course) {
// var pattern = new RegExp(query, "gi");
// var result = course.get("name").match(pattern);
// return result
// })
//
// var filteredCollection = new MerlinsBoard.Collections.Courses([],{owner: MerlinsBoard.CurrentUser});
// filteredCollection.set(filtered);
//
// var searchList = new MerlinsBoard.Views.CoursesList({collection: filteredCollection});
// this.$('section.course-results').html(searchList.render().$el);
// }

search: function (event) {
event.preventDefault();
var queryCourse = $("input.course-find-input").val();
this.searchCollection.fetch({data: $.param(query: queryCourse)});
this.searchCollection().fetch({data: $.param({query: queryCourse})});

var searchList = new MerlinsBoard.Views.CoursesList({collection: this.searchCollection});
$('section.course-results').html(searchList.render.$el); //needs to be global from DOM.
var searchList = new MerlinsBoard.Views.CoursesList({collection: this.searchCollection()});
//want to call remove on search results
$('section.course-results').html(searchList.render().$el);
}

})
22 changes: 5 additions & 17 deletions app/assets/javascripts/views/courses/courses_enroll.js
@@ -1,32 +1,20 @@
MerlinsBoard.Views.CoursesEnroll = Backbone.View.extend({
initialize: function () {
this.coursesearchView = new MerlinsBoard.Views.CoursesSearch({collection: this.collection}); //render will put these in manually
this.coursesearchView = new MerlinsBoard.Views.CoursesSearch();
this.usercoursesView = new MerlinsBoard.Views.CoursesList({collection: this.model.courses()});
this.usertaughtcoursesView = new MerlinsBoard.Views.CoursesList({collection: this.model.taughtcourses()});
},

template: JST['courses/enroll'],

render: function () {
this.$el.html(this.template());

this.$("section.courses-attended").html(this.usercoursesView.render().$el);
this.$("section.courses-taught").html(this.usertaughtcoursesView.render().$el);
this.$("section.course-search").html(this.coursesearchView.render().$el);

return this
},

//below two, again - hardcode URLs instead

show: function (event) {
event.preventDefault();
var id = $(event.currentTarget).data("id");
Backbone.history.navigate("course/" + id + "/enroll", {trigger:true})
},

events: {
"click a": "show"
return this
}

});
4 changes: 2 additions & 2 deletions app/assets/javascripts/views/grades/grade-search-student.js
@@ -1,9 +1,9 @@
MerlinsBoard.Views.SearchStudent = Backbone.View.extend({
MerlinsBoard.Views.SearchStudentGradesResults = Backbone.View.extend({
initialize: function () {
this.listenTo(this.collection, "add remove reset", this.render)
},

className: "grades-studentsearch",
className: "grades-student-search",

template: JST["grades/grades-student-search"],

Expand Down
38 changes: 18 additions & 20 deletions app/assets/javascripts/views/grades/grades-student.js
@@ -1,12 +1,12 @@
//Lists a students grade - an admin has access to this view to edit grades
MerlinsBoard.Views.GradesStudent = Backbone.View.extend({
initialize: function () {
this.listenTo(this.collection, "add change:grade remove", this.render)
this.listenTo(this.collection, "add change:grade remove sync", this.render)
_.bindAll(this, "gradeSaveCallback", "gradeSaveErrorCallback")
//for jbuidler - nest each of a student's grade under them along with basic information about the assignment
//for jbuidler - nest each of a student's grade under them along with basic information about the assignmen
},

template: JST["grades/grades-student"],
template: JST["grades/grades-student-list"],

events: {
"click .grade-number":"editGrade",
Expand All @@ -18,41 +18,39 @@ MerlinsBoard.Views.GradesStudent = Backbone.View.extend({
tagName: "section",

render: function () {
var renderedContent = this.template({grades: this.collection});
var renderedContent = this.template({grades: this.collection, student: this.collection.student()});
this.$el.html(renderedContent);
return this.$el
return this
},

editGrade: function (event) {
var gradeString = $(event.currentTarget).text();
var gradeString = $(event.currentTarget).val();
var num = parseInt(gradeString);
var $input = $("<input type='number'>").addClass('grade-input').val(num);

var $input = $("<input type='number' min='0' step='1' max='100'>").addClass('grade-input').val(num);
this.modelNumber = $(event.currentTarget).data('id');

$(".grade-number").html(input)
$(".grade-number[data-id=".concat(this.modelNumber,"]")).html($input)
},

saveGrade: function (event) {
var editedGrade = this.collection.getOrFetch(this.modelNumber);
var newGrade = $('input.grade-input').val();
var newGrade = parseInt($('input.grade-input').val());
var courseID = this.collection.course_id;

debugger

editedGrade.set({grade: newGrade});
//two options, send in the params with the model and strong params takes care of it
//or send it in as an option for the save option
editedGrade.save({},{success: this.gradeSaveCallback,
error: this.gradeSaveErrorCallback,
data: $.param({course_id: courseID})
editedGrade.save({course_id: courseID},{success: this.gradeSaveCallback(editedGrade),
error: this.gradeSaveErrorCallback
});
},

gradeSaveCallback: function () {
this.collection.add(editedGrade,{merge: true})//this should be a closure - also editedGrade I think should persist as a variable...
// $(".grade-number").html(editedGrade.get('grade')); this wont work because I'm inspecific, but I may not need it anyway to rerender
gradeSaveCallback: function (editedGrade) {
this.collection.fetch(); //unideal - needs to be banished with composite view paradigm.
// this.collection.add(editedGrade,{merge: true});
},

gradeSaveErrorCallback: function (model, response) {
gradeSaveErrorCallback: function (model, resp) {

var errorArray = resp.responseJSON
var $errorList = $("<ul>").addClass('errors');
_.each(errorArray, function (error) {
Expand Down
1 change: 0 additions & 1 deletion app/assets/templates/courses/coursesearch.jst.ejs
Expand Up @@ -10,5 +10,4 @@ all this will be is organized links-->
</form>

<section class="course-results">

</section>
4 changes: 2 additions & 2 deletions app/assets/templates/courses/list.jst.ejs
@@ -1,3 +1,3 @@
<% courses.each( function (course) { %>
<li><a href="#" data-id="<%=course.id%>"><%= course.escape("name") %></a></li>
<% })%>
<li><a href="<%= '#course/' + course.id + '/enroll'%>"><%= course.escape("name") %></a></li>
<% })%>
8 changes: 3 additions & 5 deletions app/assets/templates/grades/grades-student-list.jst.ejs
@@ -1,12 +1,10 @@
<h1>Grades for <%=grades.escape('fname') + " " + grades.escape('lname')%></h1>
<h1>Grades for <%= student.escape('fname') + " " + student.escape('lname')%></h1>

<section class="grade-errors"></section>

<% grades.each(function (grade) { %>
<h4> grade.escape('assignment_title') </h4>
<p> grade.escape('description')</p>
<h4> Assignment name:<%= grade.escape('title') %></h4>
<p> description: <%= grade.escape('description') %></p>
Grade: <strong class="grade-number" data-id="<%= grade.id %>"><%= grade.get('grade')%></strong>
<br>
<% }) %>

//I think all I'll do is just pretend like nothing happened if the entry is invalid
26 changes: 17 additions & 9 deletions app/controllers/api/grades_controller.rb
@@ -1,20 +1,23 @@
class Api::GradesController < Api::ApiController
before_action(except: [:index]) {admins_only(params["course_id"])} #wil always need to pass this in on every fetch
before_action(only: [:update, :show]) {admins_only(params["course_id"])}
before_action :is_user_or_instructor?, only: [:index]

def index
#I may want an internal control here instead of using the before_action...
@grades = Grade.includes(:assignment,:course,:user).where("user_id = ?", params["user_id"])
@student = @grades.first.user
@course_id = params["course_id"].to_i

@grades = @grades.select {|grade| grade.course.id == params["course_id"].to_i}
end

def destroy
@grade = Grade.find(params[:id])
@grade.destroy
render json: {}
end
#Neither of these may be needed because they should only be created/destroyed depending on the assignment

# def destroy
# @grade = Grade.find(params[:id])
# @grade.destroy
# render json: {}
# end
# def create
# @grade = Grade.new(grade_params)
#
Expand All @@ -25,10 +28,15 @@ def destroy
# end
# end

def show
@grade = Grade.find(params[:id])
render json: @grade
end
#will need to create unique validator to ensure congruency between course auth and resource id course....
def update
@grade = Grade.find(params[:id])

if @grade.save
if @grade.update(grade_params)
render json: @grades
else
render json: @grade.errors.full_messages, status: 422
Expand All @@ -43,7 +51,7 @@ def is_user_or_instructor?
end

def grade_params
params.require(:grade).permit(:grade, :assignment_id, :user_id)
params.permit(:grade, :assignment_id, :user_id) #need to change grade column - it confuses params_wrapper
end

end
11 changes: 6 additions & 5 deletions app/views/api/grades/index.json.jbuilder
@@ -1,16 +1,17 @@
json.grades @grades do |grade_obj|
json.id grade_obj.id
json.grade grade_obj.grade
json.assignment_id grade_obj.assignment_id
json.id grade_obj.assignment_id
json.user_id grade_obj.user_id
json.assignment_title grade_obj.assignment.title
json.assignment_description grade_obj.assignment.description
json.title grade_obj.assignment.title
json.description grade_obj.assignment.description
end

json.student_fname @student.fname
json.student_lname @student.lname
json.course_id @student.course.id
json.course_id @course_id

#remember then that for a single model, only top-level attrs will be assigned
# for a collection, each entry in the array should be top-level attrs (or wrapped in only a single object wrapper)
# but the array itself must be top-level
# weird: Cannot mix json.array! with other top-level attrs
# weird: Cannot mix json.array! with other top-level attrs
1 change: 1 addition & 0 deletions config/routes.rb
Expand Up @@ -15,6 +15,7 @@
resources :announcements #might have to "member do" for easy-access custom routes from a particular course
resources :assignments
resources :resources
resources :grades, only: [:update, :show]
resources :courses do
get "course_search", on: :collection
end
Expand Down

0 comments on commit 134f548

Please sign in to comment.