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

URL with name instead of id #318

Closed

Conversation

Termanty
Copy link
Contributor

This pull request changes course id to course name in URL. Same thing is done also for exercise.
Security hole is removed by introducing check for URL parameters so that teachers from one organization can't do changes to courses in other organization.

@jamo
Copy link
Member

jamo commented Aug 10, 2015

Just a thought - we might want to use this: http://guides.rubyonrails.org/routing.html#overriding-named-route-parameters so that we would have params[:name] instead of params[:id]

@@ -206,10 +206,17 @@ def assign_show_view_vars

def set_organization
@organization = Organization.find_by(slug: params[:organization_id])
fail ActiveRecord::RecordNotFound unless @organization
Copy link
Member

Choose a reason for hiding this comment

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

find_by! maybe?

jruo added 3 commits August 12, 2015 13:41
…ad-of-id

Conflicts:
	app/controllers/points_controller.rb
	app/controllers/solutions_controller.rb
	app/controllers/submissions_controller.rb
	app/models/course.rb
	app/views/exercises/show.html.erb
	app/views/submissions/_submission_details.html.erb
	config/routes.rb
	spec/controllers/points_controller_spec.rb
@jruo
Copy link
Contributor

jruo commented Aug 12, 2015

Resolved conflicts and tests pass.

@mpartel
Copy link
Member

mpartel commented Aug 12, 2015

This doesn't preserve old URLs does it? This will almost certainly cause strange errors to users for a while after the update, until their clients refresh the course data. If we go ahead with this, the update should be done late at night IMO.

@jamo
Copy link
Member

jamo commented Aug 12, 2015

Good point, I was already thinking on it:
Yes, I was thinking whether we really would like to keep supporting the use of id for these, and I didn't come up with any good reasons; thus I'd say only using name as the route param would be fine.

And without any changes on the server configuration this would most likely require all tmc-netbeans users to refresh the course list.
But I was thinking on generating apache url rewrite map to temporarily support old urls. Though I'm not yet sure whether it is worth generating that file...

@mpartel
Copy link
Member

mpartel commented Aug 12, 2015

Ah, good point. IMO requiring a course list refresh from each user is unacceptable. One option might be to patch the client to do this automatically if the course URL returns a 4xx.

I'd really like to avoid more "temporary" server-side hacks if possible.

@jamo
Copy link
Member

jamo commented Aug 12, 2015

Yea, we definitely should not break the behavior with our server side changes. :)
That patch for the client would probably make sense, but first forcing an update out and then making this change seems to be of bigger cost, than the rewrite map this time. And we definitely should be able to remove the rewrite map in a week or so.

I'v suggested adding this behavior to tmc-core testmycode/tmc-core#39

@jamo
Copy link
Member

jamo commented Sep 15, 2015

I feel that it might just make more sense to keep on supporting our current routing schemes for a while; thus we should continue working on this to allow both kind of urls to be used for routing.

@jamo jamo force-pushed the master branch 2 times, most recently from c8fc4d0 to 2d7423c Compare October 27, 2015 12:59
@nygrenh nygrenh force-pushed the master branch 2 times, most recently from dcd24ed to d5cd6cb Compare July 3, 2017 11:37
@nygrenh nygrenh closed this Apr 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants