ACL matching issue

[ela34]

I'm using the ACL file below.
If I have a JWT with role set to '3p' and the route I'm trying to access is '/api/4p/login' then I'm authorized to access this route.
My understanding is that I should not be authorized. Is it an issue with my ACL?

[
	{
		"group": "3p",
		"permissions": [
			{
				"resource": "/api/3p/*",
				"methods": "*",
				"action": "allow"
			}
		]
	},
	{
		"group": "4p",
		"permissions": [
			{
				"resource": "/api/4p/*",
				"methods": "*",
				"action": "allow"
			}
		]
	},
]

[ela34]

"api/4p/login" is not a subroute of "api/3p/*". It's just a different route.
So when my role is set to '3p' only ACL rules from the '3p' groups should apply and as such access to 'api/4p/login' should be denied. At least this is my understanding, I do not understand what I'm getting wrong...

[nyambati]

I am currently trying to replicate your issue and it seems like I am getting different results. I might have to look at your implementation to be able to figure out why this might be the case. Hope you would not mind pairing.

The only possible reason I could think of is that you have not applied the acl.authorize middleware or the middleware is not within the scope of the routes you have specified. Which means that the routes resolve before hitting the middleware.

[ela34]

In common.js around line 150, it seems that the first items in resourceArray are not checked against the first items in routeArray, so I end up in:

// if the length of resource is less than route
if (routeArray.length > resourceArray.length) {
  return resourceArray.includes('*');
}

returning true.

With resourceArray being:

[
  "api",
  "4p",
  "*"
]

And routeArray being:

[
  "api",
  "3p",
  "v1",
  "device"
]

[ela34]

I modified matchUrlToResource as follow (with test code) and now it behaves correctly from my understanding of the rules definition. Can you check?

const urlToArray = url => {
	if (typeof url !== 'string') {
		throw new Error('Only string arguments are allowed');
	}
	return url.replace(/^\/+|\/+$/gm, '').split('/');
};

const createRegexFromResource = resource => {
	if (resource.startsWith(':') || resource === '*') {
		return '.*';
	}
	return `^${resource}$`;
};

const matchUrlToResource = (route, resource) => {
	//  create an array form both route url and resource
	const routeArray = urlToArray(route);
	const resourceArray = urlToArray(resource);

	for (let i = 0; i < routeArray.length; i++) {
		if (i < resourceArray.length) {
			if (resourceArray[i] === '*')
				return true;
			if (!routeArray[i].match(createRegexFromResource(resourceArray[i])))
				return false;
		} else
			return false;
	}
	if (resourceArray.length > routeArray.length) {
		if (resourceArray[routeArray.length] !== '*')
			return false;
	}

	return true;
};

function check(route, resource, res) {
	const result = matchUrlToResource(route, resource)
	if (res !== result)
		console.log('Error: check', route, 'against', resource, 'returned', result, 'instead of', res)
	else
		console.log('   Ok: check', route, 'against', resource, 'returned', result)
}

check('api/3p/v1/device', 'api/4p/*', false)		// route, resource
check('api/3p/v1/device', 'api/3p/*', true)
check('api/3p/v1/device', 'api/3p/v1', false)
check('api/3p/v1/device', 'api/3p/:id/device', true)
check('api/3p/v1/device', 'api/3p/:id/bug', false)
check('api/3p/v1/device', '*', true)
check('api', 'api/4p/*', false)
check('api/4p', 'api/4p/*', true)
check('', 'api/4p/*', false)
check('api/4p', '*', true)
check('api/4p', '', false)

[nyambati]

Let me review this and I will get back to you.

[nyambati]

@ela34 I have added your implementation and made a release

[ela34]

Thanks!