-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -27,23 +27,24 @@ export interface AppCredentials { | |||||||||||||||||||||
* User-extensible type for request.auth credentials. | ||||||||||||||||||||||
*/ | ||||||||||||||||||||||
export interface AuthCredentials< | ||||||||||||||||||||||
AuthUser extends object = UserCredentials, | ||||||||||||||||||||||
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 { | ||||||||||||||||||||||
|
@@ -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. */ | ||||||||||||||||||||||
|
@@ -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 (). | ||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Does it give you any issues? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||||||||||||||||
|
||||||||||||||||||||||
export type MergeRefs<T extends ReqRef> = MergeType<ReqRefDefaults, T>; | ||||||||||||||||||||||
|
||||||||||||||||||||||
/** | ||||||||||||||||||||||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
andartifacts
can be a single primitive value; so canapp
, so canuser
. 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.