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

85 complete item 1b of security improvement #121

Closed
wants to merge 28 commits into from

Conversation

Ye-Tien
Copy link

@Ye-Tien Ye-Tien commented Oct 1, 2023

Describe your changes

 Update role checked endpoints which have access to the resource and be annotated with @is_user_or_has_role or @has_role

Issue ticket number and link

Role checked endpoints audited to determine who should have access to the resource, GET: required, DELETE: required (admin), POST: required (admin)
register POST: not required

login POST: not required

send_code POST: not required

verify_code POST: not required

reset_password POST: required
GET: required (admin)

PATCH: required (admin)

DELETE: required (admin)
POST: required (admin)

DELETE: required (admin)
editProfile POST: required

getProfile POST: required
class RoleRequest(Resource):
GET: required (admin)
POST: required (admin)
PATCH: required (admin)
DELETE: required (admin)

class QualificationsRequest(Resource):
GET: required (admin)
POST: required (admin)
PATCH: required (admin)
DELETE: required (admin)

class AssetTypeRequest(Resource):
GET: required (admin)
POST: required (admin)
PATCH: required (admin)
DELETE: required (admin)
GET: required (admin)

DELETE: required (admin)

PATCH: required (admin)
class TenancyConfig(Resource):
GET: not required
POST: required (admin)
PATCH: required (admin)
DELETE: required

imageRequest GET: not required
GetQuestionRequest GET: required 

GetRandomQuestionRequest GET: required 

DeleteQuestion GET: required (admin)

CreateQuestion GET: required (admin)

UpdateQuestion GET: required (admin)

CheckAnswer GET: required

CheckSingleAnswer GET: required

CheckMulitpleAns GET: required
GetUserInfoRequest GET: required

GetAvailabilitiesRequest GET: required

GetAllVolunteer GET: required (admin)
GET: required (admin)

POST: required (admin)

PATCH: required (admin)
GET: required

PATCH: required (admin)
VehicleRequest

GET: required (admin)

POST: required (admin)

DELETE: required (admin)

 

GetInputA

GET: required (admin)

 

GetInputR

GET: required (admin)

 

GetInputP

GET: required (admin)

 

GetInputV

GET: required (admin)

 

GetInputQ

GET: required (admin)

 

GetVehicleRequest

Currently used by developers
GET: required

POST: required

GET: required
editProfile POST: required

getProfile POST: required
dietary_option GET: need to confirm if this API is being used before the user is registered

POST: required

dietary_requirement GET: required
GET: required

DELETE: required (admin)

POST: required (admin)
register POST: not required

login POST: not required

send_code POST: not required

verify_code POST: not required

reset_password POST: required
class TenancyConfig(Resource):

GET: not required
POST: required (admin)
PATCH: required (admin)
DELETE: required (admin)

imageRequest GET: not required
@Ye-Tien Ye-Tien linked an issue Oct 1, 2023 that may be closed by this pull request
1 task
reset_password POST: not required
@@ -84,6 +85,7 @@ def post(self):
This is a class to retrieve the dietary requirement data from the database
"""
@requires_auth
@is_user_or_has_role('id', UserType.VOLUNTEER, UserType.ROOT_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 correct? The ID parameter should be the user ID. It looks like all these are just the ID of the thing

Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested this with a volunteer user?

Copy link
Collaborator

@emilymclean emilymclean left a comment

Choose a reason for hiding this comment

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

I think all of the usages of is_user_or_has_role that have 'id' as the userId parameter are incorrect. Please audit and verify/test with a volunteer account.

@@ -84,6 +85,7 @@ def post(self):
This is a class to retrieve the dietary requirement data from the database
"""
@requires_auth
@is_user_or_has_role('id', UserType.VOLUNTEER, UserType.ROOT_ADMIN)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested this with a volunteer user?

@@ -79,6 +78,8 @@ def post(self):


class reset_password(Resource):

@is_user_or_has_role('id', UserType.VOLUNTEER, UserType.ROOT_ADMIN)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be guarded

@@ -29,14 +29,16 @@

class AssetTypeRole(Resource):
@requires_auth
@is_user_or_has_role('id', UserType.VOLUNTEER, UserType.ROOT_ADMIN)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This data isn't scoped to the user, and I don't see why we should even care about role for this GET?

@@ -36,6 +36,7 @@

class EditProfile(Resource):
@requires_auth
@is_user_or_has_role('id', UserType.VOLUNTEER, UserType.ROOT_ADMIN)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, is this the right parameter?

@@ -30,6 +30,7 @@

class GetQuestionRequest(Resource):
@requires_auth
@is_user_or_has_role('id', UserType.VOLUNTEER, UserType.ROOT_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 data scoped to the user, and is this the right parameter?

@@ -18,6 +18,7 @@

class GetUserInfoRequest(Resource):
@requires_auth
@is_user_or_has_role('id', UserType.VOLUNTEER, UserType.ROOT_ADMIN)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same with these, right parameter?

@@ -26,12 +26,14 @@

class UserRole(Resource):
@requires_auth
@has_role(UserType.ROOT_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 a volunteer supposed to be able to get/write to these parameters? If not, this is fine

@emilymclean
Copy link
Collaborator

This obviously wasn't tested even once because you would have realised the role check decorator doesn't work like that

@emilymclean emilymclean closed this Mar 3, 2024
@fishchimp fishchimp deleted the 85-complete-item-1b-of-security-improvement branch April 28, 2024 06:30
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.

Complete Item 1B of security improvement
2 participants