-
Notifications
You must be signed in to change notification settings - Fork 452
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
Apple - new option for using private key instead secret key. #1019
base: master
Are you sure you want to change the base?
Conversation
Could we get a docs like and a rebase please :) |
# Conflicts: # src/Apple/Provider.php
I don't know what you mean exactly. I also put some description in README.md. Can you tell me exactly what I should add? :) |
return (new self( | ||
new Request(), | ||
config('services.apple.client_id'), | ||
config('services.apple.client_secret'), |
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.
Will this method still work if the secret is dervied?
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.
Yes it will work. I had to do that (create new object) because this function is called statically (don't know why) and it was necessary to call other non static functions.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This seems to be inspired from https://bannister.me/blog/generating-a-client-secret-for-sign-in-with-apple-on-each-request Perhaps add support for private keys that are stored in the database / string format too? My proposal would be something like:
This will ensure backwards compatibility for those of us who are already using this integration. Thank you. |
Done ;) |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Is this ready for review/merge? |
Yes, it should be ready for review/merge. I've been using this successfully into production on multiple websites. This would be an awesome addition. |
Yes, i use it in production too. |
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.
One small change and a comment.
src/Apple/Provider.php
Outdated
protected function getJwtConfig() | ||
{ | ||
if (!$this->jwtConfig) { | ||
$private_key_path = config('services.apple.private_key', ''); |
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.
can we access this via the standard method, ie add to
public static function additionalConfigKeys()
{
return ['private_key', 'otherkey...'];
}
$this->getConfig('private_key', '')
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.
Yes, we can. Changed.
$this->privateKey = file_get_contents($private_key_path); | ||
} | ||
|
||
$this->jwtConfig = Configuration::forSymmetricSigner( |
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.
Just to confirm, there is no bc break here, existing integrations using the secret key will continiue to work fine?
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.
yes, if you pass the path to the private key file, it will get the contents of the private key file, otherwise you put the private key contents by default.
@hamrak just a quick thing re; config and a question, then I will merge and tag new vers :) |
if (!empty($this->privateKey)) { | ||
$appleToken = new AppleToken($this->getJwtConfig()); | ||
$this->clientSecret = $appleToken->generate(); | ||
config()->set('services.apple.client_secret', $this->clientSecret); |
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.
Might need to set this to a variable on the class with above switch to $this->getConfig
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.
Why do you think so?
{ | ||
return (new self( | ||
new Request(), | ||
config('services.apple.client_id'), |
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.
config('services.apple.client_id'), | |
$this->getConfig('client_id', '') |
And other calls to config()
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.
You can call $this-> in static function, or ?
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.
oh my bad, didn't realise it was static. Is that why it overrides the global config above?
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.
oh my bad, didn't realise it was static. Is that why it overrides the global config above?
yes
{ | ||
return (new self( | ||
new Request(), | ||
config('services.apple.client_id'), |
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.
oh my bad, didn't realise it was static. Is that why it overrides the global config above?
cc @hamrak sorry can you resolve conflicts and we can get this in |
# Conflicts: # src/Apple/Provider.php
Done. |
Rather than static client secrets, Apple requires that you derive a client secret yourself from your private key. They use the JWT standard for this, using an elliptic curve algorithm with a P-256 curve and SHA256 hash. In other words, they use the ES256 JWT algorithm.
This PR add new configuration options that allow you to use private key instead of manually generating secret key every 6 months.
Need to merge #1018 first.