Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Part of #1971384: [META] Convert page callbacks to controllers
Convert these page callback to a new-style Controller, using the instructions on http://drupal.org/node/1800686 . They can share a single controller class, which takes the flood control system and config system as dependencies.
contact_flood_control() can then become a protected method on that class, using those object variables.
Comment | File | Size | Author |
---|---|---|---|
#147 | contact-page-1938390-147.patch | 17.86 KB | andypost |
#139 | interdiff.txt | 981 bytes | andypost |
#139 | drupal8.contact-module.1938390-139.patch | 17.21 KB | andypost |
#137 | drupal8.contact-module.1938390-137.patch | 16.72 KB | andypost |
#135 | drupal8.contact-module.1938390-135.patch | 16.89 KB | andypost |
Comments
Comment #1
Crell CreditAttribution: Crell commentedComment #2
Crell CreditAttribution: Crell commented.
Comment #3
disasm CreditAttribution: disasm commentedComment #4
disasm CreditAttribution: disasm commentedAttaching patch of what I did so far. Marking needs review so others can see the testbot failures. It's failing to load /contact with no category parameter. contact_category is set to 0 by default in contact.routing.yml.
Comment #6
Crell CreditAttribution: Crell commentedroute_name
route_name
This could easily get spun off to a separate access checker.
_anonymous: TRUE
If set to true, only the anon user can access it. If set to FALSE, anyone BUT the anonymous user can access it.
Because of the way access checkers stack, this can just be a _permission check. If a person has that permission, they pass. If not, return NULL and let other checkers do their thing.
I wonder if this could be its own checker? Maybe, or maybe just part of what's left of the ContactPageAccess checker.
That service can/should be injected into the access checker. Easy service to add.
This should also get the config manager injected.
Again, stackable permission checker.
Although, that does raise an interesting question of how we handle multiple permissions. Maybe the permission checker needs to allow multiple permissions, and accept any? Do we need a marker for all vs. any? Hm...
Needs docblock.
Oh dear oh dear. This is going to be a problem indeed, if global $user is going to always collide with parameters... Not sure what to do here. I'd like to have someone from the security team weigh in, because this is going to be an issue everywhere until we eliminate global $user.
Can we make eliminating global $user a critical security issue, which therefore puts pressure on the Symfony session conversion? :-)
What we should probably do here is have contactFloodControl() return a boolean, then do the actual error setting and exception throwing in the method itself. That subjectively feels cleaner, because it puts the IO in the controller method rather than in a logic method. Same above.
Of course, if we can figure out how to move configurable flood control to the access system, that would be even better.
Config manager should be an injected dependency so that we don't need to call config().
Comment #7
disasm CreditAttribution: disasm commentedI've made all the requested changes other than stacking the access checks. The way the checks are written, it's very hierarchical, for example, an admin overrides all the other checks. Also, the anonymous user in this case isn't the poster, but the account being posted too, so breaking it out into a user access check isn't viable either.
Comment #8
disasm CreditAttribution: disasm commentedComment #9
disasm CreditAttribution: disasm commentedattached fixes argument on ContactBundle
Comment #10
disasm CreditAttribution: disasm commentedThird times a charm I hope!
Comment #11
sunHm. Instead of asking for a security review, we should have a (web) test that verifies all of the existing permission permutations in HEAD, so we do not need a security review.
Apparently, that seems to be the whole point and sole purpose of the ContactPersonalTest (which thus lacks "Access" in its name). :)
That said, I agree with @disasm that the access checks performed there are fairly specific, and the order in which the checks are being performed is of importance, too. Trying to resemble that in _pseudo: _sub: ORs, and _ANDs would only result in an array(#form => array(#API => doom)).
Speaking of, I also do not understand why this access handler is registered as a service in the first place.
Comment #12
sun@disasm: D'oh! :) Please test your patches locally first. ;) I manually canceled the previous two, in order to not delay other patches in the queue. (There's no automation for that yet.)
Comment #14
disasm CreditAttribution: disasm commentedSorry sun, was trying the simplytest.me dreditor tool out (which is pretty slick). Thanks for canceling the tests for me!
Attached is a patch that successfully installed locally. It will still have the same failures from #4, but all the dependency injection should be working correctly now.
Comment #16
disasm CreditAttribution: disasm commentedAttached patch adds missing use flag.
Comment #17
Crell CreditAttribution: Crell commentedsun: I'm not as much concerned about this class specifically security-wise. I'm concerned about global $user colliding with the expectation that the parameter will be named $user if it's a User object. In Drupal 7 we're only a naming convention away from a privilege escalation attack. With the User object naming convention, we now are begging for someone to mess up the naming two-step this issue has to cause a privilege escalation bug.
That's what I'm concerned about, and don't know how to solve without just removing global $user. (Which we all want to do, but can't do yet.)
Comment #18
Crell CreditAttribution: Crell commentedComment #19
sunWhy is that an issue here? Or, why can't we do what we always do? I.e.,
function foo(User $account)
...?Comment #21
Crell CreditAttribution: Crell commentedBecause then the path pattern in the route needs to be /user/{account}, which means that we have to manually specify which converter to use so that the ID gets turned into an object. That's totally doable, but "Oh yeah, even though we automatically handle any entity you have to NOT do that for users and do it manually to avoid a security risk, because we already have a global by that name" is a really crappy answer to give module developers.
I'm inclined to file a critical for global $user and force the issue. It's been a noose around our neck for years, and it's only going to tighten. We need to just solve it.
Comment #22
disasm CreditAttribution: disasm commentedreroll!
Comment #24
kim.pepperdisasm, are you still working on this?
Comment #25
andypostPatch also should drop
_contact_personal_tab_access()
helper that used now in #1853526: Reintroduce Views integration for contact.moduleProbably should be build on top of
ContactCategoryAccessController
to replace_contact_personal_tab_access()
Comment #26
disasm CreditAttribution: disasm commentedattached is a reroll with a minor typo fix.
Comment #27
disasm CreditAttribution: disasm commentedComment #28
disasm CreditAttribution: disasm commentedJust realized this has an old style bundle for registering the access check. Attached patch switches to services.yml.
Comment #30
disasm CreditAttribution: disasm commented#28: drupal-contact_forms-1938390-28.patch queued for re-testing.
Comment #32
andypostNeeds re-roll, also there's
isPersonal()
method for category entityAnd follow-up for #1853526: Reintroduce Views integration for contact.module
needs 2 spaces from left.
Comment #33
andypostSee commited #1856556: Convert user contact form into a contact category/bundle
Comment #34
dawehnerlet's go with $user and $account but the other way round :) $account is the current active account interface and $user the user in the url.
Access checkers should always return static::ALLOW and static::DENY
Missing @file
It seems to be that this empty line is not needed.
Just use {@inheritdoc}
Instead we should use "AccounterInterface $account" in the method, so you get the current account directly without relying on global user. Once you have done this pass it into the user_access methods.
entity_load_multiple|entity_create|entity_get_form could be easily replaced by an injected entity storage controller.
This could be also injected.
Try to explain when this exception is thrown.
Try to explain when this exception is thrown.
Comment #35
ygerasimov CreditAttribution: ygerasimov commentedHere is new version of the patch taking into account comments from #32 and #34.
Comment #37
BerdirFail: 'The service "access_check.contact" has a dependency on a non-existent service "user.data".'
That test needs to enable dependency user.module.
Lots of stuff below, given that we change so much with injection and so on, let's also clean up the documentation and some weird code while we're at it.
Missing leading \ in front of Drupal.
appliesTo() went in, so this should now use that, see the other implementations for how that works.
if ($contact_account->isAnonymous())
if ($contact_account->isBlocked())
Missing @param for entity manager.
There should be no spaces between @param's.
Missing leading \
I know @dawehner requested this, but @throws, per our documentation standard, does not have a description. It's is *explicitly* documented to not have one.
I agree that it should but it's usually best to avoid discussing that in actual issues and stick to the standard.
Not sure if it makes sense to reference the hook menu implementation here, that just defines the title and might go away completely?
I don't think the submit callback still exists, that's part of the contact message entity form controller now?
And last, this is not a form (anymore) :)
In short, drop? Or replace with useful ones, like the contact message form controller?
Is there any reason why this is not just a load($default_category)? Loading all just to check if the default exists seems like a left-over of pre-entity API's.
Somewhere here the check about this not being the personal contact page and if so, throwing a not found exception got lost while re-rolling.
Missing type. And $recipient vs. $account.
Same as above.
This should actually type hint on UserInterface I think, as we pass in an entity object, not global $user.
Those classes don't exist anymore, part of the same class, so just remove it :)
Comment #38
jhodgdonBerdir asked me to comment here in IRC.
Our current docs standards say that @throws should just be followed by the name of the exception class and not a description. Presumably the Exception class name should go 90% of the way there to telling you when it would be thrown.
https://drupal.org/coding-standards/docs#throws
That said, many of the @throws tags in core now have descriptions as well, and the API module as it is now can handle this being a paragraph like @return or @param. JavaDoc supports a paragraph after the class name too. There's not probably a good reason that it should just be the class name, except that there's not much point in something like:
@throws \Whatever\Namespace\OutOfSpaceException if it's out of space
which tells you nothing that the name of the exception didn't already tell you.
So, I would be comfortable just changing the standard to match the (apparently accepted) practice in Core. In which case the standard would be to follow @throws with the name of the class and an optional description if the exception class isn't clear enough (indenting two spaces if you need to go to a new line).
Thoughts?
Comment #39
BerdirOk, opened #2045181: Document and standardize how @throws should be described. to finalize and document that. Let's keep what we have now, then.
Also, $user->hasPermission() landed, so we can clean that up too...
Comment #40
ygerasimov CreditAttribution: ygerasimov commentedThank you for the review! Very good catches. Here is updated patch.
Comment #42
jibranI think this is classic example of #2043781: Drupal::request()->attributes->get('account') may conflict with an account object loaded from the path. We should wait for this issue so we can remove
global $user
$account->hasPermission()
Comment #43
pfrenssenI was also working on this :) I had the two other calls to user_access() covered as mentioned in #42. Am looking forward to removing global $user.
Comment #44
dawehnerThat is a clear problem and should wait until $request->attributes->get('account') got renamed to _account or something else.
Comment #45
Crell CreditAttribution: Crell commented$request->attributes->get('account') has been renamed to $request->attributes->get('_account'). Carry on! :-)
Comment #46
BerdirAdditionally, it's now save to name the argument {user} and use $user.
Comment #47
dawehnerSome cleanup here and there.
Comment #49
disasm CreditAttribution: disasm commented#47: contact-1938390-47.patch queued for re-testing.
Comment #51
dawehnerMh another one :(
Comment #53
dawehner#51: contact-1938390-51.patch queued for re-testing.
Comment #55
andypostChange notice for global user https://drupal.org/node/2032447
Also access check should be different for default and configured site page so implemented
contactSitePageCategory()
Building logic moved to helper method
contactCategoryForm()
Used Translator for controller
Added tests
Cleaned tests
Comment #57
andypostremoved debug lines
Comment #59
andypost#57: contact-page-1938390-56.patch queued for re-testing.
Comment #61
dawehner+1 for the interdiff posted in #57, let's get it green in the meantime.
Comment #62
andypostMerged fix for
menu_item_route_access()
from #2040065: Remove _account from request and use the current user service instead.Most of failures are caused by absence $_account attribute so tabs are not rendered
Comment #64
andypostRevert useless split, should pass now most of broken tests
Comment #65
andypostand interdiff of all changes
Comment #66
andypostFinally should work, reverted unnecessary changes, also interdiff from 51
This hunk still required from #2040065: Remove _account from request and use the current user service instead.
Comment #67
andypostAlso generator needs injection
Out of scope: the displayed message and url should be changed to point to choose default category
Comment #69
andypostShould now pass with bits of #2057607-4: The request does not contain the _account on exception pages (403/404)
Comment #70
Crell CreditAttribution: Crell commentedThis returns TRUE/FALSE, which is wrong. It should do:
return hasPermission() ? static::ALLOW : static::DENY;
The comment isn't part of the patch, but since it needs to be updated anyway let's fix the grammar there. "Do not allow the 'personal' category to be deleted, as it's used for the personal contact form."
I would suggest we start injecting just the config object fro create(). During preparation for the Palantir training seminar this week we realized that it's really hard to test a class where we inject the config factory rather than the config object. If possible, just inject the config object.
Same here. I'd suggest injecting the storage controller only from create(). That will make this line short enough to fit on one line. :-)
Comment #71
andypost@Crell I does not get you suggestion about config factory but unified
$contactSettings
and removed config factory.Fixed 1,2,3,4
PS: This still depends on #2062151: Create a current user service to ensure that current account is always available so
contact-page-1938390-hacks.txt
provides hacks (symfony & #2057607-4)So the entity manager still needed here
Comment #72
Crell CreditAttribution: Crell commentedThis is the part I'm suggesting you move up. In the create() method, do:
$container->get('plugin.manager.entity')->getStorageController('contact_category').
Then save just the storage controller.
Then this code block can turn into just $this->contactStorage->load($default_category);
Make sense?
Comment #73
dawehnerThis contains temporary fixes from two different issues. Are we sure we want to get that in now?
We can finally return a title in the render array!
Comment #74
Crell CreditAttribution: Crell commentedComment #75
disasm CreditAttribution: disasm commentedI'll let someone more knowledgable than I to decide if we want to get those temporary fixes in now or not. This patch puts the form in a render array, and adds the title there instead of using drupal_set_title(). Also rebased, and contact.pages.inc is gone now! yay!
Comment #76
disasm CreditAttribution: disasm commentedrenaming render_array -> form
Comment #78
disasm CreditAttribution: disasm commentedAddressing #72. Also, PathBasedGeneratorInterface is now UrlGeneratorInterface.
Comment #80
jibranwe can use current user service here.
This is not needed I think.
:/
extends ControllerBase and remove the boiler plate code.
If we are doing $form['#title'] why we need dst.
This has nothing to do with controller this should be in contactManger but leave it for now.
Comment #81
disasm CreditAttribution: disasm commentedSpoke with timplunkett. We definitely shouldn't touch menu.inc, anything in core, or worse symfony vendor files in a route controller patch. If there's a reason for the changes, we need a new issue for that.
Also, addressed comments in #80. This is a single patch, but has two interdiffs, one for removing the stuff that shouldn't be in it, and the other for addressing the changes in #80.
Comment #83
disasm CreditAttribution: disasm commentedTrying this as a combined patch with #2057607: The request does not contain the _account on exception pages (403/404) and see how far we get with the tests now. If it passes, we'll postpone the issue on that.
Note to future contributors, the latest patch is in #81. DO NOT under any circumstances work off of this patch.
Comment #85
disasm CreditAttribution: disasm commentedFixing injection in controller, and using Drupal::currentUser() in access checker.
Comment #88
andypostI think access checker depends on #2048223: Add $account argument to AccessCheckInterface::access() method and use the current_user service
Comment #89
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #90
andypostCleaned up and removed out of scope changes.
Added @todo for
ContactPageAccess::access()
#2048223: Add $account argument to AccessCheckInterface::access() method and use the current_user serviceComment #91
dawehnerLet's use $this->currentUser()
Should we escape that title?
Comment #93
disasm CreditAttribution: disasm commentedAddresses comments in #91. Also, removes a bunch of injected services that aren't needed with ControllerBase.
Comment #94
jibranWorking on the fails in
ContactLinkTest
assigning to myself so that @disasm will not reroll new patch :PComment #95
jibranFixed tests.
Comment #96
jibranand unassigning.
Comment #97
dawehnerI would not call that "access manager" given that there is a different class called access manager. What about the "accss checker"?
Comment #98
jibranaccess checker it is.
Comment #99
dawehnerNice!
Comment #100
andypostRe-roll after #1981368: Convert all routes to 'module_name.foo_bar' naming convention
Comment #100.0
andypostLet's work on both page callbacks at once.
Comment #101
webchickPatch no longer applies.
Comment #102
andypostanother merge, path changes
Comment #103
alexpottPatch no longer applies.
Comment #104
andypostRe-rolls after #1834002: Configuration delete operations are all over the place
Comment #106
andypost#104: drupal8.contact-module.1938390-104.patch queued for re-testing.
Comment #107
andypostre-roll after #2089635: Convert non-test non-form page callbacks to routes and controllers
Comment #108
andypostadded changes from #2093027: Regression: contact category titles not used for page title anymore
Comment #109
tim.plunkettThis was changed in the regression issue, be sure to use the updated patch from there.
Why isn't this using the checkNamedRoute approach?
This is not needed
We have a RequestHelper instead, but see above about checkNamedRoute
Comment #111
andypost@tim.plunkett Thanx for
checkNamedRoute()
Comment #113
andypostRe-roll after #2093027: Regression: contact category titles not used for page title anymore
Also simplified code in
ContactLink
handlerComment #114
jibranThank you @tim.plunkett now i can sleep well at night. When @dawehner explained this to me in #95 I thought one line removal and 10 line addition :S.
Removing redundant tags.
Comment #115
dawehnerWrong doc.
Comment #116
andypostfixed
Comment #117
andypostIs not needed for breadcrumbs, because have no effect now
Comment #119
andypost#116: drupal8.contact-module.1938390-116.patch queued for re-testing.
Comment #120
jibranI think it is ready to fly.
Comment #121
tim.plunkett#116: drupal8.contact-module.1938390-116.patch queued for re-testing.
Comment #122
sun"page" is not a valid entity operation.
Comment #123
tim.plunkettThese are all entity operations in core:
We don't limit to create/view/update/delete...
Comment #124
sunThe operations that you listed are all operations (even linguistically). "page" is not an operation (not even linguistically).
Comment #125
andypostMakes a lot of sense to use verb here
Comment #126
sunThe operation for the contact message entity is certainly "create". That's the simple part.
Now the question is what the actual operation of the contact category is — within the scope and context of the message create operation.
Technically, you rather view the category (as in: see/view access), in order to create a message within the category.
As we're talking about entity access, "view" appears to be the proper entity operation. If you are not permitted to view the category, then you are also not allowed to create a message in that category.
Makes sense?
Comment #127
andypostYes, it makes sense too but:
1) we need a check for specific controller or route - and the global permission 'access contact forms' (view) should be extended with route-specific
ID (==|!=) 'personal'
2) dx-- when we introduce
ContactPageAccess
classes for every single route3) and moving more logic in access controller implementation so access controller operates with services and entities
Comment #128
tim.plunkettI absolutely concede the verb part here. I was just making a point that it is not restricted to the normal operation names you would assume.
@andypost's points are all valid
Comment #129
sun"contact" is definitely an improvement over "page" in terms of operation name.
But please bear in mind that this is Drupal core that you are patching. Contributed and custom modules are looking at core and will interpret this code as a desired way for doing things and as a best practice.
(Funnily, despite its stupidly simple goal and use-case, Contact module happens to encompass a range of most advanced architectural patterns.)
Do we really want contrib + custom modules to nilly-willy introduce new entity operations?
How is that going to work out in terms of security? (access permission management/control) How will I be able to control access to this new "contact" operation?
(Given the existing operations, I'm certainly able to control "view" access to contact categories, but doing so will not encompass the "contact" operation.)
Are we able to think of an entity operation name that is not limited to Contact module, and which would work for similar modules in the same space? (e.g., private messages, comment notify, etc.pp.)
Comment #130
andypost@sun I think a lot about it and following your #2100397: [meta] Ensure that DX issues identified by a recent review are covered with individual issues but I see no constructive suggestions in your comments now.
Currently core have no agreement|policy about usage of
EntityRenderController
for config entities except block that @tim.plunkett pointed in IRCCurrently core have no way to implement access controller that depends on entity ID and the 'symfony security model' is not used
@sun Issue #1839516: Introduce entity operation providers that you filed have a patch so let's discuss entity operations in appropriate issue.
I think the scope issue is find balance between separation of the logic for each access and content controllers is done
Comment #131
sunI do not understand why DX comes up as a topic here — the concern I'm raising is about entity [operations] architecture and security.
I do not know what @tim.plunkett may or may not have said in IRC, because I was not in IRC when that discussion may have happened. Whenever such discussions are happening, it is best practice to post an accurate summary to the respective issue, so as to include everyone else.
I do not understand why the proposed solution of using "view" as an operation name (or alternatively, finding an operation name that would work for other modules aside from Contact module) does not count as helpful/constructive.
For completeness, I may not have fully understood #127 — what I've heard are data points that essentially suggest "'view' would be more work, but doable."
The scope of this (and for this matter, any other) issue is always to ensure that a change (1) architecturally makes sense and (2) does not introduce code debt that will be hard to fix or get rid of later on.
Comment #132
andypost1. security - we use mix of permissions and category ID in access controller, and entity-specific access for flood at handler level (controller)
1 & 3. operation view is not good here because 'view' of Category is 'create' for Message - so I'd prefer to have entity-specific operations same time as we have entity-specific methods
2. block module the only one that uses
BlockRenderController
for config entity4. scope of this issue as part of other (meta) to make this callbacks swap-able and overridable in contrib in context of current core architecture
So I see no reason for 'make view permission work' here because then we need to put the access check into scope of route|request to make ID (personal) applies different for different routes
Comment #133
andypostLet's see if usage of 'view' could break
Comment #134
jibranBack to RTBC as 'view' is proper verb and entity operation. I hope it also fixes @sun's concerns.
Comment #135
andypostre-roll
Comment #136
alexpottPatch no longer applies.
Comment #137
andypostre-roll
Comment #139
andypostFix broken tests
Comment #140
kim.pepper#139: drupal8.contact-module.1938390-139.patch queued for re-testing.
Comment #140.0
kim.pepperUpdated issue summary.
Comment #141
pfrenssenRerolled.
Comment #143
andypostShould fix tests, the related #2105123: Add a currentUser() method to plugin base classes that need it
Comment #144
andypostre-roll
Comment #145
Crell CreditAttribution: Crell commentedI think this looks good now. Thanks!
Comment #146
alexpottNeeds a reroll
Comment #147
andypostre-roll
Comment #148
alexpottCommitted 94cba6d and pushed to 8.x. Thanks!
Removed the following use statements during commit because they are not used :)
[Edit] fix link to commit hash