Skip to content

Commit 2f94fbb

Browse files
NoNameProvidedNoNameProvided
authored
Merge pull request #283 from 19majkel94/fix/authorization-checker
Fix sync and async auth checker bug
2 parents 2b0251c + fb2a154 commit 2f94fbb

File tree

3 files changed

+189
-78
lines changed

3 files changed

+189
-78
lines changed

‎src/driver/express/ExpressDriver.ts‎

Lines changed: 19 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -103,23 +103,27 @@ export class ExpressDriver extends BaseDriver {
103103
throw new AuthorizationCheckerNotDefinedError();
104104

105105
const action: Action = { request, response, next };
106-
const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles);
107-
108-
const handleError = (result: any) => {
109-
if (!result) {
110-
let error = actionMetadata.authorizedRoles.length === 0 ? new AuthorizationRequiredError(action) : new AccessDeniedError(action);
111-
return this.handleError(error, actionMetadata, action);
106+
try {
107+
const checkResult = this.authorizationChecker(action, actionMetadata.authorizedRoles);
108+
109+
const handleError = (result: any) => {
110+
if (!result) {
111+
let error = actionMetadata.authorizedRoles.length === 0 ? new AuthorizationRequiredError(action) : new AccessDeniedError(action);
112+
this.handleError(error, actionMetadata, action);
113+
} else {
114+
next();
115+
}
116+
};
117+
118+
if (isPromiseLike(checkResult)) {
119+
checkResult
120+
.then(result => handleError(result))
121+
.catch(error => this.handleError(error, actionMetadata, action));
112122
} else {
113-
next();
123+
handleError(checkResult);
114124
}
115-
};
116-
117-
if (isPromiseLike(checkResult)) {
118-
checkResult
119-
.then(result => handleError(result))
120-
.catch(error => this.handleError(error, actionMetadata, action));
121-
} else {
122-
handleError(checkResult);
125+
} catch (error) {
126+
this.handleError(error, actionMetadata, action);
123127
}
124128
});
125129
}

‎src/driver/koa/KoaDriver.ts‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -77,25 +77,29 @@ export class KoaDriver extends BaseDriver {
7777
throw new AuthorizationCheckerNotDefinedError();
7878

7979
const action: Action = { request: context.request, response: context.response, context, next };
80-
const checkResult = actionMetadata.authorizedRoles instanceof Function ?
81-
getFromContainer<RoleChecker>(actionMetadata.authorizedRoles).check(action) :
82-
this.authorizationChecker(action, actionMetadata.authorizedRoles);
83-
84-
const handleError = (result: any) => {
85-
if (!result) {
86-
let error = actionMetadata.authorizedRoles.length === 0 ? new AuthorizationRequiredError(action) : new AccessDeniedError(action);
87-
return this.handleError(error, actionMetadata, action);
80+
try {
81+
const checkResult = actionMetadata.authorizedRoles instanceof Function ?
82+
getFromContainer<RoleChecker>(actionMetadata.authorizedRoles).check(action) :
83+
this.authorizationChecker(action, actionMetadata.authorizedRoles);
84+
85+
const handleError = (result: any) => {
86+
if (!result) {
87+
let error = actionMetadata.authorizedRoles.length === 0 ? new AuthorizationRequiredError(action) : new AccessDeniedError(action);
88+
return this.handleError(error, actionMetadata, action);
89+
} else {
90+
return next();
91+
}
92+
};
93+
94+
if (isPromiseLike(checkResult)) {
95+
return checkResult
96+
.then(result => handleError(result))
97+
.catch(error => this.handleError(error, actionMetadata, action));
8898
} else {
89-
next();
99+
return handleError(checkResult);
90100
}
91-
};
92-
93-
if (isPromiseLike(checkResult)) {
94-
return checkResult
95-
.then(result => handleError(result))
96-
.catch(error => this.handleError(error, actionMetadata, action));
97-
} else {
98-
handleError(checkResult);
101+
} catch (error) {
102+
return this.handleError(error, actionMetadata, action);
99103
}
100104
});
101105
}

‎test/functional/auth-decorator.spec.ts‎

Lines changed: 149 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import "reflect-metadata";
22
import {Get} from "../../src/decorator/Get";
3-
import {createExpressServer, createKoaServer, getMetadataArgsStorage} from "../../src/index";
3+
import { createExpressServer, createKoaServer, getMetadataArgsStorage, NotAcceptableError } from "../../src/index";
44
import {assertRequest} from "./test-utils";
55
import {JsonController} from "../../src/decorator/JsonController";
66
import {Authorized} from "../../src/decorator/Authorized";
@@ -9,7 +9,7 @@ import {RoutingControllersOptions} from "../../src/RoutingControllersOptions";
99
const chakram = require("chakram");
1010
const expect = chakram.expect;
1111

12-
describe("Controller responds with value when Authorization succeeds", function () {
12+
describe("Controller responds with value when Authorization succeeds (async)", function () {
1313

1414
before(() => {
1515

@@ -70,6 +70,67 @@ describe("Controller responds with value when Authorization succeeds", function
7070

7171
});
7272

73+
describe("Controller responds with value when Authorization succeeds (sync)", function () {
74+
75+
before(() => {
76+
77+
// reset metadata args storage
78+
getMetadataArgsStorage().reset();
79+
80+
@JsonController()
81+
class AuthController {
82+
83+
@Authorized()
84+
@Get("/auth1")
85+
auth1() {
86+
return { test: "auth1" };
87+
}
88+
89+
@Authorized(["role1"])
90+
@Get("/auth2")
91+
auth2() {
92+
return { test: "auth2" };
93+
}
94+
95+
}
96+
});
97+
98+
const serverOptions: RoutingControllersOptions = {
99+
authorizationChecker: (action: Action, roles?: string[]) => {
100+
return true;
101+
}
102+
};
103+
104+
let expressApp: any;
105+
before(done => {
106+
const server = createExpressServer(serverOptions);
107+
expressApp = server.listen(3001, done);
108+
});
109+
after(done => expressApp.close(done));
110+
111+
let koaApp: any;
112+
before(done => {
113+
const server = createKoaServer(serverOptions);
114+
koaApp = server.listen(3002, done);
115+
});
116+
after(done => koaApp.close(done));
117+
118+
describe("without roles", () => {
119+
assertRequest([3001, 3002], "get", "auth1", response => {
120+
expect(response).to.have.status(200);
121+
expect(response.body).to.eql({ test: "auth1" });
122+
});
123+
});
124+
125+
describe("with roles", () => {
126+
assertRequest([3001, 3002], "get", "auth2", response => {
127+
expect(response).to.have.status(200);
128+
expect(response.body).to.eql({ test: "auth2" });
129+
});
130+
});
131+
132+
});
133+
73134
describe("Authorized Decorators Http Status Code", function () {
74135

75136
before(() => {
@@ -129,48 +190,90 @@ describe("Authorized Decorators Http Status Code", function () {
129190

130191
});
131192

132-
describe("Authorization checker allows to throw", function() {
133-
before(() => {
134-
// reset metadata args storage
135-
getMetadataArgsStorage().reset();
136-
137-
@JsonController()
138-
class AuthController {
139-
@Authorized()
140-
@Get("/auth1")
141-
auth1() {
142-
return { test: "auth1" };
143-
}
144-
145-
}
146-
});
147-
148-
const serverOptions: RoutingControllersOptions = {
149-
authorizationChecker: async (action: Action, roles?: string[]) => {
150-
throw new Error('Custom Error');
151-
}
152-
};
153-
154-
let expressApp: any;
155-
before(done => {
156-
const server = createExpressServer(serverOptions);
157-
expressApp = server.listen(3001, done);
158-
});
159-
after(done => expressApp.close(done));
160-
161-
let koaApp: any;
162-
before(done => {
163-
const server = createKoaServer(serverOptions);
164-
koaApp = server.listen(3002, done);
165-
});
166-
after(done => koaApp.close(done));
167-
168-
describe("custom errors", () => {
169-
assertRequest([3001, 3002], "get", "auth1", response => {
170-
expect(response).to.have.status(500);
171-
expect(response.body).to.have.property("name", "Error");
172-
expect(response.body).to.have.property("message", "Custom Error");
173-
174-
});
175-
});
193+
describe("Authorization checker allows to throw (async)", function() {
194+
before(() => {
195+
// reset metadata args storage
196+
getMetadataArgsStorage().reset();
197+
198+
@JsonController()
199+
class AuthController {
200+
@Authorized()
201+
@Get("/auth1")
202+
auth1() {
203+
return { test: "auth1" };
204+
}
205+
}
206+
});
207+
208+
const serverOptions: RoutingControllersOptions = {
209+
authorizationChecker: async (action: Action, roles?: string[]) => {
210+
throw new NotAcceptableError("Custom Error");
211+
},
212+
};
213+
214+
let expressApp: any;
215+
before(done => {
216+
const server = createExpressServer(serverOptions);
217+
expressApp = server.listen(3001, done);
218+
});
219+
after(done => expressApp.close(done));
220+
221+
let koaApp: any;
222+
before(done => {
223+
const server = createKoaServer(serverOptions);
224+
koaApp = server.listen(3002, done);
225+
});
226+
after(done => koaApp.close(done));
227+
228+
describe("custom errors", () => {
229+
assertRequest([3001, 3002], "get", "auth1", response => {
230+
expect(response).to.have.status(406);
231+
expect(response.body).to.have.property("name", "NotAcceptableError");
232+
expect(response.body).to.have.property("message", "Custom Error");
233+
});
234+
});
235+
});
236+
237+
describe("Authorization checker allows to throw (sync)", function() {
238+
before(() => {
239+
// reset metadata args storage
240+
getMetadataArgsStorage().reset();
241+
242+
@JsonController()
243+
class AuthController {
244+
@Authorized()
245+
@Get("/auth1")
246+
auth1() {
247+
return { test: "auth1" };
248+
}
249+
}
250+
});
251+
252+
const serverOptions: RoutingControllersOptions = {
253+
authorizationChecker: (action: Action, roles?: string[]) => {
254+
throw new NotAcceptableError("Custom Error");
255+
},
256+
};
257+
258+
let expressApp: any;
259+
before(done => {
260+
const server = createExpressServer(serverOptions);
261+
expressApp = server.listen(3001, done);
262+
});
263+
after(done => expressApp.close(done));
264+
265+
let koaApp: any;
266+
before(done => {
267+
const server = createKoaServer(serverOptions);
268+
koaApp = server.listen(3002, done);
269+
});
270+
after(done => koaApp.close(done));
271+
272+
describe("custom errors", () => {
273+
assertRequest([3001, 3002], "get", "auth1", response => {
274+
expect(response).to.have.status(406);
275+
expect(response.body).to.have.property("name", "NotAcceptableError");
276+
expect(response.body).to.have.property("message", "Custom Error");
277+
});
278+
});
176279
});

0 commit comments

Comments
 (0)