Skip to content

fix(auth): Address several auth typing inconsistencies #993

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

Merged
merged 5 commits into from
Aug 19, 2020

Conversation

MathBunny
Copy link
Contributor

During my work for refactoring auth to auto-generate typings I came across several inconsistencies in source and typing files. I'm not sure how to best address them or which require changes, but I did an attempt here.

1. auth.ts

Observation: The tenant field is defined at the root of the object in the source while it's in the firebase scope in the typings.

Source:

/** Interface representing a decoded ID token. */
export interface DecodedIdToken {
aud: string;
auth_time: number;
email?: string;
email_verified?: boolean;
exp: number;
firebase: {
identities: {
[key: string]: any;
};
sign_in_provider: string;
sign_in_second_factor?: string;
second_factor_identifier?: string;
[key: string]: any;
};
iat: number;
iss: string;
phone_number?: string;
picture?: string;
sub: string;
tenant?: string;
uid: string;
[key: string]: any;
}

Typing:

firebase: {
/**
* Provider-specific identity details corresponding
* to the provider used to sign in the user.
*/
identities: {
[key: string]: any;
};
/**
* The ID of the provider used to sign in the user.
* One of `"anonymous"`, `"password"`, `"facebook.com"`, `"github.com"`,
* `"google.com"`, `"twitter.com"`, or `"custom"`.
*/
sign_in_provider: string;
/**
* The type identifier or `factorId` of the second factor, provided the
* ID token was obtained from a multi-factor authenticated user.
* For phone, this is `"phone"`.
*/
sign_in_second_factor?: string;
/**
* The `uid` of the second factor used to sign in, provided the
* ID token was obtained from a multi-factor authenticated user.
*/
second_factor_identifier?: string;
/**
* The ID of the tenant the user belongs to, if available.
*/
tenant?: string;
[key: string]: any;
};

2. auth-config.ts

Observation: The SAMLAuthProviderConfig contains an enableRequestSigning in the source which is absent in typings.

Source:

/** The SAML Auth provider configuration interface. */
export interface SAMLAuthProviderConfig extends AuthProviderConfig {
idpEntityId: string;
ssoURL: string;
x509Certificates: string[];
rpEntityId: string;
callbackURL?: string;
enableRequestSigning?: boolean;
}

Typing:

interface SAMLAuthProviderConfig extends admin.auth.AuthProviderConfig {
/**
* The SAML IdP entity identifier.
*/
idpEntityId: string;
/**
* The SAML IdP SSO URL. This must be a valid URL.
*/
ssoURL: string;
/**
* The list of SAML IdP X.509 certificates issued by CA for this provider.
* Multiple certificates are accepted to prevent outages during
* IdP key rotation (for example ADFS rotates every 10 days). When the Auth
* server receives a SAML response, it will match the SAML response with the
* certificate on record. Otherwise the response is rejected.
* Developers are expected to manage the certificate updates as keys are
* rotated.
*/
x509Certificates: string[];
/**
* The SAML relying party (service provider) entity ID.
* This is defined by the developer but needs to be provided to the SAML IdP.
*/
rpEntityId: string;
/**
* This is fixed and must always be the same as the OAuth redirect URL
* provisioned by Firebase Auth,
* `https://project-id.firebaseapp.com/__/auth/handler` unless a custom
* `authDomain` is used.
* The callback URL should also be provided to the SAML IdP during
* configuration.
*/
callbackURL?: string;
}

As an extension, a similar pattern can be found here for SAMLAuthProviderConfig:

/** The public API request interface for updating a SAML Auth provider. */
export interface SAMLUpdateAuthProviderRequest {
idpEntityId?: string;
ssoURL?: string;
x509Certificates?: string[];
rpEntityId?: string;
callbackURL?: string;
enableRequestSigning?: boolean;
enabled?: boolean;
displayName?: string;
}

3. user-import-builder.ts

Observation: The tenantId for UserImportRecord is nullable in typings but not source.

Source:

/** User import record as accepted from developer. */
export interface UserImportRecord {
uid: string;
email?: string;
emailVerified?: boolean;
displayName?: string;
phoneNumber?: string;
photoURL?: string;
disabled?: boolean;
metadata?: UserMetadataRequest;
providerData?: Array<UserProviderRequest>;
multiFactor?: {
enrolledFactors: SecondFactor[];
};
customClaims?: {[key: string]: any};
passwordHash?: Buffer;
passwordSalt?: Buffer;
tenantId?: string;
}

Typing:

/**
* The identifier of the tenant where user is to be imported to.
* When not provided in an `admin.auth.Auth` context, the user is uploaded to
* the default parent project.
* When not provided in an `admin.auth.TenantAwareAuth` context, the user is uploaded
* to the tenant corresponding to that `TenantAwareAuth` instance's tenant ID.
*/
tenantId?: string | null;

4. auth-config.ts

Observation: As a consequence from the third point, the tenantId field is nullable in one case so it should be propagated.

Source:

/** UploadAccount endpoint request user interface. */
interface UploadAccountUser {
localId: string;
email?: string;
emailVerified?: boolean;
displayName?: string;
disabled?: boolean;
photoUrl?: string;
phoneNumber?: string;
providerUserInfo?: Array<{
rawId: string;
providerId: string;
email?: string;
displayName?: string;
photoUrl?: string;
}>;
mfaInfo?: AuthFactorInfo[];
passwordHash?: string;
salt?: string;
lastLoginAt?: number;
createdAt?: number;
customAttributes?: string;
tenantId?: string;
}

function populateUploadAccountUser(
user: UserImportRecord, userValidator?: ValidatorFunction): UploadAccountUser {
const result: UploadAccountUser = {
localId: user.uid,
email: user.email,
emailVerified: user.emailVerified,
displayName: user.displayName,
disabled: user.disabled,
photoUrl: user.photoURL,
phoneNumber: user.phoneNumber,
providerUserInfo: [],
mfaInfo: [],
tenantId: user.tenantId,
customAttributes: user.customClaims && JSON.stringify(user.customClaims),

This would be a compilation error:

Type 'string | null | undefined' is not assignable to type 'string | undefined'.
Type 'null' is not assignable to type 'string | undefined'.

5. user-record.ts

The typings have displayName and enrollmentTime as optional while not in the source.

Source:

export abstract class MultiFactorInfo {
public readonly uid: string;
public readonly displayName: string;
public readonly factorId: MultiFactorId;
public readonly enrollmentTime: string;

Typing:

interface MultiFactorInfo {
/**
* The ID of the enrolled second factor. This ID is unique to the user.
*/
uid: string;
/**
* The optional display name of the enrolled second factor.
*/
displayName?: string;
/**
* The optional date the second factor was enrolled, formatted as a UTC string.
*/
enrollmentTime?: string;
/**
* The type identifier of the second factor. For SMS second factors, this is `phone`.
*/
factorId: string;
/**
* @return A JSON-serializable representation of this object.
*/
toJSON(): Object;
}

@MathBunny MathBunny marked this pull request as ready for review August 14, 2020 16:35
@hiranya911
Copy link
Contributor

hiranya911 commented Aug 14, 2020

(1) Typings are correct the way this is currently implemented. Perhaps we should just drop that field in the implementation type.
(2) I believe enableRequestSigning is not publicly exposed yet. So typings are correct. Not sure how to handle this case though.
(3), (4) I don't think this field can be nullable. We should probably update the typings.
(5) Typings should be correct.

src/auth.d.ts Outdated
/**
* The ability for SAML provider to perform request signing.
*/
enableRequestSigning?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't currently support this. I think we should remove it. I think we need the flexibility to have internal implementations not exposed externally. Many SDKs with auto-generated references already have the ability to set visibility on some APIs and attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, there's no built-in mechanism in TypeScript to hide members of an interface. Any library that's doing so is either using third-party tools or some sort of a hack. So we will have to do the same here (@MathBunny the undocumented --stripInternal flag is probably our best bet here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I agree. I verified the @internal option and it worked as intended (updated the respective PR).

src/auth.d.ts Outdated
* Whether the SAML provider can sign requests. If not provided, the existing
* configuration's value is not modified.
*/
enableRequestSigning?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Contributor

@hiranya911 hiranya911 left a comment

Choose a reason for hiding this comment

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

LGTM

@hiranya911 hiranya911 removed their assignment Aug 17, 2020
@MathBunny MathBunny merged commit b3312ac into master Aug 19, 2020
@MathBunny MathBunny deleted the hlazu-auto-misc-typing-fix-3 branch August 19, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants