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

[FrameworkBundle][Workflow] Add a way to register a guard expression in the configuration#21935

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
fabpot merged 1 commit intosymfony:masterfromlyrixx:workflow-security
Mar 14, 2017

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedMar 8, 2017
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
Doc PR-

Many people already asked for this feature so ... here we go 🎉


Usage:

transitions:journalist_approval:guard:"is_fully_authenticated() and has_role('ROLE_JOURNALIST') or is_granted('POST_EDIT', subject)"from:wait_for_journalistto:approved_by_journalistpublish:guard:"subject.isPublic()"from:approved_by_journalistto:published

@stof
Copy link
Member

stof commentedMar 8, 2017

third option: do as the SecurityListener in FrameworkExtraBundle: allow expressions, but not by using the ExpressionVoter itself, allowing you to exposeis_granted to call the full-featured access decision logic (for instance usingis_granted('EDIT', subject) to vote on the subject rather than just onnull)

Pros:

  • very flexible
  • easy for simple checks (if you also expose the functions available inside the ExpressionVoter language, as done in@Security).
  • we can expose additional variables in the expression if needed, as we control it (the workflow name, the transition name, etc... depending on what actually makes sense)
  • we can add more workflow-specific functions (a way to check whether another place is also marked, not sure whether it makes sense ?)

I see several cons in your option 2:

  • not allowing to vote on an object (the one being marked is a candidate here), or enforcing it depending on the implementation (both ways can have issues, as we need to be able to do both of them)
  • not flexible at all when wanting to implement more complex checks, as expressions are not available (not sure why you listed this as a Pro. It is not one IMO). And custom voters don't help much here if there is no access to the subject.

@lyrixx
Copy link
MemberAuthor

@stof Thanks. That's very cool. I did not think to do that ;) I will do that !

Just to clarify your point about 2/: I said it's more flexible because in the end you write pure PHP. That's why it is the most flexible solution. Thus you have access to the object in the voter (because the listener pass it to the AuthChecker)

@stof
Copy link
Member

stof commentedMar 8, 2017

@lyrixx but forcing to pass it is not always the most flexible. I have voters which explicitly vote onnull and abstain when being asked to vote on an object (because a most specific voter is responsible for it). The RoleVoter accepts anything (it ignores the subject entirely) so it does not hurt to pass it in this case, but it is less flexible.

@lyrixx
Copy link
MemberAuthor

I pushed the new version ;) Ready for review.

@linaori
Copy link
Contributor

More of a semantic, but why 'guard' instead of 'authorization'?

@lyrixx
Copy link
MemberAuthor

@iltar usually people useguard for this kind of things. More over the event is namedguard.

refs:

Thus, here you can block a transition not only with the security but with, for example, the subject itself. So authorization seems too "security related" to me.

@linaori
Copy link
Contributor

linaori commentedMar 8, 2017
edited
Loading

The expression seems to be used for authorization, hence I was wondering. Guard sounds fine in this case though.

@lyrixx
Copy link
MemberAuthor

I updated the PR description to add another example where we don't use the security. I guess it's better now.

linaori reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the3.3 milestoneMar 9, 2017
@nicolas-grekas
Copy link
Member

In routing, this is called "condition", maybe better and more consistent?

@lyrixx
Copy link
MemberAuthor

In that case, it will not be consistent with the workflow. to me,guard is better.

jvasseur reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

Fabbot is red, and deps=low line also

@lyrixxlyrixxforce-pushed theworkflow-security branch 2 times, most recently frome3b0355 toe00d2f4CompareMarch 10, 2017 09:53
@lyrixx
Copy link
MemberAuthor

@nicolas-grekas Rebase + Fixed tests. Now it's green.

@stof Is it what you had in mind ?

@lyrixx
Copy link
MemberAuthor

👍

@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commitab3b12d intosymfony:masterMar 14, 2017
fabpot added a commit that referenced this pull requestMar 14, 2017
…ard expression in the configuration (lyrixx)This PR was merged into the 3.3-dev branch.Discussion----------[FrameworkBundle][Workflow] Add a way to register a guard expression in the configuration| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | ----Many people already asked for this feature so ... here we go 🎉---Usage:```yml            transitions:                journalist_approval:                    guard: "is_fully_authenticated() and has_role('ROLE_JOURNALIST') or is_granted('POST_EDIT', subject)"                    from: wait_for_journalist                    to: approved_by_journalist                publish:                    guard: "subject.isPublic()"                    from: approved_by_journalist                    to: published```Commits-------ab3b12d [FrameworkBundle][Workflow] Add a way to register a guard expression in the configuration
@lyrixxlyrixx deleted the workflow-security branchMarch 14, 2017 23:40
@fduch
Copy link
Contributor

This functionality (and also other extensions) was implemented inhttps://github.com/GlobalTradingTechnologies/workflow-extensions-bundle long time ago)

See for exampletransition blocking feature

lyrixx added a commit that referenced this pull requestApr 5, 2017
This PR was merged into the 3.3-dev branch.Discussion----------[Workflow] sync the changelog| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#20751,#21334,#21933,#21935,#21950| License       | MIT| Doc PR        |Commits-------98a18ee [Workflow] sync the changelog
@fabpotfabpot mentioned this pull requestMay 1, 2017
@destillat
Copy link
Contributor

What about adding guard expressions forworkflow.guard andworkflow.id.guard events too?

@lyrixx
Copy link
MemberAuthor

Hello@destillat

you are commenting a closed/merged PR. So I suggest you to open a new issue instead.
When you will do that, please add more information because ATM, I'm not sure to understand what you want.

Thanks.

@bendavies
Copy link
Contributor

FYI this is not documented

@lyrixx
Copy link
MemberAuthor

@bendavies

you are commenting a closed/merged PR.
Could you open an issue (if it does not exist) in the doc repo ?https://github.com/symfony/symfony-docs

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

9 participants

@lyrixx@stof@linaori@nicolas-grekas@fabpot@fduch@destillat@bendavies@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp