Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
getWorkspaceId() contains this todo
/**
* {@inheritdoc}
*/
public function getWorkspaceId(Request $request) {
$workspace_id = $request->query->get('workspace') ?: NULL;
if (!$workspace_id && isset($_SESSION['workspace'])) {
// @todo Review from a security perspective.
$workspace_id = $_SESSION['workspace'];
}
return $workspace_id ?: $this->container->getParameter('workspace.default');
}
Comment | File | Size | Author |
---|---|---|---|
#41 | do_security_review_of-2597464-41.patch | 44.5 KB | timmillwood |
#26 | 2597464-post-not-get_26.patch | 31.23 KB | josephdpurcell |
#25 | interdiff-16-24.txt | 8.48 KB | josephdpurcell |
#25 | 2597464-post-not-get_25.patch | 27.27 KB | josephdpurcell |
#16 | interdiff-2597464.txt | 7.02 KB | Crell |
Comments
Comment #2
dixon_Bumping as this is security related.
Comment #3
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedMy take:
1) workspace needs to be made a global cache context. Which is something you have to do manually in sites/default/services.yml. It's easy, but needs to be documented.
2) GET parameters that change state are totally no. :-) Even if it's just session state.
3) How bad this is depends on how much other workspace access control there is in the system.
I will be working to change this to a POST anyway as part of #2658400: Switching workspaces, per request from Tim.
Comment #4
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedCorrection, I'm working on that switch for this issue now. :-)
Comment #5
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedWork in progress. This is also available in my sandbox for merging if you are so inclined: https://www.drupal.org/sandbox/crell/2663020
This replaces the list of links with a list of buttons, each of which is a tiny form. Submitting that form will switch the active workspace. I also am disabling the query parameter switching entirely.
Still todo:
1) Figure out why the layout sucks and my CSS to fix it isn't getting picked up.
2) Switch away from using $_SESSION to a non-super-global-based approach. This could use a little discussion to decide how to force a workspace. Just calling a method on the negotiator seems weird and hard-code-y, but that may be the best option available. Tim, Dixon, thoughts?
Comment #9
timmillwoodThe patch looks like a good start, not had chance to apply it yet though. Definitely needs tests fixing.
My only experience with Toolbar CSS is Deploy module, so not sure if that gives you some hints http://cgit.drupalcode.org/deploy/tree/deploy.module
Could we store the workspace against the user object? I guess we don't want to allow anonymous users to switch workspace.
Comment #10
timmillwood@Crell - I've just committed an update to multiversion add an icon to the "Workspaces" tab.
This may cause a merge conflict, but also add a working
multiversion.toolbar.css
file.Comment #11
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedOK, updated patch. The conflict was minimal so no worries there. Fixes a bug, and uses flexbox(!) to make the buttons look like links and stuff. I haven't looked into all of the flexbox fallbacks for various and sundry old browsers, but this works in current Chrome and looks pretty good to me, visually. (Someone who actually specializes in CSS may disagree.)
Addendum: I just realized it breaks in horrific ways when you switch the toolbar to vertical mode, so I'll fix that next.
Still to do: Make it not use $_SESSION directly if at all possible. I need to see where we ended up on that front for what the preferred alternative is.
The idea behind using lots of little forms here is that it allows us to organize, layout, or show only subsets of the list in other locations if appropriate. This approach allows those other cases to organize the mini forms however makes sense, including putting them into a scroll bar or similar.
Comment #15
timmillwoodWe will also need passing test ;-)
Comment #16
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedNew patch fixes the vertical layout, and removes the test for the block that I just deleted.
Now here's the problem. With this change, the only way to change the active workspace is via a form, which we place in the toolbar. That breaks any tests that used the query parameter to switch workspaces, obviously. I'm trying to update those tests, but there's a problem: The toolbar doesn't seem to be available to drupalPostForm(), even when the toolbar module is enabled. Presumably that's because it is rendered via Javascript. However, there's no other API than "mess with the session directly" for setting the workspace for more than a single request.
That suggests we need to add such an API, but also that we need to expose it somehow in a test-friendly way. I am unclear how to do that safely.
While I was at it, I also noticed the WorkspaceSwitcherInterface, which is built around exposing the old GET links. That should assumedly go away, too, but in favor of what I'm not entirely certain. Perhaps repurpose it to show the buttons? Does that mean we bring back the block I just killed, and make it use the buttons instead? Maybe, I'm not sure.
Tim, let me know what direction you want to take here. Unfortunately I have only one day left before I go to India so there's a limit to what I can do in that time.
Comment #20
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedWorkspaces appear to be non-functional when JavaScript is disabled; the "Workspaces" link goes to the home page. However, it appears much of the rest of the navigation still works.
One option would be to create a page that "Workspaces" goes to and shows the list of workspaces there, with forms, and that the web test be for the test on this page. The assumption in this scenario is that the Toolbar forms would function similarly to this new page.
Comment #21
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedWorth noting, I don't see any other navigation items that change the state of the system on click. They are all strictly for navigation.
I am unaware of any other modules that would benefit from having quick access to state-modifying actions in the nav. If there are not, the effort of modifying the test framework to support testing JavaScript generated forms may prove little value in the long term.
Alternatively, we could leave this as is, and simply write a kernel test for the form. However, I don't see any examples of that.
My inclination would be to do the following:
Thoughts?
Comment #22
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedThere are two challenges with this that I see:
Comment #23
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedPer discussion with Tim, will will proceed by making Multiversion's workspace switching accessible even when JavaScript is disabled. This gives us the benefit of testability, and accessibility for the user.
To do this, we will:
One thing to figure out: What happens if the user switches workspaces and they are on a node that is only accessible in one workspace?
Comment #24
timmillwoodSwitching workspaces should always take the user to the homepage, just as it does now from the current implementation.
Also just to check, the current implementation is staying the same when Javascript is enabled, the alternative from the workspace list page is an alternative that works without javascript.
Also for the current workspace indicator, I think something quick for now, then we can iterate on it, maybe an "Active" column with a tick in it, or something.
Comment #25
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedHere is a patch of where I'm at:
Todos:
Here is a screenshot: https://s3.amazonaws.com/uploads.hipchat.com/51684/970789/sMARdt0HgMtv7O...
Any early feedback is welcome. I plan to continue this next week.
Comment #26
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedThis takes care of 1 and 4 from #25, but still need to complete the rest of todos, primarily removing old logic and consolidating code.
Additionally, we need to submit a patch to Workspaces module for any changes that should be migrated over there.
Setting to "Needs review" just to see if tests pass.
Comment #30
timmillwoodTests are currently failing because DrupalCI pulls in Drupal 8.1.x and Multiversion currently only works with 8.0.x
Comment #31
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedHmm... k. If that is the case, this patch may very well fail too. I've made some major changes, which will likely need discussion. I'm going to submit it anyway to see if tests do in fact pass.
Comment #35
timmillwoodA couple of nit picks, I think we should add
font-weight: normal;
to the toolbar buttons, and remove/overridebox-shadow
on button hover.I think we discussed keeping the switcher block, just updating it to have the toolbar like buttons in it.
Not sure I like the two steps needed to set active workspace from the workspace collection list, but not sure there's a way around it. I do like having the ability to set active from there.
Comment #36
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedThe BlockTest is giving me fits because we're no longer dealing with GET requests. Typical WebTestBase works well when you don't need to do any interactions, like clicking.
My thought is to use BrowserTestBase and trigger a form submission on the block (since the block now has a series of forms instead of links). Yet, I'm having trouble getting that to succeed. I'm attaching the patch anyway. I'll follow up tomorrow.
Comment #37
josephdpurcell CreditAttribution: josephdpurcell at Palantir.net for Acquia commentedComment #41
timmillwoodGot the test working, also changed it to be a switcher test and testing the Activate form too.
Then added a bit of CSS to the block to make the buttons look like pretty links.
Comment #43
timmillwoodOur Travis tests seem to be ok https://travis-ci.org/dickolsson/drupal-multiversion/jobs/113627498
Comment #48
timmillwood