Skip to content

[FEAT] [SECURITY] OAuth component #31937

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

Closed
wants to merge 132 commits into from
Closed

[FEAT] [SECURITY] OAuth component #31937

wants to merge 132 commits into from

Conversation

Guikingone
Copy link
Contributor

Q A
Branch? 4.4 ?
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #...
License MIT
Doc PR To add

Hi everyone 👋

Here's my first "big" PR on Symfony, recently, I've decided to implement OAuth authentication in a side-project application, in order to do it, I've used HttpClient (thanks to Nicolas Grekas) and I've been amazed by the simplicity of the implementation but something was weird ... Why Symfony doesn't have a "native" OAuth support/Client/Server/Component?
Then I saw that the hackaton highlighted the idea of having a native support: #30914

So, I've decided to take time to work on a strict implementation (well, as strict as I could do) thanks to the RFC and HttpClient and here's the first result, a new "OAuth component" (actually, that's an experimentation 😄), for now, this experimentation only support the "client" aspect (the server has been started) and that I'm aware that a lot of feature can be lacking or to update.

So, why opening this PR now ?

First, I'm aware that this is a WIP and that a lot of feedback can be linked to some choices that I've made, please, do not hesitate to explain me if there's a better option/way to do it.

Second, I think that it's a good idea to discuss about this idea and explain why it's not a good idea/option to do it as a component (or even as a feature?) before going further on the development 🙂

So here's the PR with the first parts of the components, feel free to comments and have a great day 🙂

fabpot and others added 30 commits May 28, 2019 14:18
* 4.4:
  bumped Symfony version to 4.3.0
  updated VERSION for 4.3.0-RC1
  updated CHANGELOG for 4.3.0-RC1
  Create an abstract HTTP transport and extend it in all HTTP transports
  Updated "experimental" annotations for 4.3
* 4.4:
  Allow Symfony 5.0
This PR was merged into the 5.0-dev branch.

Discussion
----------

Bump Symfony 5.0 to PHP 7.2

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Not ready yet.

7.2 because it has all we need for the next two years (7.3 doesn't provide anything we absolutely need, and 7.4 is not even released so cannot be considered yet.)

7.2.9 because of https://tracker.debian.org/pkg/php7.2

Needs:
- [x] symfony/security-acl#48
- [x] doctrine/orm#7723
- [x] doctrine/DoctrineBundle#975
- [x] doctrine/DoctrineCacheBundle#158
- [x] #31657

Commits
-------

d94d9d7 Bump Symfony 5.0 to PHP 7.2
This PR was merged into the 5.0-dev branch.

Discussion
----------

Bump deps to ^4.4|^5.0 for Symfony 5.0

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Our usual dependency cleanup.

Commits
-------

2dffc58 Bump deps to ^4.4|^5.0 for Symfony 5.0
…trict_variables option (yceruto)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[TwigBundle] Remove default value deprecation for twig.strict_variables option

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

See previous PR in 4.1 #25780

Commits
-------

8320737 Remove default value deprecation for twig.strict_variables
* 4.4:
  fix typo
  [Translation] fix dep
This PR was merged into the 5.0-dev branch.

Discussion
----------

[Validator] Remove checkDNS option in Url

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

See #25516

Commits
-------

885703f [Validator] Remove checkDNS option in Url
This PR was merged into the 5.0-dev branch.

Discussion
----------

[Validator] Remove fallback dependency checks

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

See #28644

Commits
-------

aa84cfd     [Validator] Remove fallback dependency checks
…ors (ro0NL)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Validator] Remove DateTime support in date/time validators

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

See #25015

Commits
-------

37c1cbb [Validator] Remove DateTime support in date/time validators
This PR was merged into the 5.0-dev branch.

Discussion
----------

[HttpKernel] Cleanup legacy in ConfigDataCollector

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

See #28939

Commits
-------

09ca33c [HttpKernel] Cleanup legacy in ConfigDataCollector
This PR was squashed before being merged into the 5.0-dev branch (closes #31671).

Discussion
----------

[Intl] Cleanup legacy  API

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes (including intl-data group)
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Commits
-------

67105d2 [Intl] Cleanup legacy  API
This PR was merged into the 5.0-dev branch.

Discussion
----------

[Messenger] minor: Remove logging middleware

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | none   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#11619 <!-- required for new features -->

<!--
Replace this notice by a short README for your feature/bugfix. This will help people
understand your PR and can be used as a start for the documentation.

Additionally (see https://symfony.com/roadmap):
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the master branch.
-->

Removes the LoggingMiddlewere

Commits
-------

2e6fbd5 [Messenger] minor: Remove logging middleware
This PR was merged into the 5.0-dev branch.

Discussion
----------

Remove dead code

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

cleanup

Commits
-------

ceb83e6 Remove dead code
…e legacy translation directories (yceruto)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[FrameworkBundle] Removed capability to load/debug/update legacy translation directories

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Ref: #28997

Commits
-------

b6eb1f4 Removed capability to load/debug/update legacy translation directories
…nterface in intl forms (yceruto)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Form] Remove deprecated implementation of ChoiceLoaderInterface in intl forms

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

See previous PR #26825 (4.1)

Commits
-------

b22cbf4 Remove deprecated implementation of ChoiceLoaderInterface in intl forms
nicolas-grekas and others added 10 commits June 6, 2019 18:29
* 4.4:
  [FrameworkBundle] fix compat with some SF5 components
* 4.4:
  fix typo
This PR was merged into the 5.0-dev branch.

Discussion
----------

[Validator] remove deprecated code paths

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Commits
-------

7bb0359 [Validator] remove deprecated code paths
…grekas)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[FrameworkBundle] remove deprecated code paths

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Needs #31898

Commits
-------

30418cf [FrameworkBundle] remove deprecated code paths
…der` service (yceruto)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[FrameworkBundle] Fixed replace argument of `routing.loader` service

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

Fix #31702
> (1/1) OutOfBoundsException
The index "2" is not in the range [0, 1].

https://github.com/symfony/symfony/blob/bd8d8a2cfd50dffb4aa07908865931fb82a9e7bf/src/Symfony/Bundle/FrameworkBundle/Routing/DelegatingLoader.php#L33

Commits
-------

5e2e79d Fixed replace argument of `routing.loader` service
* 4.4:
  Several components are incompatible with HttpKernel 5.0
  [Validator] fix deprecation layer of ValidatorBuilder
  [HttpKernel] Fix missing use for request and response classes
  bumped Symfony version to 4.3.2
  updated VERSION for 4.3.1
  updated CHANGELOG for 4.3.1
…lyrixx)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[Workflow] Removed un-used dependency in composer.json

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

---

I forgot to remove this deps in #31824

Commits
-------

28f2dac [Workflow] Removed un-used dependency in composer.json
tests(components): progress on token & provider tests
@nicolas-grekas
Copy link
Member

should target 4.4, master is only for deprecated code removals :)

@nicolas-grekas nicolas-grekas added this to the next milestone Jun 7, 2019
->scalarNode('redirect_uri')
->isRequired()
->end()
->scalarNode('accessToken_url')
Copy link
Contributor

Choose a reason for hiding this comment

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

mixed camelCase with underscore

->end()
->scalarNode('access_token_url')
->isRequired()
->end()
Copy link
Contributor

Choose a reason for hiding this comment

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

imo most of these options shouldn't have a "default", as for a client everything should be specific (e.g I shouldn't have the same key for multiple providers).

Especially as all of these (or almost) are required for each providers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I'm gonna change this 🙂

@Guikingone Guikingone changed the base branch from master to 4.4 June 8, 2019 06:20
@Guikingone Guikingone changed the base branch from 4.4 to master June 8, 2019 06:21
@Guikingone Guikingone changed the base branch from master to 4.4 June 8, 2019 06:28
@Guikingone
Copy link
Contributor Author

Guikingone commented Jun 8, 2019

Hi everyone 👋

This PR is closed, I've created a new one (wrong rebase process on this one 🙁 ) -> #31952

Sorry for the new one :(

@Guikingone Guikingone closed this Jun 8, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
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.