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

Added public and private class hashes [#138853633] #288

Merged
merged 1 commit into from Feb 21, 2017
Merged

Conversation

dougmartin
Copy link
Member

Adds a public and private class hash. The public hash can be used with a new API endpoint at api/v1/classes/info?public_class_hash=<hash> to retrieve the class name, the students in the class (email and name) and the private class hash. The endpoint is secured by a combination of the random public class hash and a check to ensure the user is either a teacher or student in the class.

The private class hash was created to use as part of a class key that Sharinator uses with Firebase. The Sharinator Dashboard is passed a class=<base64encode(api/v1/classes/info?public_class_hash=<hash>)> parameter that it decodes and then calls to get the private class hash. This allows the Dashboard url to be shared but the data to be protected when the user of the shared url is not a teacher or student of the class.

@dougmartin dougmartin force-pushed the add-class-hash branch 3 times, most recently from 810bb81 to 2b8a5f7 Compare February 9, 2017 14:30
@scytacki scytacki self-requested a review February 9, 2017 17:16
@scytacki
Copy link
Member

scytacki commented Feb 9, 2017

I like the idea of putting a private key behind a secure endpoint. Did you check what happens to this private key when the class is copied, it probably shouldn't be copied too.

I don't think the public key is worth it. I think just using the class id as long as it requires an authorized user to access it. In this case the route could just be:
apii/v1/classes/[class_id]
Also see below about anonymous access with the classword which kind of overlaps with the public key concept.

Did you happen to find this api that I added before:
https://github.com/concord-consortium/rigse/blob/master/app/controllers/portal/clazzes_controller.rb#L745

It would be good to include that functionality in the new api endpoint and then we can deprecate the old one I added. So the additional functionality is:

  • support anonymous access as long as the classword is passed in ?class_word=[class_word]
  • add the class uri, state, and teachers

I also wonder why you didn't use pundit? We have similar access checks in the offerings controller: http://github.com/concord-consortium/rigse/blob/master/app/policies/portal/offering_policy.rb#L75-L76

Copy link
Member

@scytacki scytacki left a comment

Choose a reason for hiding this comment

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

I added comment above about several points. This review box is too small for large comments.

@scytacki
Copy link
Member

@dougmartin what about using Pundit? If it is pain it isn't necessary, but if it is simple then it seems good for consistency.

Also I don't think you need a separate info action and route. The show action can support taking a classword as a parameter (that would be moved into pundit if you switch to it).

@scytacki scytacki modified the milestone: v1.16.0 Feb 14, 2017
@dougmartin
Copy link
Member Author

I didn't use pundit because most of the API controllers don't and I had the separate branch going with the pundit changes in the api controller.

@scytacki
Copy link
Member

Looks good. I realized that the info route is necessary because the clients calling it won't know the id of the class they want info on.

Also I was concerned this would have problems with the new CSRF token enabling that Piotr has done. However, these are both GET requests which shouldn't using the CSRF token.

@scytacki scytacki merged commit 0720e75 into master Feb 21, 2017
@scytacki scytacki deleted the add-class-hash branch February 21, 2017 18:33
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.

None yet

2 participants