Test case fail but behaviour seams correct

[brunoslalmeida]

This test case is failing:

it('Restricts access to protected route for multiple authorized roles that are not defined as plugin roles', (done) => {
				const server = new Hapi.Server();
				
				server.auth.scheme('custom', internals.authSchema);
				server.auth.strategy('default', 'custom', {});

				server.route({ method: 'GET', path: '/', options: {
					auth: 'default',
					plugins: {'hapiAuthorization': {roles: ['USER', 'ADMIN']}},
					handler: (request, h) => { return "Authorized";}
				}});
				server.register(plugin, {}).then(() => {
					server.inject({method: 'GET', url: '/', credentials: {role: 'ADMIN'}}).then((res) => {
						internals.asyncCheck(() => {
							expect(res.payload).to.equal('Authorized');
							expect(res.statusCode).to.equal(200);
						}, done);
					});
				});
			});

options this time have this:

const plugin = {
			name: 'hapiAuthorization',
			version: '0.0.0',
			plugin: Plugin.plugin,
			path: libpath,
			options: {
				roles: ['OWNER', 'MANAGER', 'EMPLOYEE'],
				hierarchy: true,
				roleHierarchy: ['OWNER', 'MANAGER', 'EMPLOYEE']
			}
		};

two roles is being add to this route specifically.

plugins: {'hapiAuthorization': {roles: ['USER', 'ADMIN']}} 

The problems is in isGranted.

internals.isGranted = function(userRole, requiredRole, hierarchy) {

	// If we're using a hierarchy, get all the possible roles
	if(hierarchy) {
    const index = hierarchy.indexOf(userRole);	// Get the index of userRole in the hierarchy

		// If the user's role is not any of the possible roles
		if (index === -1) {
			return false;
		}

		userRoles = _.rest(hierarchy, index);	// Get all the possible roles in the hierarchy
	} else {
		userRoles = userRole;
	}

At this point userRole = ADMIN, requiredRole = [ 'USER', 'ADMIN' ] and *hierarchy = [ 'OWNER', 'MANAGER', 'EMPLOYEE' ]. As the code goes into the if(hierarchy), the next step is return false, because neither user or admin ara inside hierarchy array.

To solve this it's is possible to check requiredRole first or to add requiredRole to hierarchy. I am not sure how to add it to hierarchy (at the begin or at the end).

This happens because hierarchy was always undefined. When i fixed that error this shows up.

Whats your suggestion ?

[aquelatecnologia]

Any comment?

[RicardoRdzG]

I don't get what is the issue.
the first thing you posted is a self contained test
the second one is a plugin configuration that is independent of the self contained test so one can not affect the other.
Then you said you add a plugin configuration for the route that overrides the second configuration.
In that route configuration you are requiring roles that are not defined at your plugin configuration level.
That test would not pass if you are sending a request with a role that doesn't match your route configuration.

[brunoslalmeida]

The issue is that the test case expect a success not a failure. All this code was taken from the current master branch.

[RicardoRdzG]

but what is the use case? it doesn't make sense that you expect the test to pass if you didn't set up your roles and hierarchy correctly in the beginning. A user request should not pass if the role for the route is not in the plugin configuration. The roles and hierarchy should be defined first at plugin level so that evaluation can occur. At route level you are not adding roles to configuration, you are specifying which ones are allowed to access it.

[brunoslalmeida]

You got it. That's it. The issue is ... the test result is wrong, it should not pass but fail.

[brunoslalmeida]

The actual test case is expecting to pass but it expect a failure. Just take a look tests

[brunoslalmeida]

I was questioning the author if the test should fail or if should be checked the specific role before hierarchy. As he is the one accepting PR it doesn't really matters what I think.

[RicardoRdzG]

I get it now but seems that with your fix the behaviour must be changed. According to that test the hierarchy should just add all the roles below in the hierarchy to the user roles and not fail if the user role is not in it

[aquelatecnologia]

I will try some changes on monday and then i will make an other PR.

[RicardoRdzG]

sent a pr fixing the failing test to your repo