Security domain doesn't match every field in security context for find operations?

[Atlinx]

Problem

I have this security setting for one of my models:

...
userRole: {
    domain: {
      ...EMPTY_SECURITY_DOMAIN,
      roleCode: "roleCode",
      userId: "userId",
    },
    permissions: {
      MANAGE_USER_ROLES: PERMISSION.ALLOW,
    }
  },
}
...

I have this security context generated for a query

{
  ...
  MANAGE_USER_ROLES: [
    {
      userId: "628f7deaf6c13488b29797a0",
      roleCode: "USER",
    },
  ],
  ...
}

And this is the operation domain, which is specified by the user on every request

{
  securityDomain: {
    userId: [
      "628f7deaf6c13488b29797a0",
    ],
    roleCode: [
      "USER",
    ],
  },
}

However, I'm allowed to query for user roles whose roleCode matches but whose userId doesn't match. Additionally, users are able to update their existing roles to a higher roleCode, despite the roleCode not existing in their securityContext. Finally, users can delete roles they have from others.

Summary

BROKEN
Users can query for roles that aren't theirs.

query {
  userRoles {
    id
    roleCode
    userId 
  }
}

{
  "data": {
    "userRoles": [
      {
        "id": "628d7c338d18b842c3d3a55c",
        "roleCode": "USER",
        "userId": "628d7c328d18b842c3d3a55b"
      },
      {
        "id": "628f7deaf6c13488b29797a1",
        "roleCode": "USER",
        "userId": "628f7deaf6c13488b29797a0"
      }
    ]
  }
}

BROKEN
Users can promote their roles by updating their roleCode to something that isn't in their security context:

mutation($filter: UserRoleFilterInput!, $changes: UserRoleUpdateInput!) {
  updateUserRoles(filter: $filter, changes: $changes)
}

{
  "filter": {
    "userId": { "eq": "628f7deaf6c13488b29797a0" }
  },
  "changes": {
    "roleCode": "SUPER_ADMIN",
  }
}

{
  "data": {
    "updateUserRoles": true
  }
}

BROKEN
Users can delete all roles that match only the roleCode, meaning they can remove their own roles from others

mutation($deleteUserRolesFilter2: UserRoleFilterInput!) {
  deleteUserRoles(filter: $deleteUserRolesFilter2)
}

"deleteUserRolesFilter2": {
  "roleCode": { "eq": "USER" }
}

{
  "data": {
    "deleteUserRoles": true
  }
}

WORKS
User cannot create a new role that's not in their security context

mutation($record: UserRoleInsertInput!) {
  createUserRole(record: $record) {
    id
  }
}

{
  "record": {
    "roleCode": "SUPER_ADMIN",
     "userId": "628f7deaf6c13488b29797a0"
  },
}

{
  "errors": [
    {
      "message": "[Security Policy Middleware] Unauthorized write.",
      ...
    }
  ]
  ...
}

[edobrb]

Indeed, the filter built by SecurityDomain works in such a way as to put the various domains in $or. In your case the filter from your SecurityDomain

{
  securityDomain: {
    userId: [
      "628f7deaf6c13488b29797a0",
    ],
    roleCode: [
      "USER",
    ],
  },
}

will be built in:

{ $or: [{ userId: { $in: ["628f7deaf6c13488b29797a0"] } }, { roleCode: { $in: ["USER"] } }] }

I guess that makes more sense to built it between $and to allow for greater expressiveness:

{ $and: [{ userId: { $in: ["628f7deaf6c13488b29797a0"] } }, { roleCode: { $in: ["USER"] } }] }

As for update and delete: it is correct that the operation is successful because we do not check the original filter, however the original filter is put in $and with the filter built by the SecurityDomain. The effect that you are able to modify or delete only the entities to which you have access, btw no errors will be thrown.

[edobrb]

Yes, it should prevent the update because the filter now is stricter. Anyway no errors will be thrown in update and delete operations.

[Atlinx]

I've run some tests and everything works, except for the update operation. Users can still promote themselves to a higher role, despite not having the permissions to read or write that role in their security context.

[edobrb]

This is strange because the update uses the same logic as the delete.

typetta/src/dal/dao/middlewares/securityPolicy/security.middleware.ts

Lines 147 to 152 in be9ff5e

	if (args.operation === 'update') {
	if (!crud.update) {
	throw new SecurityPolicyUpdateError({ permissions: relatedSecurityContext.map((policy) => [policy.permission, policy.domain]), operationDomains: operationSecurityDomain })
	}
	return { continue: true, operation: 'update', params: { ...args.params, filter } }
	}

VS

typetta/src/dal/dao/middlewares/securityPolicy/security.middleware.ts

Lines 161 to 166 in be9ff5e

	if (args.operation === 'delete') {
	if (!crud.delete) {
	throw new SecurityPolicyDeleteError({ permissions: relatedSecurityContext.map((policy) => [policy.permission, policy.domain]), operationDomains: operationSecurityDomain })
	}
	return { continue: true, operation: 'delete', params: { ...args.params, filter } }
	}

I'll investigate the problem in the next few days, thanks for reporting.

[Atlinx]

I'm not too familiar with the Typetta internals but maybe you need to have a check for whether the changes for the update operation abide by the permissions?

[Atlinx]

@edobrb Would it be possible for Typetta to throw an error when the user tries to update/delete an entity that's outside of their permissions? Returning a success message makes it a bit misleading.

[minox86]

In other words, a user should not be able to update fields that are in at least one security domain of an entity. It makes sense to me, but we can check if it is general enough to be added to the framework. Il Ven 27 Mag 2022, 18:54 Atlinx ***@***.***> ha scritto:

…

I'm not too familiar with the Typetta internals but maybe you need to have a check for whether the changes for the update operation abide by the permissions? — Reply to this email directly, view it on GitHub <#214 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAD7YBAVBWVGSHB3JFZMD2DVMD43ZANCNFSM5XCVFO3Q> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[Atlinx]

I'm not really sure what "in at least one security domain of an entity" means. I need to restrict the possible values a user can set for their roles, but I understand it might be too niche/specific if you hack it into the existing security system.

I've made another discussion post (#220) pitching the idea of a validation function, which should fix the broader issue of controlling the acceptable values of a field.

[edobrb]

Now that I've understand your problem more in depth I think that is too specific to justify an update to the security feature of Typetta. As you mentioned it could be solved with a validation middleware. Let us know if there is any other problem.