Create VanillaConnect library #1
Conversation
DaazKu
added
the
WIP
DaazKu
force-pushed
the
feature/initial
branch
from
b4c123d
to
5a0127f
Compare
DaazKu
force-pushed
the
feature/initial
branch
from
1e2ae8f
to
e8358d7
Compare
DaazKu
force-pushed
the
feature/initial
branch
from
4ab510c
to
290fd6f
Compare
DaazKu
removed
the
WIP
DaazKu
added
WIP
| /** | ||
| * List of errors that were encountered during the validation process. | ||
| * | ||
| * @var array |
There was a problem hiding this comment.
You're switching up the doc style here. This should probably look like:
/**
* @var array List of errors that were encountered during the validation process.
*/| public static function extractClientID($jwt) { | ||
| $parts = explode('.', $jwt); | ||
| if (count($parts) !== 3) { | ||
| throw new Exception('Wrong number of segments.'); |
There was a problem hiding this comment.
Is there an Exception class available while you're in the Vanilla\VanillaConnect namespace? I think you might be missing use Exception in this class file.
| } | ||
|
|
||
| /** | ||
| * @return string |
There was a problem hiding this comment.
Need a description for this method. There are several more instances, below, that also need descriptions.
| public function createRequestAuthJWT($nonce, array $extraClaimItems = []) { | ||
| $authHeader = array_merge( | ||
| self::JWT_REQUEST_HEADER_TEMPLATE, | ||
| ['azp' => $this->clientID] |
There was a problem hiding this comment.
Should this fail if $clientID hasn't been set? This same question applies to the createResponseAuthJWT method.
There was a problem hiding this comment.
It is not possible to not set the clientID since it is required by the constructor.
I'll add exception to the constructor to make sure that no one does:
new VanillaConnect(null, '');
| * @param array $jwtHeader Array that will receive the JWT header's content on success. | ||
| * @return array|bool The decoded payload or false otherwise. | ||
| */ | ||
| public function validateRequest($jwt, &$jwtClaim=[], &$jwtHeader=[]) { |
There was a problem hiding this comment.
Should give the equal signs a little space
| * It is a pretty loose regex that should enforce what is needed without blocked weird cases. | ||
| * | ||
| * One line regex: | ||
| * /^(?<scheme>(?:https?:)?\/\/)(?:[^\s]+?@)?(?<host>[^\/\s]+)(?<path>\/[^?#\s]*)$/ |
There was a problem hiding this comment.
I appreciate the one-liner, but it looks to be missing the userpwd portion.
| /** | ||
| * VanillaConnectProvider constructor. | ||
| * | ||
| * @param string S$clientID |
There was a problem hiding this comment.
I think an S got away from you.
|
|
||
| if ($urlEncodingError) { | ||
| $errors['request_invalid_redirect_tip'] = | ||
| "Seems like the redirect URL was not properly encoded. Invalid character detected."; |
There was a problem hiding this comment.
Why'd you go with the "double quote" string syntax here?
| if ($this->vanillaConnect->validateRequest($requestJWT, $authClaim)) { | ||
| $nonce = $authClaim['nonce']; | ||
| } else { | ||
| $errors = $this->vanillaConnect->getErrors(); |
There was a problem hiding this comment.
We're recording specific errors in this function and then potentially overwriting all of them with whatever comes back from VanillaConnect.
| $claim = ['errors' => $errors]; | ||
| } | ||
|
|
||
| $responseJWT = $this->vanillaConnect->createResponseAuthJWT($nonce, $claim); |
There was a problem hiding this comment.
For the sake of my IDE, can we set $nonce to null by default, so there's no perceived chanced it might not be set?
There was a problem hiding this comment.
Your IDE is stupid :P. There is no codepath where $nonce is not set.
There was a problem hiding this comment.
She's just sensitive, because the only times she sees it getting set are inside an if conditional block
initvector
added
the
Revisit
DaazKu
removed
the
Revisit
Information
Original spec:
Read the README.md
Notable differences:
?jwtinstead of?ssosince the latter is reserved to jsConnectpush sso/embedTODO
How to test
You can see the library used here and here as a pushsso.