Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Security] add CAS 2.0 AccessToken handler#48276

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@nacorp
Copy link
Contributor

QA
Branch?6.3
Bug fix?no
New feature?yes
Deprecations?no
Tickets
LicenseMIT
Doc PRin progress

Hello,

Thanks to the new access token, I've added theCAS one.

In order to make it work :

services.yaml

Symfony\Component\Security\Http\AccessToken\Handler\CasHandler:arguments:$validationUrl:'%env(CAS_SERVER_VALIDATION_URL)%'security.access_token_extractor.cas:class:Symfony\Component\Security\Http\AccessToken\QueryAccessTokenExtractorarguments:            -'ticket'

Thank you@welcoMattic for the conference at the SymfonyCon and@jeremyFreeAgent for your support on my first PR on this project!

jeremyFreeAgent and Enz000 reacted with eyes emoji
@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 6.2 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.2" but it seems your PR description refers to branch "6.3".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@carsonbotcarsonbot changed the title[security] add CAS AccessToken handler[Security] add CAS AccessToken handlerNov 21, 2022
@welcoMatticwelcoMattic changed the title[Security] add CAS AccessToken handler[Security] add CAS 2.0 AccessToken handlerNov 22, 2022
@fabpotfabpot modified the milestones:6.2,6.3Nov 23, 2022
@drupol
Copy link
Contributor

Hello,

How about adding proxy authentication while we are at it ?
https://apereo.github.io/cas/6.5.x/authentication/Configuring-Proxy-Authentication.html

@nacorp
Copy link
ContributorAuthor

Hello,

How about adding proxy authentication while we are at it ?https://apereo.github.io/cas/6.5.x/authentication/Configuring-Proxy-Authentication.html

Hello@drupol,
In my opinion it should be in a dedicated pr as it's a non mandatory use case

@drupol
Copy link
Contributor

Hello,

I do not agree that much. Proxy validation is part of the CAS protocol, therefore, it's better to add it in here... or else you're going to implement half of the protocol here... is this what we want?

@welcoMattic
Copy link
Member

@nacorp@drupol is the proxy authentication part avoid the actual code to work as expected? If not, we can consider to add it later in another PR. But if this proxy is mandatory to make the authentication process work, it must be provided in this PR.

@drupol
Copy link
Contributor

Proxy authentication involves the creation of a specific endpoint on the app and therefore is not part of this PR.

I would suggest to not implement the CAS protocol and discard this PR as long as the protocol is not fully implemented.

@nacorp
Copy link
ContributorAuthor

I do confirm that the proxy authentication of the cas protocol is optional and this PR, as is, allows us to use CAS protocol with default configuration.

@chalasr
Copy link
Member

chalasr commentedDec 21, 2022•
edited
Loading

It's ok if we don't implement the full specification, as long as the design allows implementing the other missing features.
I'm not super familiar to CAS but shouldn't we rather go with the last version of the protocol (3)?

@nacorp
Copy link
ContributorAuthor

As « the most noticeable update between versions 2.0 and 3.0 is the ability to return the authentication/user attributes », this PR will also work for simple authentication against CAS3 server. However, the developper will not not get all the data available from the authentication server - which is not a problem, imo, since the main goal of this PR is to deal with auth.

@nacorp
Copy link
ContributorAuthor

poke@welcoMattic :-)

@nacorpnacorp requested review fromwelcoMattic and removed request forchalasr andwouterjJanuary 20, 2023 18:58
@chalasr
Copy link
Member

This misses some DI integration in SecurityBundle i.e. the service configuration/registration needed to allow forauthenticators: { access_token: { token_handler: 'cas' } }.
You could find some inspiration in#48272

@nacorp
Copy link
ContributorAuthor

OK as#48272 will ship a brand new factory I'll do that as soon as it's merged!

@nacorpnacorpforce-pushed thesecurity-add-CAS-AccessToken-handler branch 2 times, most recently from32b95c8 to900b87aCompareMay 4, 2023 14:31
@nicolas-grekasnicolas-grekas modified the milestones:6.3,6.4May 23, 2023
@nacorpnacorpforce-pushed thesecurity-add-CAS-AccessToken-handler branch from900b87a tof1d7218CompareSeptember 21, 2023 12:23
@welcoMatticwelcoMattic added the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelOct 6, 2023
@nacorpnacorpforce-pushed thesecurity-add-CAS-AccessToken-handler branch fromc4cf678 tod53eecfCompareOctober 7, 2023 12:47
@nicolas-grekasnicolas-grekas removed the ❄️ Feature FreezeImportant Pull Requests to finish before the next Symfony "feature freeze" labelOct 17, 2023
@welcoMatticwelcoMattic modified the milestones:6.4,7.1Oct 17, 2023
*/
publicfunctiongetUserBadgeFrom(string$accessToken):UserBadge
{
$response =$this->client->request('GET',$this->getvalidationUrl($accessToken));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$response =$this->client->request('GET',$this->getvalidationUrl($accessToken));
$response =$this->client->request('GET',$this->getValidationUrl($accessToken));

@fabpotfabpotforce-pushed thesecurity-add-CAS-AccessToken-handler branch from6c51e6c toa1be5dfCompareFebruary 3, 2024 15:25
@fabpot
Copy link
Member

Thank you@nacorp.

{
$response =$this->client->request('GET',$this->getValidationUrl($accessToken));

$xml =new \SimpleXMLElement($response->getContent(),0,false,$this->prefix,true);
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the extension is missing?

nicolas-grekas added a commit that referenced this pull requestFeb 9, 2024
…ersion (alamirault)This PR was merged into the 7.1 branch.Discussion----------[Security] Update CAS 2.0 AccessTokenHandler changelog version| Q             | A| ------------- | ---| Branch?       | 7.1| Bug fix?      | no| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Issues        | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->| License       | MIT#48276 has been merged in 7.1, not 6.4Commits-------56e2a41 [Security] Update CAS 2.0 AccessTokenHandler changelog version
@fabpotfabpot mentioned this pull requestMay 2, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@stofstofstof left review comments

@OskarStarkOskarStarkOskarStark left review comments

@fabpotfabpotfabpot approved these changes

@welcoMatticwelcoMatticwelcoMattic approved these changes

@wouterjwouterjAwaiting requested review from wouterj

+1 more reviewer

@JeroenyJeroenyJeroeny left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

10 participants

@nacorp@carsonbot@drupol@welcoMattic@chalasr@fabpot@stof@OskarStark@Jeroeny@nicolas-grekas
[8]ページ先頭
©2009-2025 Movatter.jp