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

[Pending] Specifying hashInputOrder #937

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

[Pending] Specifying hashInputOrder #937

wants to merge 13 commits into from

Conversation

xil222
Copy link
Contributor

@xil222 xil222 commented Jul 8, 2020

waiting for requirement of md5 confirmation
add functionality and tests for specifying inputHashOrder for algorithms:
hmac{md5, sha1, sha256, sha512} and {sha1, sha256, sha512}

@xil222 xil222 requested a review from bojeil-google July 8, 2020 23:33
@hiranya911
Copy link
Contributor

This implementation makes sense to me. It also happens to match an internally approved API proposal. @bojeil-google WDYT?

@@ -570,6 +570,8 @@ export namespace admin.auth {
pageToken?: string;
}

type HashInputOrderType = 'SALT_FIRST' | 'PASSWORD_FIRST';
Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to update the implementation. They are now autogenerating the type definitions. Take a look at some of the latest Auth changes related to modularization in the repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't quite understand here. You're referring to the usage of HashInputOrderType but not this line right?
Cause what I saw is like removing "?" or "| null" along with the type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @xil222, I don't refer specifically to this line, but in general, they are moving away from auth.d.ts and now are auto-generating the type definitions from the code. This PR may be helpful to understand these changes:
https://github.com/firebase/firebase-admin-node/pull/1009/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It took me sometime but I am not sure I understand it correctly or PR did help.

  1. I didn't see how it resolved auto-generating type definition. In the link above, the only reference to type is line 79 in src/auth/user-import-builder-internal.ts:
    export type ValidatorFunction = (data: UploadAccountUser) => void;
    It's exactly the same implementation as line 158 in src/auth/user-import-builder.ts
    There's no sign of auto-generating type definition here.
    What I can see in src/auth/user-import-builder-internal.ts doing is importing few interfaces exported in src/auth/user-import-builder.ts, but I didn't see how it do with autogenerating the type definitions.

  2. In the link above, src/auth/user-import-builder-internal.ts was implemented in another branch which hasn't been merged into master branch yet. Even if my first point was wrong, that few lines of code edition could realize the auto-generating of type definitions, real implementation should be done only after that branch has been merged. I can't remove or take off the definition of this line.

  3. I need more clarification on auto-generating type definitions. I can't see how the implementation could help with that.

Therefore, maybe it's better merge this PR into master for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok it is fine to stick with the current implementation in master. Though I think if this ends up landing after the other branch lands, you may have to make the changes then.

@hiranya911 just a heads on this.

src/auth/user-import-builder.ts Show resolved Hide resolved
src/auth/user-import-builder.ts Outdated Show resolved Hide resolved
src/auth/user-import-builder.ts Outdated Show resolved Hide resolved
src/auth/user-import-builder.ts Outdated Show resolved Hide resolved
test/unit/auth/user-import-builder.spec.ts Show resolved Hide resolved
test/unit/auth/user-import-builder.spec.ts Outdated Show resolved Hide resolved
test/unit/auth/user-import-builder.spec.ts Outdated Show resolved Hide resolved
test/unit/auth/user-import-builder.spec.ts Show resolved Hide resolved
test/integration/auth.spec.ts Outdated Show resolved Hide resolved
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