Repository: osmlab/maproulette3
Pull request #1257 (2020-06-05 00:20:04)

Incorporate grants on project objects
* When determining the roles a user has on a project, combine the
grants on both the user object and the project object to help prevent
erroneous security errors from stale data

* Refresh the user data after a new project is created, as that user's
grants have almost certainly changed

* Remove call to server assigning project creator an admin role after
project creation, as the server now does that automatically


Patch:
diff --git a/src/components/AdminPane/HOCs/WithManageableProjects/WithManageableProjects.js b/src/components/AdminPane/HOCs/WithManageableProjects/WithManageableProjects.js
index 3198ec54..99dcfa2c 100644
--- a/src/components/AdminPane/HOCs/WithManageableProjects/WithManageableProjects.js
+++ b/src/components/AdminPane/HOCs/WithManageableProjects/WithManageableProjects.js
@@ -117,7 +117,7 @@ const mapStateToProps = state => ({
   entities: state.entities
 })
 
-const mapDispatchToProps = dispatch => {
+const mapDispatchToProps = (dispatch, ownProps) => {
   const actions = bindActionCreators({
     fetchManageableProjects,
     fetchProject,
@@ -142,7 +142,7 @@ const mapDispatchToProps = dispatch => {
 
   actions.toggleProjectEnabled = project => {
     const updatedProject = Object.assign({}, project, {enabled: !project.enabled})
-    return dispatch(saveProject(updatedProject))
+    return dispatch(saveProject(updatedProject, ownProps.user))
   }
 
   return actions
diff --git a/src/components/AdminPane/Manage/EditProject/EditProject.js b/src/components/AdminPane/Manage/EditProject/EditProject.js
index 831d7f0e..3be3dbde 100644
--- a/src/components/AdminPane/Manage/EditProject/EditProject.js
+++ b/src/components/AdminPane/Manage/EditProject/EditProject.js
@@ -50,7 +50,7 @@ export class EditProject extends Component {
         formData.name = _snakeCase(formData.displayName).replace(/^home_/, 'project_')
       }
 
-      this.props.saveProject(formData).then(project => {
+      this.props.saveProject(formData, this.props.user).then(project => {
         if (project) {
           this.props.history.push(`/admin/project/${project.id}`)
         }
diff --git a/src/interactions/User/AsManager.js b/src/interactions/User/AsManager.js
index 89a115fd..3c8f147f 100644
--- a/src/interactions/User/AsManager.js
+++ b/src/interactions/User/AsManager.js
@@ -1,12 +1,14 @@
 import { Role, rolesImply, ROLE_SUPERUSER }
        from '../../services/Grant/Role'
 import { TargetType } from '../../services/Grant/TargetType'
+import { GranteeType } from '../../services/Grant/GranteeType'
 import _get from 'lodash/get'
 import _map from 'lodash/map'
 import _isObject from 'lodash/isObject'
 import _filter from 'lodash/filter'
 import _isFinite from 'lodash/isFinite'
 import _each from 'lodash/each'
+import _uniq from 'lodash/uniq'
 import { AsEndUser } from './AsEndUser'
 
 /**
@@ -37,13 +39,25 @@ export class AsManager extends AsEndUser {
       return []
     }
 
-    return _map(
-      _filter(this.user.grants, grant =>
-        grant.role === ROLE_SUPERUSER ||
-        (grant.target.objectType === TargetType.project && grant.target.objectId === project.id)
-      ),
-      'role'
+    // Combine the grants on the user with those on the project. This is
+    // potentially more lenient, but helps prevent erroneous security errors in
+    // the event of stale data (and the server will stop anything if the user
+    // actually lacks permission)
+    const userGrants = _filter(this.user.grants, grant =>
+      grant.role === ROLE_SUPERUSER || (
+        grant.target &&
+        grant.target.objectType === TargetType.project &&
+        grant.target.objectId === project.id
+      )
+    )
+
+    const projectGrants = _filter(project.grants, grant =>
+      grant.grantee &&
+      grant.grantee.granteeType === GranteeType.user &&
+      grant.grantee.granteeId === this.user.id
     )
+
+    return _uniq(_map(userGrants.concat(projectGrants), 'role'))
   }
 
   /**
diff --git a/src/services/Project/Project.js b/src/services/Project/Project.js
index 9f2d50c2..eb7c814a 100644
--- a/src/services/Project/Project.js
+++ b/src/services/Project/Project.js
@@ -13,11 +13,9 @@ import RequestStatus from '../Server/RequestStatus'
 import genericEntityReducer from '../Server/GenericEntityReducer'
 import { RECEIVE_CHALLENGES } from '../Challenge/ChallengeActions'
 import { RESULTS_PER_PAGE } from '../Search/Search'
-import { Role } from '../Grant/Role'
-import { addServerError,
-         addError } from '../Error/Error'
+import { addServerError, addError } from '../Error/Error'
 import AppErrors from '../Error/AppErrors'
-import { findUser, ensureUserLoggedIn } from '../User/User'
+import { findUser, ensureUserLoggedIn, fetchUser } from '../User/User'
 
 /** normalizr schema for projects */
 export const projectSchema = function() {
@@ -193,7 +191,7 @@ export const searchProjects = function(searchCriteria, limit=RESULTS_PER_PAGE) {
  * whether it already has an id) and update the redux store with the latest
  * version from the server.
  */
-export const saveProject = function(projectData) {
+export const saveProject = function(projectData, user) {
   return function(dispatch) {
     // Setup the save endpoint to either edit or create the project depending
     // on whether it has an id.
@@ -212,11 +210,10 @@ export const saveProject = function(projectData) {
       dispatch(receiveProjects(normalizedResults.entities))
       const project = _get(normalizedResults, `entities.projects.${normalizedResults.result}`)
 
-      // If we just created the project, add the owner as an admin.
+      // If we just created the project, we should refresh the user as they
+      // almost certainly have new grants
       if (areCreating && project) {
-        return setProjectManagerRole(
-          project.id, project.owner, true, Role.admin
-        )(dispatch).then(() => project)
+        return fetchUser(user.id)(dispatch).then(() => project)
       }
       else {
         return project


Test patch:
diff --git a/src/interactions/User/AsManager.test.js b/src/interactions/User/AsManager.test.js
index 6efe6e12..f261f851 100644
--- a/src/interactions/User/AsManager.test.js
+++ b/src/interactions/User/AsManager.test.js
@@ -1,7 +1,9 @@
 import AsManager from './AsManager'
+import _cloneDeep from 'lodash/cloneDeep'
 import { GUEST_USER_ID } from '../../services/User/User'
 import { Role, ROLE_SUPERUSER } from '../../services/Grant/Role'
 import { TargetType } from '../../services/Grant/TargetType'
+import { GranteeType } from '../../services/Grant/GranteeType'
 
 const superGrant = {id: -1, role: ROLE_SUPERUSER}
 const adminGrant123 = {id: 123, role: Role.admin, target: {objectType: TargetType.project, objectId: 123}}
@@ -9,9 +11,9 @@ const readGrant456 = {id: 456, role: Role.read, target: {objectType: TargetType.
 const adminGrant789 = {id: 789, role: Role.admin, target: {objectType: TargetType.project, objectId: 789}}
 const adminGrant101 = {id: 101, role: Role.admin, target: {objectType: TargetType.project, objectId: 101}}
 const writeGrant234 = {id: 234, role: Role.write, target: {objectType: TargetType.project, objectId: 234}}
-const adminGrant102 = {id: 102, role: Role.admin, target: {objectType: TargetType.project, objectId: 102}}
-const writeGrant102 = {id: 1021, role: Role.write, target: {objectType: TargetType.project, objectId: 102}}
-const readGrant102 = {id: 1022, role: Role.read, target: {objectType: TargetType.project, objectId: 102}}
+const adminGrant102 = {id: 102, role: Role.admin, target: {objectType: TargetType.project, objectId: 102}, grantee: {granteeType: GranteeType.user, granteeId: 246}}
+const writeGrant102 = {id: 1021, role: Role.write, target: {objectType: TargetType.project, objectId: 102}, grantee: {granteeType: GranteeType.user, granteeId: 246}}
+const readGrant102 = {id: 1022, role: Role.read, target: {objectType: TargetType.project, objectId: 102}, grantee: {granteeType: GranteeType.user, granteeId: 246}}
 const groupAdminGrant987 = {id: 9871, role: Role.admin, target: {objectType: TargetType.group, objectId: 987}}
 const groupWriteGrant987 = {id: 9872, role: Role.write, target: {objectType: TargetType.group, objectId: 987}}
 
@@ -25,6 +27,7 @@ const project456 = {id: 456, grants: [readGrant456]}
 const project789 = {id: 789, grants: [adminGrant789]}
 const project101 = {id: 101, grants: [adminGrant101], owner: 987654321}
 const project102 = {id: 102, grants: [adminGrant102, writeGrant102, readGrant102]}
+const project103 = {id: 103, grants: []}
 
 const challenge123_1 = {id: 1231, parent: 123}
 const challenge123_2 = {id: 1232, parent: 123}
@@ -35,9 +38,11 @@ const group987 = {id: 987}
 
 describe('projectRoles', () => {
   it("returns the project roles possessed by the user", () => {
+    const project = _cloneDeep(project102)
+    project.grants = []
     const manager = AsManager(powerUser)
 
-    const roles = manager.projectRoles(project102)
+    const roles = manager.projectRoles(project)
     expect(roles).toContain(Role.admin)
     expect(roles).toContain(Role.read)
     expect(roles).not.toContain(Role.write)
@@ -50,6 +55,17 @@ describe('projectRoles', () => {
     expect(roles).toContain(ROLE_SUPERUSER)
   })
 
+  it("includes the user's granted roles from the project", () => {
+    const user = _cloneDeep(powerUser)
+    user.grants = []
+    const manager = AsManager(user)
+
+    const roles = manager.projectRoles(project102)
+    expect(roles).toContain(Role.admin)
+    expect(roles).toContain(Role.read)
+    expect(roles).toContain(Role.write)
+  })
+
   it("returns an empty list for a user possessing none of the project's roles", () => {
     const manager = AsManager(powerUser)