Skip to content

Match slashes in ../{id} resource routes#4138

Merged
MorrisJobke merged 1 commit into
masterfrom
resources_match_fullid
Apr 4, 2017
Merged

Match slashes in ../{id} resource routes#4138
MorrisJobke merged 1 commit into
masterfrom
resources_match_fullid

Conversation

@rullzer

@rullzer rullzer commented Mar 29, 2017

Copy link
Copy Markdown
Member

Fixes #2954

Before we could match on /{id} however if the id contains a /
this would not match properly. But since we define the resource routes
internally we now make sure that we match all chars (up until the ?).

to test:

  1. create a group foo/bar
  2. Delete the group

Before: nope
Now: yes, master

CC: @hirschrobert

@rullzer rullzer added this to the Nextcloud 12.0 milestone Mar 29, 2017
@mention-bot

Copy link
Copy Markdown

@rullzer, thanks for your PR! By analyzing the history of the files in this pull request, we identified @BernhardPosselt, @Xenopathic and @icewind1991 to be potential reviewers.

@nickvergessen nickvergessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should specify that on the routes which are affected:
https://github.com/nextcloud/notifications/blob/master/appinfo/routes.php#L24

Not in general. And with your current approach I think you even kill existing restrictions?

@nickvergessen

Copy link
Copy Markdown
Member

This will also cause problems with:
/groups/{groupId} vs. /groups/{groupId}/subadmins ?! (provisioning api) also the name does not match there, so they still wouldnt allow slashes...

@rullzer

rullzer commented Mar 29, 2017

Copy link
Copy Markdown
Member Author

we don't do it in general. Only for resources like https://github.com/nextcloud/server/blob/master/settings/routes.php#L40

I see no use case where you'd generate a resource url like <prefix>/foo/bar and only want to match on foo and ignore bar (because bar will be ignored as the resources specifies itself as <prefix>/{id})

@rullzer

rullzer commented Mar 29, 2017

Copy link
Copy Markdown
Member Author

@nickvergessen so this way now or are you going to rewrite it to use the provisioning api?

@nickvergessen nickvergessen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah let's try it

@rullzer
rullzer requested a review from BernhardPosselt April 3, 2017 06:53
Fixes #2954

Before we could match on <prefix>/{id} however if the id contains a /
this would not match properly. But since we define the resource routes
internally we now make sure that we match all chars (up until the ?).

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@rullzer
rullzer force-pushed the resources_match_fullid branch from 44ed097 to 31f9be7 Compare April 4, 2017 06:37

@MorrisJobke MorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke
MorrisJobke merged commit fa41078 into master Apr 4, 2017
@MorrisJobke
MorrisJobke deleted the resources_match_fullid branch April 4, 2017 20:54
@MorrisJobke

Copy link
Copy Markdown
Member

Reverted in #4381

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cannot delete group with a slash in its name

4 participants