Allow group 0 to be created by provisioning API #29712

phil-davis · 2017-11-29T10:03:59Z

Description

Only the empty string should be rejected as a group name (or things that == the empty string such as null and false). This lets "0" be created as a group name.

Related Issue

#29698

Motivation and Context

Zero and empty must not live in the same house.

How Has This Been Tested?

The new integration test fails before and passes after this fix.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2017-11-29T12:50:39Z

Codecov Report

Merging #29712 into master will not change coverage.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #29712   +/-   ##
=========================================
  Coverage     62.49%   62.49%           
- Complexity    17619    17621    +2     
=========================================
  Files          1045     1045           
  Lines         58070    58070           
=========================================
  Hits          36288    36288           
  Misses        21782    21782

Impacted Files	Coverage Δ	Complexity Δ
apps/provisioning_api/lib/Groups.php	`100% <100%> (ø)`	`23 <0> (+2)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17f398b...6b7b87c. Read the comment docs.

PVince81 · 2017-11-29T13:03:56Z

apps/provisioning_api/lib/Groups.php

@@ -130,7 +130,7 @@ public function getGroup($parameters) {
 	public function addGroup($parameters) {
 		// Validate name
 		$groupId = $this->request->getParam('groupid', '');
-		if(empty($groupId)){
+		if($groupId == ''){


can you make that === ? because == might also match null values. If we also want to check for nulll values, also add is_null($groupId)

I wanted to match all of:

if ($group === "") if (is_null($group)) if ($group === false)

and those 3 things all are done in 1 statement by:

if ($group == "")

In theory the return value of getParam is always a string - it "should" never be null or false, but who knows.

you could check if $group is a string and if not also show the same error. Or even throw an exception because if the the group name is not a string something went terrible wrong.

Way too much time spent on a 1-line fix. Somebody tell me what code will be accepted for review and then I can write that. Otherwise I will just be guessing what is wanted and going round-and-round the long CI process.

individual-it · 2017-11-29T15:21:21Z

Why is Jenkins failing? Do the Scenarios depend on each other?

phil-davis · 2017-11-29T15:52:05Z

Yes, it does not look ideal - groups that are created by when sending POST to "/cloud/groups" are not deleted after each scenario so they "add up". It means that some scenarios cannot be run just by themselves. So I will need to do some work to sort that out.

(creating users has the same issue)

phil-davis · 2017-11-29T17:10:13Z

tests/integration/features/provisioning-v1.feature

@@ -140,13 +150,16 @@ Feature: provisioning

 	Scenario: Getting all groups
 		Given as an "admin"
+		And group "0" exists


I "fixed up" this test so that at least it explicitly specifies each group to exists then checks that they are returned by the /cloud/groups endpoint.
Actually each group (except admin) is created in one of the previous scenarios by code that does:

When sending "POST" to "/cloud/groups" with

That step does not "remember" that it created a group, because that is a generic step for "driving the API". So in those kind of scenarios, the created group is not deleted by the AfterScenario code.

Thus this scenario would actually pass without specifically needing to mention that the 4 groups "exist".

Creating users has the same problem - various scenarios create users that are not deleted in the AfterScenario code. But all that sort of thing needs to be looked at separately, otherwise this "little" PR will become "bigger than Ben Hur" if it starts rectifying existing "features" of the test infrastructure.

Changes since original review - please review again.

phil-davis · 2017-11-30T14:15:22Z

apps/provisioning_api/lib/Groups.php

@@ -130,7 +130,7 @@ public function getGroup($parameters) {
 	public function addGroup($parameters) {
 		// Validate name
 		$groupId = $this->request->getParam('groupid', '');
-		if(empty($groupId)){
+		if(($groupId === '') || is_null($groupId) || ($groupId === false)){


@PVince81 I went for the full list here - empty() essentially means "blank or null or false or 0 or '0'". We want 0 or '0' to be allowed and the others can continue to be banned as a group name.

phil-davis · 2017-12-01T07:19:45Z

Backport stable10 #29734

lock · 2019-08-01T20:02:02Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

phil-davis added 3 - To Review app:provisioning_api labels Nov 29, 2017

phil-davis added this to the development milestone Nov 29, 2017

phil-davis self-assigned this Nov 29, 2017

phil-davis requested a review from individual-it November 29, 2017 10:03

phil-davis force-pushed the api-create-group-0 branch from e37b857 to 46c5610 Compare November 29, 2017 10:23

individual-it previously approved these changes Nov 29, 2017

View reviewed changes

phil-davis requested a review from PVince81 November 29, 2017 12:50

phil-davis mentioned this pull request Nov 29, 2017

[stable10] Allow group 0 to be created by provisioning API #29713

Closed

PVince81 reviewed Nov 29, 2017

View reviewed changes

phil-davis force-pushed the api-create-group-0 branch from 46c5610 to e69a50a Compare November 29, 2017 17:00

phil-davis commented Nov 29, 2017

View reviewed changes

phil-davis mentioned this pull request Nov 30, 2017

[stable10] Allow group 0 to be created by provisioning API #29720

Closed

Allow group 0 to be created by provisioning API

6b7b87c

phil-davis force-pushed the api-create-group-0 branch from e69a50a to 6b7b87c Compare November 30, 2017 14:12

phil-davis commented Nov 30, 2017

View reviewed changes

butonic approved these changes Dec 1, 2017

View reviewed changes

phil-davis merged commit f1ac101 into master Dec 1, 2017

phil-davis deleted the api-create-group-0 branch December 1, 2017 07:14

phil-davis added the backport-request label Dec 1, 2017

phil-davis mentioned this pull request Dec 1, 2017

[stable10] Allow group 0 to be created by provisioning API #29734

Merged

phil-davis removed the backport-request label Dec 1, 2017

individual-it mentioned this pull request Dec 4, 2017

a group called "0" cannot be created by provisioning API #29698

Closed

ownclouders added the status/STALE label Dec 20, 2017

ownclouders mentioned this pull request Apr 26, 2018

Subadmin can create newgroup using userprovisioning API #31283

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow group 0 to be created by provisioning API #29712

Allow group 0 to be created by provisioning API #29712

phil-davis commented Nov 29, 2017 •

edited

Loading

codecov bot commented Nov 29, 2017 •

edited

Loading

PVince81 Nov 29, 2017

PVince81 Nov 29, 2017

phil-davis Nov 29, 2017 •

edited

Loading

phil-davis Nov 29, 2017

individual-it Nov 30, 2017

phil-davis Nov 30, 2017 •

edited

Loading

individual-it commented Nov 29, 2017

phil-davis commented Nov 29, 2017 •

edited

Loading

phil-davis Nov 29, 2017

phil-davis Nov 30, 2017 •

edited

Loading

phil-davis commented Dec 1, 2017

lock bot commented Aug 1, 2019

Allow group 0 to be created by provisioning API #29712

Allow group 0 to be created by provisioning API #29712

Conversation

phil-davis commented Nov 29, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Nov 29, 2017 • edited Loading

Codecov Report

PVince81 Nov 29, 2017

Choose a reason for hiding this comment

PVince81 Nov 29, 2017

Choose a reason for hiding this comment

phil-davis Nov 29, 2017 • edited Loading

Choose a reason for hiding this comment

phil-davis Nov 29, 2017

Choose a reason for hiding this comment

individual-it Nov 30, 2017

Choose a reason for hiding this comment

phil-davis Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

individual-it commented Nov 29, 2017

phil-davis commented Nov 29, 2017 • edited Loading

phil-davis Nov 29, 2017

Choose a reason for hiding this comment

phil-davis Nov 30, 2017 • edited Loading

Choose a reason for hiding this comment

phil-davis commented Dec 1, 2017

lock bot commented Aug 1, 2019

phil-davis commented Nov 29, 2017 •

edited

Loading

codecov bot commented Nov 29, 2017 •

edited

Loading

phil-davis Nov 29, 2017 •

edited

Loading

phil-davis Nov 30, 2017 •

edited

Loading

phil-davis commented Nov 29, 2017 •

edited

Loading

phil-davis Nov 30, 2017 •

edited

Loading