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

[10.x] Add arguments to the signed middleware to ignore properties #46987

Merged
merged 7 commits into from
May 10, 2023

Conversation

WendellAdriel
Copy link
Contributor

@WendellAdriel WendellAdriel commented May 8, 2023

Why

Based on this tweet from Claudio Dekker. This allows passing properties to the signed middleware to ignore properties instead of extending it and adding the keys to the $ignore property.

Also, this adds the possibility to pass the ignore parameters to the static methods: relative and absolute.

Pros of this implementation

  • Make the routes file easier to understand
  • Remove a lot of 'custom' Middleware files that individually take up a lot of 'visual' space while providing near-zero real value

Result

Middleware

// Before
Route::get('foo')->middleware('signed');
Route::get('embed')->middleware(EmbeddableSignedRoute::class);
Route::get('callback')->middleware(SignedRouteWithCallback::class);
Route::get('relative')->middleware('signed');

// After
Route::get('foo')->middleware('signed');
Route::get('embed')->middleware('signed:embed');
Route::get('callback')->middleware('signed:callback');
Route::get('callback')->middleware('signed:callback');
Route::get('relative')->middleware('signed:relative');
Route::get('relative-embed')->middleware('signed:relative,embed');

Static Methods

ValidateSignature::absolute('foo', 'bar'); // 'Illuminate\Routing\Middleware\ValidateSignature:foo,bar'
ValidateSignature::relative('foo', 'bar'); // 'Illuminate\Routing\Middleware\ValidateSignature:relative,foo,bar'

@erikgaal
Copy link
Contributor

erikgaal commented May 8, 2023

Hi there,

I guess this is an acceptable solution. Only issue I see is when you would want to ignore the relative parameter within an absolute URL. Might be a bit pedantic, but we should not forget about about developer experience and what someone can expect when using these API's.

Can we maybe update the factory-like static methods to include ignored parameters?

ValidateSignature::absolute('ignore', 'me');

@GrahamCampbell GrahamCampbell changed the title 10.x - Add arguments to the signed middleware to ignore properties [10.x] Add arguments to the signed middleware to ignore properties May 8, 2023
@WendellAdriel
Copy link
Contributor Author

Hi there,

I guess this is an acceptable solution. Only issue I see is when you would want to ignore the relative parameter within an absolute URL. Might be a bit pedantic, but we should not forget about about developer experience and what someone can expect when using these API's.

Can we maybe update the factory-like static methods to include ignored parameters?

ValidateSignature::absolute('ignore', 'me');

That's a nice idea, let me do some testing here!

@WendellAdriel
Copy link
Contributor Author

@erikgaal added the implementation that you suggested, thanks! 😉 💪

@erikgaal
Copy link
Contributor

erikgaal commented May 8, 2023

Awesome!!

@taylorotwell
Copy link
Member

The middleware already ships in the skeleton though? You don't have to extend or do anything manually.

@WendellAdriel
Copy link
Contributor Author

@taylorotwell this is in cases like in the example where multiple routes will have different ignore parameters, in this case, it's needed to create new middleware classes to extend and add the needed parameters in the ignore/except.

With this PR the current behavior remains the same if people want to use it with the ignore/except property but it will also provide the other way to use it with the list of properties in the middleware definition.

@dennisprudlo
Copy link
Contributor

dennisprudlo commented May 8, 2023

I have not better solution but signed:embed kind of suggests that the embed parameter is actually the one that will be signed.

But every alternative I can think of (signed:except:embed,state and so on) makes it even worse 😅

Best alternative I can think of is a fluent ->signed(...) method but I don't know if this is desired.

@driesvints
Copy link
Member

I'll re-open this so @taylorotwell can have another look.

@driesvints driesvints reopened this May 9, 2023
@taylorotwell
Copy link
Member

@WendellAdriel one thought I have with this is should the passed ignore arguments override the default "except" ones totally or should they merge with them?

@WendellAdriel
Copy link
Contributor Author

@taylorotwell that's a really good question.
Right now it's set to override and I think it makes sense because you can have your default configuration on the except and if you need something custom, then you can pass manually the arguments to ignore.

@taylorotwell
Copy link
Member

@WendellAdriel I guess I can see it both ways. You may define the things you always want to ignore in the middleware. If you add additional ones on route registration I would potentially expect those (the things you want to sometimes ignore) to be merged in with the things you always want to ignore.

@WendellAdriel
Copy link
Contributor Author

@taylorotwell tomorrow morning my time I’ll update the PR with that then!

@WendellAdriel
Copy link
Contributor Author

@taylorotwell I updated the PR with some changes to allow it to merge the arguments passed in the middleware with the ignore/except property and added a new test case for that

@taylorotwell taylorotwell merged commit d9583ea into laravel:10.x May 10, 2023
15 checks passed
@taylorotwell
Copy link
Member

Thanks!

@WendellAdriel WendellAdriel deleted the signature-middleware-params branch May 10, 2023 20:28
@WendellAdriel
Copy link
Contributor Author

Thanks!

I’m happy to help!!! 💪

milwad-dev pushed a commit to milwad-dev/framework that referenced this pull request May 12, 2023
…aravel#46987)

* 10.x - Add arguments to the signed middleware to ignore properties

* Fix code style

* Add possibility to pass ignore parameters to the static methods

* Fix code style

* formatting

* Update middleware to merge arguments passed to middleware to the class ignore/except list

---------

Co-authored-by: Taylor Otwell <taylor@laravel.com>
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

5 participants