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 OpenID Response Type #1316

Open
wants to merge 49 commits into
base: master
Choose a base branch
from

Conversation

marcriemer
Copy link

No description provided.

@marcriemer marcriemer changed the title Added OpenID Responce Type Add OpenID Response Type Nov 30, 2022
@doobry-systemli
Copy link

I'm glad to see some progress here! @marcriemer do you intend to continue working on it? Did you get stuck somewhere particularly?

@marcriemer
Copy link
Author

@doobry-systemli So far my implementation works very well. I will continue my work on in this feature. Please let me know If you have any successions for improvements.

@Sephster
Copy link
Member

Sephster commented Jun 8, 2023

Thanks for this PR. I am a bit stumped as to why the tests aren't running in this PR. Everything looks ok to me. Do you have any ideas? If not, I will put in a GH support request as I have a vague recollection something similar has happened in the past

@Sephster
Copy link
Member

Sephster commented Jun 8, 2023

Thanks for this PR. I am a bit stumped as to why the tests aren't running in this PR. Everything looks ok to me. Do you have any ideas? If not, I will put in a GH support request as I have a vague recollection something similar has happened in the past

And now they are running... odd. Must have been some weird delay. Apologies for taking so long to pick this up. Are you able to take a look at the failing tests? Thank you

Copy link

@fruitwasp fruitwasp left a comment

Choose a reason for hiding this comment

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

keep up the great work! Thanks for this PR!

src/ClaimExtractorIntercace.php Outdated Show resolved Hide resolved
@pat0s
Copy link

pat0s commented Feb 19, 2024

Hi @marcriemer,
I would like to ask one question. I got a little bit lost. I think it's not a mistake, but it is confusing.

interface ClaimSetRepositoryInterface
{
    /**
     * Get ClaimSetEntries
     *
     * @param AccessTokenEntityInterface $authCode
     *
     * @return ClaimSetInterface
     */
    public function getClaimSetEntry(AccessTokenEntityInterface $authCode): ClaimSetInterface;
}
$claimSet = $this->claimRepository->getClaimSetEntry($accessToken);

if ($claimSet instanceof ClaimSetInterface) {
            foreach ($this->extractor->extract($accessToken->getScopes(), $claimSet->getClaims()) as $claimName => $claimValue) {
                $builder->withClaim($claimName, $claimValue);
            }
        }

getClaimSetEntry() sounds like I should return ClaimSetEntry. ClaimSetEntry also implements ClaimSetEntryInterface, which means new method getScope(). I think it is irrelevant in this case. It seems like I should return only ClaimSet from this repository.

ClaimSet is new Entity which I created. It implements only ClaimSetInterface and stores all claims with values (from all scopes) in one array. Entity is created according to method's return type ClaimSetInterface.

So in my case method getClaimSetEntry() returns new ClaimSet(['email' => 'email@example', 'email_verified' => 1, ...])

@abin2hats
Copy link

Hi @marcriemer

Thank you for the initiative.

Why this PR is not merged even if the PR is reviewed?

Is there any plan to merge this PR?

Thank you.

@Sephster
Copy link
Member

Version 9 bringing in the device code is being worked on just now. Then I will start to look at other features. V9 RC1 should be tagged today all being well

@marcriemer
Copy link
Author

@pat0s Thanks for bringing this up. You are right, the method should return a ClaimSetEntryInterface.

The ClaimSetEntryInterface extends ClaimSetInterface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants