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

[Security] add CAS 2.0 AccessToken handler #48276

Merged
merged 1 commit into from
Feb 3, 2024

Conversation

nacorp
Copy link
Contributor

@nacorp nacorp commented Nov 21, 2022

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR in progress

Hello,

Thanks to the new access token, I've added the CAS 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\QueryAccessTokenExtractor
        arguments:
            - 'ticket'

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

@carsonbot
Copy link

Hey!

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

Symfony has a contribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (see https://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (see https://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

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

drupol commented Nov 28, 2022

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
Contributor Author

nacorp commented Dec 1, 2022

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

drupol commented Dec 1, 2022

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

drupol commented Dec 20, 2022

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
Contributor Author

nacorp commented Dec 20, 2022

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 commented Dec 21, 2022

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
Contributor Author

nacorp commented Dec 21, 2022

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
Contributor Author

nacorp commented Jan 20, 2023

poke @welcoMattic :-)

@nacorp nacorp requested review from welcoMattic and removed request for wouterj and chalasr January 20, 2023 18:58
@chalasr
Copy link
Member

chalasr commented Jan 21, 2023

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

@nacorp
Copy link
Contributor Author

nacorp commented Jan 23, 2023

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

@nacorp nacorp force-pushed the security-add-CAS-AccessToken-handler branch 2 times, most recently from 32b95c8 to 900b87a Compare May 4, 2023 14:31
@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nacorp nacorp force-pushed the security-add-CAS-AccessToken-handler branch from 900b87a to f1d7218 Compare September 21, 2023 12:23
@welcoMattic welcoMattic added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 6, 2023
@nacorp nacorp force-pushed the security-add-CAS-AccessToken-handler branch from c4cf678 to d53eecf Compare October 7, 2023 12:47
@nicolas-grekas nicolas-grekas removed the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 17, 2023
@welcoMattic welcoMattic modified the milestones: 6.4, 7.1 Oct 17, 2023
*/
public function getUserBadgeFrom(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));

@fabpot fabpot force-pushed the security-add-CAS-AccessToken-handler branch from 6c51e6c to a1be5df Compare February 3, 2024 15:25
@fabpot
Copy link
Member

fabpot commented Feb 3, 2024

Thank you @nacorp.

@fabpot fabpot merged commit 3a4889f into symfony:7.1 Feb 3, 2024
4 of 9 checks passed
{
$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 request Feb 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.4

Commits
-------

56e2a41 [Security] Update CAS 2.0 AccessTokenHandler changelog version
@fabpot fabpot mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants