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 Authenticator for Symfony 5.1+ #95

Merged
merged 2 commits into from
Jun 14, 2020
Merged

Add Authenticator for Symfony 5.1+ #95

merged 2 commits into from
Jun 14, 2020

Conversation

fabpot
Copy link
Contributor

@fabpot fabpot commented Jun 13, 2020

No description provided.

Copy link

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

I think the code looks fine.

There are 2 things in this PR that makes me think about the security on a more generic level:

A) I see you're using custom passports to transfer information from the authenticate() method to the createAuthenticatedToken() method. That feels like we're still missing something in the passport - e.g. a way to add arbitrary information to a passport (a badge needs verification and a custom passport requires quite a lot of work to share some data)

B) Seems like this authenticator is quite generic (but I have no experience working with oauth2 systems). Do you think it's feasible to implement something like this class as an oauth 2 authenticator in Symfony itself (or at least a base authenticator)?

@fabpot fabpot merged commit f8f435c into master Jun 14, 2020
@fabpot fabpot deleted the authenticator branch June 14, 2020 14:00
@fabpot
Copy link
Contributor Author

fabpot commented Jun 14, 2020

A) I think we could have "attributes" on SelfValidatingPassport?
B) Probably yes, see https://github.com/knpuniversity/oauth2-client-bundle / https://github.com/hwi/HWIOAuthBundle though

fabpot added a commit to symfony/symfony that referenced this pull request Jun 20, 2020
This PR was merged into the 5.2-dev branch.

Discussion
----------

[Security] Add attributes on Passport

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | n/a <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | not yet

see symfonycorp/connect#95
/cc @wouterj

Commits
-------

440ada3 [Security] Add attributes on Passport
symfony-splitter pushed a commit to symfony/security-http that referenced this pull request Jun 20, 2020
This PR was merged into the 5.2-dev branch.

Discussion
----------

[Security] Add attributes on Passport

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | yes <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | n/a <!-- prefix each issue number with "Fix #", if any -->
| License       | MIT
| Doc PR        | not yet

see symfonycorp/connect#95
/cc @wouterj

Commits
-------

440ada3c5f [Security] Add attributes on Passport
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.

2 participants