Skip to content
This repository has been archived by the owner on Jan 6, 2025. It is now read-only.

I24 permission protection + metadata on course-evaluation page #113

Merged
merged 17 commits into from
Mar 2, 2021

Conversation

frinzekt
Copy link
Member

@frinzekt frinzekt commented Feb 17, 2021

Closes #24, Closes #71

Includes:

  • permission protection for the backend
  • change frontend that needs coordinators/reviewer details to use only the couse-evalution service
  • server console log linting

@frinzekt
Copy link
Member Author

One of the things I realized while I was doing this

change frontend that needs coordinators/reviewer details to use only the couse-evalution service (WIP)

is that if I bind details of coordinators/reviewers in the course-evaluation state, we lose the ability for instant interface change effect. We have a couple of options to address this problem:

  • reupdate the entire course-evaluation state everytime you modify user information (costly in terms of loading the entire course-evaluation)
  • update how the sockets onCreate/onPatch works... by identifying whether we need to modify courseEvaluation data (how it would work is that we have a function in the onCreate/onPatch to see get all permission for the course and compare whether it is still the same. If it is not the same, then do update the courseEvaluation state by dispatch ... [this is some complexity because we have never ever done a dispatch towards a feathersService state before).

What do you think @MouseAndKeyboard ?

@frinzekt
Copy link
Member Author

frinzekt commented Feb 17, 2021

Note: this needs to be thoroughly tested. Mistakes in this pull request will prevent any reviewer or coordinator from doing what they are supposed to do. Ideally, no existing features will be affected.

You have to check this in the frontend by using an account that only has "just enough" permission to do the task. Eg. if you are doing a review, you should be able to do it with just the reviewer permission... and so on for other roles except administrator

@frinzekt
Copy link
Member Author

I have raised #115 to address changing interface state when updating user permission using "event-based" sockets

@frinzekt frinzekt changed the title I24 permission protection I24 permission protection + metadata on course-evaluation page Feb 17, 2021
@frinzekt frinzekt marked this pull request as ready for review February 17, 2021 16:57
Copy link
Collaborator

@MouseAndKeyboard MouseAndKeyboard left a comment

Choose a reason for hiding this comment

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

Looks good!

const response = await services['users'].patch(userId, {
perms: newPerms,
services['users'].patch(userId, {
$pull: { perms: {course_id:evaluationId,role:'Reviewer'}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Tested and working

patch: [authenticate('jwt')],
remove: [authenticate('jwt')],
update: [authenticate('jwt'), roleBasedRestrictions(['Administrator'])], // Only Admin
patch: [authenticate('jwt'), roleBasedRestrictions(['Coordinator'])], // Only Coordinator + Admin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this supposed to be:
patch: [authenticate('jwt'), roleBasedRestrictions(['Coordinator', 'Administrator'])],

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nvm, I can see that the in role-based-restrictions.js it short-circuits for anyone with administrator

@MouseAndKeyboard
Copy link
Collaborator

Currently just finalising testing, will merge when complete

@frinzekt
Copy link
Member Author

frinzekt commented Mar 2, 2021

automated regression testing would have been great for this kind of things. However, I just got busy from setting it up. There are a couple of hurdles such as:

  • how do I test using google Oauth. My thoughts at the moment is to implement local username/password as a backup feature. It exist, but shouldn't be used idk

@MouseAndKeyboard MouseAndKeyboard merged commit ea01853 into develop Mar 2, 2021
@frinzekt
Copy link
Member Author

frinzekt commented Mar 3, 2021

for future reference, use squash merge for feature branches to develop @MouseAndKeyboard

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read Only View Non-Existing User Error Handling Permission Protection / Backend Validation
2 participants