Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: 🐛 accurate auth typings #4523

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 21 additions & 9 deletions lib/types/request.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,24 @@ export interface AppCredentials {
* User-extensible type for request.auth credentials.
*/
export interface AuthCredentials<
AuthUser extends object = UserCredentials,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering, what difference does it make to remove all those extends?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically you can say that User is a string and hapi will accept it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the types were trying to be prescriptive in "good" practices, should we just stick with what hapi does then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I removed it was because, to me, it's like the decorator types where it expects you to pass a function but in reality you can pass anything and it will decorate. I think we should type reality, no? IFAIK, technically, credentials and artifacts can be a single primitive value; so can app, so can user. Hapi does not internally use any of these values. These are user-defined and I think the user should dictate how they need to build their auth.

Example:

https://github.com/hapijs/basic/blob/master/lib/index.js#L64
https://github.com/hapijs/jwt/blob/master/lib/plugin.js#L225

They're never used and can be anything at runtime.

However, if you think we should communicate that these must be objects, I won't disagree either because it's not wrong. I've never not-used an object in these abstractions.

AuthApp extends object = AppCredentials,
AuthUser = UserCredentials,
AuthApp = AppCredentials
> {
/**
* The application scopes to be granted.
* [See docs](https://github.com/hapijs/hapi/blob/master/API.md#-routeoptionsauthaccessscope)
*/
scope?: string[] | undefined;

/**
* If set, will only work with routes that set `access.entity` to `user`.
*/
user?: MergeType<UserCredentials, AuthUser> | undefined;
user?: AuthUser

/**
* If set, will only work with routes that set `access.entity` to `app`.
*/
app?: MergeType<AppCredentials, AuthApp> | undefined;
app?: AuthApp;
}

export interface AuthArtifacts {
Expand All @@ -65,15 +66,20 @@ export type AuthMode = 'required' | 'optional' | 'try';
* [See docs](https://github.com/hapijs/hapi/blob/master/API.md#-requestauth)
*/
export interface RequestAuth<
AuthUser extends object = UserCredentials,
AuthApp extends object = AppCredentials,
CredentialsExtra extends object = Record<string, unknown>,
AuthUser = UserCredentials,
AuthApp = AppCredentials,
CredentialsExtra = Record<string, unknown>,
ArtifactsExtra = Record<string, unknown>
> {
/** an artifact object received from the authentication strategy and used in authentication-related actions. */
artifacts: ArtifactsExtra;
/** the credential object received during the authentication process. The presence of an object does not mean successful authentication. */
credentials: MergeType<CredentialsExtra, AuthCredentials<AuthUser, AuthApp>>;
credentials: (

AuthCredentials<AuthUser, AuthApp> &
CredentialsExtra
);

/** the authentication error is failed and mode set to 'try'. */
error: Error;
/** true if the request has been successfully authenticated, otherwise false. */
Expand All @@ -89,6 +95,7 @@ export interface RequestAuth<
strategy: string;
}


/**
* 'peek' - emitted for each chunk of payload data read from the client connection. The event method signature is function(chunk, encoding).
* 'finish' - emitted when the request payload finished reading. The event method signature is function ().
Expand Down Expand Up @@ -296,7 +303,12 @@ export type ReqRef = Partial<Record<keyof ReqRefDefaults, unknown>>;
/**
* Utilities for merging request refs and other things
*/
export type MergeType<T extends object, U extends object> = Omit<T, keyof U> & U;
export type MergeType<T, U> = {
[K in keyof T]: K extends keyof U
? U[K]
: T[K];
};
Comment on lines +306 to +310
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not picking the keys of U, I think we're going to miss some parts unless we do this

Suggested change
export type MergeType<T, U> = {
[K in keyof T]: K extends keyof U
? U[K]
: T[K];
};
export type MergeType<T, U> = {
[K in keyof T]: K extends keyof U
? U[K]
: T[K];
} & U;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

U would be what you're extending from. Hapi only uses T, which is ReqRefDefaults in the latter utility type, so any extra properties you might add in the reference type U are unused if they're not the same as T. This makes intersecting unnecessary.

Does it give you any issues?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, but MergeType as a generic type can be abused with a mismatch of keys. If it's not doing an actual merge of keys, maybe it should be named otherwise.

Copy link
Contributor Author

@damusix damusix Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. What name do you suggest? It should only care about objects related to hapi internal types. Anything else would be unused. It merges from left to right, so perhaps ApplyLeft ?


export type MergeRefs<T extends ReqRef> = MergeType<ReqRefDefaults, T>;

/**
Expand Down
27 changes: 13 additions & 14 deletions lib/types/response.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
UserCredentials,
AppCredentials,
AuthArtifacts,
MergeType,
AuthCredentials,
ReqRef,
ReqRefDefaults,
Expand Down Expand Up @@ -386,19 +385,19 @@ export type ResponseValue = string | object;

export interface AuthenticationData<

AuthUser extends object = UserCredentials,
AuthApp extends object = AppCredentials,
CredentialsExtra extends object = Record<string, unknown>,
AuthUser = UserCredentials,
AuthApp = AppCredentials,
CredentialsExtra = Record<string, unknown>,
ArtifactsExtra = AuthArtifacts
> {
credentials: MergeType<CredentialsExtra, AuthCredentials<AuthUser, AuthApp>>;
credentials: AuthCredentials<AuthUser, AuthApp> & CredentialsExtra;
artifacts?: ArtifactsExtra | undefined;
}

export interface Auth<
AuthUser extends object = UserCredentials,
AuthApp extends object = AppCredentials,
CredentialsExtra extends object = Record<string, unknown>,
AuthUser = UserCredentials,
AuthApp = AppCredentials,
CredentialsExtra = Record<string, unknown>,
ArtifactsExtra = AuthArtifacts
> {
readonly isAuth: true;
Expand Down Expand Up @@ -459,9 +458,9 @@ export interface ResponseToolkit<Refs extends ReqRef = ReqRefDefaults> {
* @return Return value: an internal authentication object.
*/
authenticated <
AuthUser extends object = MergeRefs<Refs>['AuthUser'],
AuthApp extends object = MergeRefs<Refs>['AuthApp'],
CredentialsExtra extends object = MergeRefs<Refs>['AuthCredentialsExtra'],
AuthUser = MergeRefs<Refs>['AuthUser'],
AuthApp = MergeRefs<Refs>['AuthApp'],
CredentialsExtra = MergeRefs<Refs>['AuthCredentialsExtra'],
ArtifactsExtra = MergeRefs<Refs>['AuthArtifactsExtra']
>(
data: (
Expand Down Expand Up @@ -537,9 +536,9 @@ export interface ResponseToolkit<Refs extends ReqRef = ReqRefDefaults> {
* [See docs](https://github.com/hapijs/hapi/blob/master/API.md#-hunauthenticatederror-data)
*/
unauthenticated <
AuthUser extends object = MergeRefs<Refs>['AuthUser'],
AuthApp extends object = MergeRefs<Refs>['AuthApp'],
CredentialsExtra extends object = MergeRefs<Refs>['AuthCredentialsExtra'],
AuthUser = MergeRefs<Refs>['AuthUser'],
AuthApp = MergeRefs<Refs>['AuthApp'],
CredentialsExtra = MergeRefs<Refs>['AuthCredentialsExtra'],
ArtifactsExtra = MergeRefs<Refs>['AuthArtifactsExtra']
>(
error: Error,
Expand Down
64 changes: 62 additions & 2 deletions test/types/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,25 @@ import {
Server,
ServerRoute,
server as createServer,
ServerRegisterPluginObject
UserCredentials
} from '../..';

const { expect: check } = lab;

type IsAny<T> = (
unknown extends T
? [keyof T] extends [never] ? false : true
: false
);


declare module '../..' {
interface UserCredentials {
someId: string;
someName: string;
}
}

interface ServerAppSpace {
multi?: number;
}
Expand All @@ -27,13 +41,44 @@ check.type<MyServer>(server);

server.app.multi = 10;

const genericRoute: ServerRoute = {
method: 'GET',
path: '/',
handler: (request, h) => {

check.type<UserCredentials>(request.auth.credentials!.user!);

const y: IsAny<typeof request.auth.credentials> = false;

return 'hello!';
}
}

server.route(genericRoute);

interface RequestDecorations {
Server: MyServer;
RequestApp: {
word: string;
},
RouteApp: {
prefix: string[];
},
AuthUser: {
id: string,
name: string
email: string
},
AuthCredentialsExtra: {
test: number
}
AuthApp: {
key: string
name: string
},
AuthArtifactsExtra: {
some: string
thing: number
}
}

Expand Down Expand Up @@ -61,6 +106,21 @@ const route: ServerRoute<RequestDecorations> = {
check.type<number>(request.server.app.multi!);
check.type<string[]>(request.route.settings.app!.prefix);

check.type<number>(request.auth.credentials!.test);

check.type<string>(request.auth.credentials!.user!.email);
check.type<string>(request.auth.credentials!.user!.id);
check.type<string>(request.auth.credentials!.user!.name);

check.type<string>(request.auth.credentials!.app!.name);
check.type<string>(request.auth.credentials!.app!.key);

check.type<string>(request.auth.artifacts.some);
check.type<number>(request.auth.artifacts.thing);

const y: IsAny<typeof request.auth.credentials> = false;
const z: IsAny<typeof request.auth.artifacts> = false;

return 'hello!'
}
};
Expand Down Expand Up @@ -123,4 +183,4 @@ server.method('test.add', (a: number, b: number) => a + b, {
segment: 'test-segment',
},
generateKey: (a: number, b: number) => `${a}${b}`
});
});