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

[HttpKernel] Add temporary URI signed#51502

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

@BaptisteContreras
Copy link
Contributor

@BaptisteContrerasBaptisteContreras commentedAug 27, 2023•
edited
Loading

QA
Branch?7.1
Bug fix?no
New feature?yes
Deprecations?no
TicketsFix#51501
LicenseMIT
Doc PRTODO

This feature add the possibilty to create a temporary URI signed as mentionned in#51501.

Few things about this implementation :

  • I added an optional parameter to the constructor of theUriSigner. I'm not sure if it can be considered as aBC ?
  • The query parameter for expiration (by default "_expiration") is considered as areserved parameter for the signer and if you try to sign an URI with this parameter, the method will throw
    a LogicalException. In fact, thecheckmethod will test the presence of the expiration query parameter to determine if it must (or not) verify the expiration.
    A problem can arise in this case : Lets say you want to sign this URI : "/demo?_expiration=foo" and you don't want a temporary URL. The URI is signed without problem but when you try to callcheck, the method will notice the
    presence of "_expiration" and try to determine if it has expired, considering the value "foo" as a timestamp.

@carsonbotcarsonbot added this to the6.4 milestoneAug 27, 2023
@BaptisteContrerasBaptisteContrerasforce-pushed theadd-expiration-in-uri-signer branch 4 times, most recently from95835c6 to9f88d63CompareAugust 27, 2023 15:59
@BaptisteContreras
Copy link
ContributorAuthor

PR updated :)

@OskarStark
Copy link
Contributor

Can you please rebase? Thanks

BaptisteContreras reacted with thumbs up emoji

@OskarStarkOskarStark requested review fromstof and removed request forstofOctober 10, 2023 14:01
@BaptisteContrerasBaptisteContreras changed the title[HttpKernel] Add temporary URI signed[HttpFoundation] Add temporary URI signedOct 30, 2023
@BaptisteContrerasBaptisteContrerasforce-pushed theadd-expiration-in-uri-signer branch 2 times, most recently from910ee02 to4614ba2CompareOctober 30, 2023 14:23
@nicolas-grekasnicolas-grekas modified the milestones:6.4,7.1Nov 15, 2023
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM with some comments thanks


privatefunctionisExpired(int$expiration):bool
{
returntime() >=$expiration;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe using Clock component optionally would be better with fallback, to allow using mock clock in tests for devs?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

It will create a dependency between HttpFoundation and the Clock component. How would you do it ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Require-dev? :)

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Mmmh I see, and in thecheck method I should get the current time like that ?

if (class_exists(\Symfony\Component\Clock\Clock::class)) {$time =  \Symfony\Component\Clock\Clock::get()->now()->format('U');        }else {$time =time();        }

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably. Or via DI, not sure what maintainers would prefer

Copy link
Member

@nicolas-grekasnicolas-grekasNov 23, 2023•
edited
Loading

Choose a reason for hiding this comment

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

I wouldn't use the Clock component in this PR. Let's use time() for now and discuss for Clock applied to HttpFoundation separately

BaptisteContreras reacted with thumbs up emoji
@BaptisteContreras
Copy link
ContributorAuthor

@nicolas-grekas if this PR is OK for you, can we have this in 7.1 ?

@carsonbotcarsonbot changed the title[HttpFoundation] Add temporary URI signed[HttpKernel] Add temporary URI signedFeb 25, 2024
@fabpot
Copy link
Member

@BaptisteContreras Are you still interested in finishing this PR?

@BaptisteContreras
Copy link
ContributorAuthor

@fabpot , yes ! I'm on it :)

@BaptisteContrerasBaptisteContrerasforce-pushed theadd-expiration-in-uri-signer branch from59ca1eb tof0c2cfbCompareMarch 17, 2024 17:35
@fabpot
Copy link
Member

Thank you@BaptisteContreras.

* The expiration is added as a query string parameter.
*/
publicfunctionsign(string$uri):string
publicfunctionsign(string$uri,\DateTimeInterface|\DateInterval|int|null$expiration =null):string
Copy link
Member

@nicolas-grekasnicolas-grekasMar 25, 2024•
edited
Loading

Choose a reason for hiding this comment

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

I missed that this is a BC break. Instead, we should comment the argument on the signature and read it with func_get_arg (see on 6.4 for similar new arguments).

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Is it a BC even if the default value does not change the old behavior of the method ?
Do you want another PR linked to a new issue to fix this ?

Choose a reason for hiding this comment

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

It's a BC break for classes that extend this class. Yes, it'd be great to fix it, PR welcome :)

BaptisteContreras reacted with thumbs up emoji
fabpot added a commit that referenced this pull requestApr 9, 2024
…8.0 (xabbuh)This PR was merged into the 7.1 branch.Discussion----------[HttpFoundation] defer addition of new method argument to 8.0| Q             | A| ------------- | ---| Branch?       | 7.1| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        |Fix#51502 (comment)| License       | MITCommits-------8fadaca defer addition of new method argument to 8.0
@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

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

@fabpotfabpotfabpot approved these changes

@stofstofAwaiting requested review from stof

@OskarStarkOskarStarkAwaiting requested review from OskarStark

@norkunasnorkunasAwaiting requested review from norkunas

Assignees

No one assigned

Projects

None yet

Milestone

7.1

Development

Successfully merging this pull request may close these issues.

[HttpFoundation] Temporary URI signed

7 participants

@BaptisteContreras@OskarStark@fabpot@nicolas-grekas@stof@norkunas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp