Better interfaces for CustomAuthProvider and BaseHttpController

[remojansen]

Hi, First, thanks for all the work.

I have been trying to use the CustomAuthProvider and I'm having some problems.

The first problem is in methods in controllers that extends BaseHttpController. When I try the following:

this.httpContext.user.details.id // Object is of type 'unknown'

I get an error because httpContext is HttpContext<unknown>. The current declaration of BaseHttpController is:

export declare class BaseHttpController {
    protected readonly httpContext: HttpContext; // <-- HttpContext<T = unknown>

It could be fixed by making BaseHttpController generic:

export declare class BaseHttpController<T> {
    protected readonly httpContext: HttpContext<T>;

The second problem is in the AuthProvider interface:

export interface AuthProvider {
  getUser(req: Request, res: Response, next: NextFunction): Promise<Principal>;
}

I was confused to see that you can both invoke next and return a promise. It would be better to support one way only.

import { injectable, inject } from "inversify";
import { interfaces } from "inversify-express-utils";

const authService = inject("AuthService");

@injectable()
class CustomAuthProvider implements interfaces.AuthProvider {

    @authService private readonly _authService: AuthService;

    public async getUser(
        req: express.Request,
        res: express.Response,
        // next: express.NextFunction <-- Should we remove this?
    ): Promise<interfaces.Principal> {
        const token = req.headers["x-auth-token"]
        const user = await this._authService.getUser(token);
        const principal = new Principal(user);
        return principal;
    }

}

Please let me know if you agree and also give me some details about the best way to go about this. Should it be fixed in this repo?

[notaphplover]

Hey @remojansen,

The first problem is in methods in controllers that extends BaseHttpController. When I try the following:

this.httpContext.user.details.id // Object is of type 'unknown'

I get an error because httpContext is HttpContext<unknown>. The current declaration of BaseHttpController is:

export declare class BaseHttpController {
   protected readonly httpContext: HttpContext; // <-- HttpContext<T = unknown>

It could be fixed by making BaseHttpController generic:

export declare class BaseHttpController<T> {
    protected readonly httpContext: HttpContext<T>;

Absolutelly, it makes sense to provide that generic. Maybe TUser for clarity.

I was confused to see that you can both invoke next and return a promise. It would be better to support one way only.

I'm not sure to be honest. You might actually be interested in replying and calling your next middleware (let's say that middleware does a background task such as sending metrics) Does it make sense?