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

feat(auth): Add Argon2 Hashing Algorithm support #637

Merged
merged 10 commits into from
Apr 25, 2022
Merged

Conversation

ssbushi
Copy link
Contributor

@ssbushi ssbushi commented Feb 7, 2022

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

Testing

  • Added unit tests.
  • Added checks for invalid inputs

API Changes

  • Adds new API for Argon2 Hashing support

RELEASE NOTE: Added Argon2 hashing algorithm support in the importUsers() API.

@prameshj
Copy link

prameshj commented Feb 7, 2022

Looks good to me.

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

Thank you, Sam! Looks pretty good! Left a question on abstraction.

src/main/java/com/google/firebase/auth/hash/Argon2.java Outdated Show resolved Hide resolved
}
}

public enum Argon2HashType {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this could use another layer of abstraction. Similar to what we have done with Hmac and RepeatableHash. We could introduce an abstract class for ARGON2Hash and implementations for ARGON2d, ARGON2i, and ARGON2id. WDYT?

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 think that's unnecessary here. For HMAC, it makes sense to add a layer of abstraction since the algorithms are identified as different from one another (HMAC_SHA256 HMAC_SHA1 HMAC_MD5, etc). Here ARGON2 is the algorithm, the hash-types are an additional configuration for the same algorithm.

Copy link
Member

Choose a reason for hiding this comment

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

I see. That makes sense. Thanks, Sam!

Comment on lines +150 to +151
VERSION_10,
VERSION_13

Choose a reason for hiding this comment

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

Why these 2 specific versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@lahirumaramba lahirumaramba left a comment

Choose a reason for hiding this comment

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

LGTM!
Please get a TW to take a quick look at the docs before we merge this. Thanks!

@ssbushi ssbushi merged commit 33b9033 into master Apr 25, 2022
@ssbushi ssbushi deleted the argon2-impl branch April 25, 2022 13:51
@lahirumaramba lahirumaramba changed the title Add Argon2 Hashing Algorithm support feat(auth): Add Argon2 Hashing Algorithm support Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants