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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 14, 2017

Conversation

Projects
None yet
9 participants
@lyrixx
Member

lyrixx commented Mar 8, 2017

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:

            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
@stof

This comment has been minimized.

Member

stof commented Mar 8, 2017

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

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

This comment has been minimized.

Member

lyrixx commented Mar 8, 2017

@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

This comment has been minimized.

Member

stof commented Mar 8, 2017

@lyrixx but forcing to pass it is not always the most flexible. I have voters which explicitly vote on null 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 lyrixx force-pushed the lyrixx:workflow-security branch from d3433b5 to dadae1c Mar 8, 2017

@lyrixx

This comment has been minimized.

Member

lyrixx commented Mar 8, 2017

I pushed the new version ;) Ready for review.

@iltar

This comment has been minimized.

Contributor

iltar commented Mar 8, 2017

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

@lyrixx

This comment has been minimized.

Member

lyrixx commented Mar 8, 2017

@iltar usually people use guard for this kind of things. More over the event is named guard.

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.

@iltar

This comment has been minimized.

Contributor

iltar commented Mar 8, 2017

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

@lyrixx

This comment has been minimized.

Member

lyrixx commented Mar 8, 2017

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

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Mar 9, 2017

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 9, 2017

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

@lyrixx

This comment has been minimized.

Member

lyrixx commented Mar 9, 2017

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

@lyrixx lyrixx force-pushed the lyrixx:workflow-security branch from dadae1c to 09efa63 Mar 10, 2017

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 10, 2017

Fabbot is red, and deps=low line also

@lyrixx lyrixx force-pushed the lyrixx:workflow-security branch 2 times, most recently from e3b0355 to e00d2f4 Mar 10, 2017

@lyrixx lyrixx force-pushed the lyrixx:workflow-security branch from e00d2f4 to ab3b12d Mar 10, 2017

@lyrixx

This comment has been minimized.

Member

lyrixx commented Mar 10, 2017

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

@stof Is it what you had in mind ?

@lyrixx

This comment has been minimized.

Member

lyrixx commented Mar 14, 2017

馃憤

@fabpot

This comment has been minimized.

Member

fabpot commented Mar 14, 2017

Thank you @lyrixx.

@fabpot fabpot merged commit ab3b12d into symfony:master Mar 14, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Mar 14, 2017

feature #21935 [FrameworkBundle][Workflow] Add a way to register a gu鈥
鈥rd 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

@lyrixx lyrixx deleted the lyrixx:workflow-security branch Mar 14, 2017

@Guite Guite referenced this pull request Mar 15, 2017

Closed

Features for 2.0 target #797

@fduch

This comment has been minimized.

Contributor

fduch commented Mar 15, 2017

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

See for example transition blocking feature

lyrixx added a commit that referenced this pull request Apr 5, 2017

minor #22291 [Workflow] sync the changelog (xabbuh)
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

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

@destillat

This comment has been minimized.

Contributor

destillat commented May 4, 2018

What about adding guard expressions for workflow.guard and workflow.id.guard events too?

@lyrixx

This comment has been minimized.

Member

lyrixx commented May 7, 2018

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

This comment has been minimized.

Contributor

bendavies commented Jun 7, 2018

FYI this is not documented

@lyrixx

This comment has been minimized.

Member

lyrixx commented Jun 7, 2018

@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 free to join this conversation on GitHub. Already have an account? Sign in to comment