-
Notifications
You must be signed in to change notification settings - Fork 126
Added feature to create third party access tokens #968
base: master
Are you sure you want to change the base?
Added feature to create third party access tokens #968
Conversation
jaypatel512
left a comment
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.
These are my comments based on my very limited understanding of subject based on this PR.
| */ | ||
| private $cipher; | ||
|
|
||
| /** |
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.
encryted -> encrypted
| $params['grant_type'] = 'refresh_token'; | ||
| $params['refresh_token'] = $refreshToken; | ||
| } | ||
|
|
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.
If subject is associated with a refreshToken, should we just pass the subject as an argument here, instead of adding it to a constructor ?
| $cred = new OAuthTokenCredential('clientId', 'clientSecret', 'subject'); | ||
|
|
||
| //{"clientId":{"clientId":"clientId","accessToken":"accessToken","tokenCreateTime":1421204091,"tokenExpiresIn":288000000}} | ||
| AuthorizationCache::push($config, 'clientId', $cred->encrypt('accessTokenWithSubject'), 1421204091, 288000000); |
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.
If subject is relevant only for third party tokens (refresh tokens), we should not cache the response. This could lead to all consequent calls with just a clientId to use this subject based access token. This will be because, we only fetch this cache by clientId and nothing else.
4ef45ec to
31f6001
Compare
No description provided.