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

Add mock data for Apple OAuth #1738

Merged
merged 2 commits into from
Sep 10, 2019
Merged

Conversation

dzunk
Copy link
Contributor

@dzunk dzunk commented Sep 9, 2019

Issue#

No-Story

Description:

Adds a mock for Apple OAuth, Faker::Omniauth.apple. This supports testing the omniauth-apple gem.

There is an interesting gotcha with Apple OAuth, that they only return the email data the first time a user authenticates. I erred on the side of having it always available rather than never, but if there's a better way to represent that, I'm definitely open to it. Maybe Faker::Omniauth.apple(email: false)

Copy link
Member

@bpleslie bpleslie left a comment

Choose a reason for hiding this comment

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

Thanks for this! Just need to update the docs: https://github.com/faker-ruby/faker/blob/master/doc/default/omniauth.md

Also, YARD docs are in the pipeline if you want to add some to the apple method 😄

Copy link
Member

@vbrazo vbrazo left a comment

Choose a reason for hiding this comment

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

Could you please add documentation /doc to explain this method?

@@ -341,6 +341,40 @@ def github(legacy_name = NOT_GIVEN, legacy_email = NOT_GIVEN, legacy_uid = NOT_G
}
end

def apple(name: nil, email: nil, uid: nil)
Copy link
Member

Choose a reason for hiding this comment

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

What about uid: SecureRandom.uuid instead of uid: nil?

Copy link
Member

Choose a reason for hiding this comment

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

I've just noticed that the def linkedin signature and the other methods in this faker generator are setting uid as Number.number(digits: 6).to_s. SecureRandom.uuid sounds better to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using SecureRandom directly I was getting a spec failure:

Error: test_determinism(TestDeterminism): RuntimeError: Faker::Omniauth.apple has an entropy leak

The actual uid value from Apple is someting like 001547.2524340274204e7ab773480dd4359bd3.2354, which is where the number+hex+number pattern came from. I set it nil in the method signature and then reassigned in the method body just for line length reasons.

Copy link
Member

Choose a reason for hiding this comment

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

If the format is different, SecureRandom wouldn't be a good choice. If you want to generate the same format, I'd recommend using bothify just like we did here, otherwise leave it as it is.

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 tried using bothify but the inner bit is a hex value, so letterify didn't provide valid data. We could add a hexify method?

@dzunk dzunk requested a review from vbrazo September 10, 2019 17:53
Copy link
Member

@bpleslie bpleslie left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for your contribution! 🏅

Copy link
Member

@vbrazo vbrazo left a comment

Choose a reason for hiding this comment

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

👍

@vbrazo vbrazo merged commit 9be8cfd into faker-ruby:master Sep 10, 2019
@dzunk dzunk deleted the omniauth_apple branch September 11, 2019 18:40
@dzunk dzunk restored the omniauth_apple branch September 11, 2019 19:00
michebble pushed a commit to michebble/faker that referenced this pull request Feb 16, 2020
* Add mock data for Apple OAuth

* Add docs for mocks
davidmorton0 pushed a commit to davidmorton0/faker that referenced this pull request Jul 12, 2021
* Add mock data for Apple OAuth

* Add docs for mocks
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.

3 participants