refactor: Code cleanup Pt4 #40
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| parameters: | ||
| level: 9 | ||
| paths: | ||
| - ./src | ||
| - ./test |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,7 +59,8 @@ public function getAudience(): ?string | |
| */ | ||
| public function getExpireAtTime(): ?DateTimeImmutable | ||
| { | ||
| $value = $this->getClaimSafe(SharedClaimsInterface::CLAIM_EXPIRE_AT); | ||
| return $value instanceof DateTimeImmutable ? $value : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -69,7 +70,8 @@ public function getExpireAtTime(): ?DateTimeImmutable | |
| */ | ||
| public function getNotBeforeTime(): ?DateTimeImmutable | ||
| { | ||
| $value = $this->getClaimSafe(SharedClaimsInterface::CLAIM_NOT_BEFORE); | ||
| return $value instanceof DateTimeImmutable ? $value : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -79,7 +81,8 @@ public function getNotBeforeTime(): ?DateTimeImmutable | |
| */ | ||
| public function getIssuedAtTime(): ?DateTimeImmutable | ||
| { | ||
| $value = $this->getClaimSafe(SharedClaimsInterface::CLAIM_ISSUED_AT); | ||
| return $value instanceof DateTimeImmutable ? $value : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -89,7 +92,8 @@ public function getIssuedAtTime(): ?DateTimeImmutable | |
| */ | ||
| public function getIssuer(): ?string | ||
| { | ||
| $value = $this->getClaimSafe(SharedClaimsInterface::CLAIM_ISSUER); | ||
| return is_string($value) ? $value : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -99,7 +103,8 @@ public function getIssuer(): ?string | |
| */ | ||
| public function getId(): ?string | ||
| { | ||
| $value = $this->getClaimSafe(SharedClaimsInterface::CLAIM_JWT_ID); | ||
| return is_string($value) ? $value : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -109,21 +114,23 @@ public function getId(): ?string | |
| */ | ||
| public function getSubject(): ?string | ||
| { | ||
| $value = $this->getClaimSafe(SharedClaimsInterface::CLAIM_SUBJECT); | ||
| return is_string($value) ? $value : null; | ||
| } | ||
|
|
||
| /** | ||
| * Get the role of the accessing user. | ||
| * | ||
| * If this is set to "editor", the requesting user may manage the contents | ||
|
|
||
| * of the plugin instance, i.e. she has administration rights. | ||
| * The type of the accessing entity can be either a "user" or a "editor". | ||
| * | ||
| * @return null|string | ||
| */ | ||
| public function getRole(): ?string | ||
| { | ||
| $value = $this->getClaimSafe(SharedClaimsInterface::CLAIM_USER_ROLE); | ||
| return is_string($value) ? $value : null; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,22 +64,52 @@ public static function createSignedTokenFromData(string $privateKey, array $toke | |
| private static function buildToken(Configuration $config, array $tokenData): Token | ||
| { | ||
| $builder = $config->builder(); | ||
| // Validate and coerce required registered claims to the expected types | ||
| $audience = $tokenData[SSOData\SharedClaimsInterface::CLAIM_AUDIENCE] ?? ''; | ||
| if (!is_string($audience) || $audience === '') { | ||
| throw new \InvalidArgumentException('aud claim must be a non-empty string for token generation'); | ||
| } | ||
|
Comment on lines
+67
to
+71
There was a problem hiding this comment.
|
||
|
|
||
| $issuedAt = $tokenData[SSOData\SharedClaimsInterface::CLAIM_ISSUED_AT] ?? null; | ||
| if (!($issuedAt instanceof \DateTimeImmutable)) { | ||
| throw new \InvalidArgumentException('iat claim must be a DateTimeImmutable for token generation'); | ||
| } | ||
|
|
||
| $notBefore = $tokenData[SSOData\SharedClaimsInterface::CLAIM_NOT_BEFORE] ?? null; | ||
| if (!($notBefore instanceof \DateTimeImmutable)) { | ||
| throw new \InvalidArgumentException('nbf claim must be a DateTimeImmutable for token generation'); | ||
| } | ||
|
|
||
| $expiresAt = $tokenData[SSOData\SharedClaimsInterface::CLAIM_EXPIRE_AT] ?? null; | ||
| if (!($expiresAt instanceof \DateTimeImmutable)) { | ||
| throw new \InvalidArgumentException('exp claim must be a DateTimeImmutable for token generation'); | ||
| } | ||
|
|
||
| $token = $builder | ||
| ->permittedFor($audience) | ||
| ->issuedAt($issuedAt) | ||
| ->canOnlyBeUsedAfter($notBefore) | ||
| ->expiresAt($expiresAt); | ||
|
|
||
| if (isset($tokenData[SSOData\SharedClaimsInterface::CLAIM_ISSUER])) { | ||
| $issuer = $tokenData[SSOData\SharedClaimsInterface::CLAIM_ISSUER]; | ||
| if (is_string($issuer) && $issuer !== '') { | ||
| $token = $token->issuedBy($issuer); | ||
| } | ||
| } | ||
|
|
||
| if (isset($tokenData[SSOData\SSODataClaimsInterface::CLAIM_USER_ID])) { | ||
| $subject = $tokenData[SSOData\SSODataClaimsInterface::CLAIM_USER_ID]; | ||
| if (is_string($subject) && $subject !== '') { | ||
| $token = $token->relatedTo($subject); | ||
| } | ||
| } | ||
|
|
||
| if (isset($tokenData[SSOData\SharedClaimsInterface::CLAIM_JWT_ID])) { | ||
| $jwtId = $tokenData[SSOData\SharedClaimsInterface::CLAIM_JWT_ID]; | ||
| if (is_string($jwtId) && $jwtId !== '') { | ||
| $token = $token->identifiedBy($jwtId); | ||
| } | ||
| } | ||
|
|
||
| // Remove all set keys as they throw an exception when used with withClaim | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Changing
deleteInstance()to returnnevercan cause a runtime fatal error if a consumer overridesexitRemoteCall()(it’sprotected) with an implementation that does not terminate execution. In that case,deleteInstance()would reach the end and violate thenevercontract. To avoid breaking extensibility/tests, consider keeping the return typevoidand using a PHPDoc@return neverfor static analysis, or alternatively also declareexitRemoteCall(): neverto enforce the contract consistently.