Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forbid = in group names #5972

Closed
wants to merge 2 commits into from
Closed

Forbid = in group names #5972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f6784a
forbid equals in names for ldap
kobergj Mar 30, 2023
15ca806
adjust tests
kobergj Mar 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions changelog/unreleased/forbid-equal-in-groupnames.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Bugfix: Forbid `=` in group names

The underlying ldap library expects the name containing key-value pairs such as `uid=122`. It panics if you send just a `=`.
It should not. However since we cannot rely on it we forbid using `=` in group names. A `BadRequest` will now be returned instead

https://github.com/owncloud/ocis/pull/5972
9 changes: 8 additions & 1 deletion services/graph/pkg/service/v0/educationclasses.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,15 @@ func (g Graph) PostEducationClass(w http.ResponseWriter, r *http.Request) {
return
}

if _, ok := class.GetDisplayNameOk(); !ok {
name, ok := class.GetDisplayNameOk()
if !ok {
logger.Debug().Err(err).Interface("class", class).Msg("could not create class: missing required attribute")
errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "Missing Required Attribute")
return
}
if !nameAccepted(*name, w, r, logger) {
return
}

// Disallow user-supplied IDs. It's supposed to be readonly. We're either
// generating them in the backend ourselves or rely on the Backend's
Expand Down Expand Up @@ -129,6 +133,9 @@ func (g Graph) PatchEducationClass(w http.ResponseWriter, r *http.Request) {

var features []events.GroupFeature
if displayName, ok := changes.GetDisplayNameOk(); ok {
if !nameAccepted(*displayName, w, r, logger) {
return
}
features = append(features, events.GroupFeature{Name: "displayname", Value: *displayName})
}

Expand Down
10 changes: 9 additions & 1 deletion services/graph/pkg/service/v0/educationschools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,15 @@ func (g Graph) PostEducationSchool(w http.ResponseWriter, r *http.Request) {
return
}

if _, ok := school.GetDisplayNameOk(); !ok {
name, ok := school.GetDisplayNameOk()
if !ok {
logger.Debug().Interface("school", school).Msg("could not create school: missing required attribute")
errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "Missing Required Attribute")
return
}
if !nameAccepted(*name, w, r, logger) {
return
}

if _, ok := school.GetSchoolNumberOk(); !ok {
logger.Debug().Interface("school", school).Msg("could not create school: missing required attribute")
Expand Down Expand Up @@ -136,6 +140,10 @@ func (g Graph) PatchEducationSchool(w http.ResponseWriter, r *http.Request) {
return
}

if !nameAccepted(*school.DisplayName, w, r, logger) {
return
}

if school, err = g.identityEducationBackend.UpdateEducationSchool(r.Context(), schoolID, *school); err != nil {
logger.Debug().Err(err).Interface("school", school).Msg("could not update school: backend error")
errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error())
Expand Down
9 changes: 8 additions & 1 deletion services/graph/pkg/service/v0/educationuser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,11 +76,15 @@ func (g Graph) PostEducationUser(w http.ResponseWriter, r *http.Request) {
return
}

if _, ok := u.GetDisplayNameOk(); !ok {
name, ok := u.GetDisplayNameOk()
if !ok {
logger.Debug().Err(err).Interface("user", u).Msg("could not create education user: missing required Attribute: 'displayName'")
errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "missing required Attribute: 'displayName'")
return
}
if !nameAccepted(*name, w, r, logger) {
return
}

identities, ok := u.GetIdentitiesOk()
if !ok {
Expand Down Expand Up @@ -354,6 +358,9 @@ func (g Graph) PatchEducationUser(w http.ResponseWriter, r *http.Request) {
}

if name, ok := changes.GetDisplayNameOk(); ok {
if !nameAccepted(*name, w, r, logger) {
return
}
features = append(features, events.UserFeature{Name: "displayname", Value: *name})
}

Expand Down
9 changes: 8 additions & 1 deletion services/graph/pkg/service/v0/groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,15 @@ func (g Graph) PostGroup(w http.ResponseWriter, r *http.Request) {
return
}

if _, ok := grp.GetDisplayNameOk(); !ok {
groupName, ok := grp.GetDisplayNameOk()
if !ok {
logger.Debug().Err(err).Interface("group", grp).Msg("could not create group: missing required attribute")
errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "Missing Required Attribute")
return
}
if !nameAccepted(*groupName, w, r, logger) {
return
}

// Disallow user-supplied IDs. It's supposed to be readonly. We're either
// generating them in the backend ourselves or rely on the Backend's
Expand Down Expand Up @@ -128,6 +132,9 @@ func (g Graph) PatchGroup(w http.ResponseWriter, r *http.Request) {

if changes.HasDisplayName() {
groupName := changes.GetDisplayName()
if !nameAccepted(groupName, w, r, logger) {
return
}
err = g.identityBackend.UpdateGroupName(r.Context(), groupID, groupName)
}

Expand Down
19 changes: 19 additions & 0 deletions services/graph/pkg/service/v0/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"net/http"
"os"
"strconv"
"strings"
"time"

"github.com/go-chi/chi/v5"
Expand All @@ -16,13 +17,15 @@ import (
"github.com/jellydator/ttlcache/v3"
libregraph "github.com/owncloud/libre-graph-api-go"
ocisldap "github.com/owncloud/ocis/v2/ocis-pkg/ldap"
"github.com/owncloud/ocis/v2/ocis-pkg/log"
"github.com/owncloud/ocis/v2/ocis-pkg/roles"
"github.com/owncloud/ocis/v2/ocis-pkg/service/grpc"
"github.com/owncloud/ocis/v2/ocis-pkg/store"
settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0"
"github.com/owncloud/ocis/v2/services/graph/pkg/identity"
"github.com/owncloud/ocis/v2/services/graph/pkg/identity/ldap"
graphm "github.com/owncloud/ocis/v2/services/graph/pkg/middleware"
"github.com/owncloud/ocis/v2/services/graph/pkg/service/v0/errorcode"
microstore "go-micro.dev/v4/store"
)

Expand Down Expand Up @@ -424,3 +427,19 @@ func parsePurgeHeader(h http.Header) bool {
}
return false
}

// ldap is using `=` signs as delimiter for key-values when doing requests
// underlying go library is having problems when trying to parse names (keys) that contain `=`
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if you'd mention the upstream issue here. I just found: go-ldap/ldap#416 (even though it's talking about base64 encoded values, the issue is really about the = sign).

// this causes our internal idm to panic. As a quick "fix" we forbid `=` signs in group/class/school ... names.
// this should be revisited when upstream library is fixed or the need for `=` in names arises
//
// nameAccepted is a convenience function setting status code to `400` and logging it
func nameAccepted(name string, w http.ResponseWriter, r *http.Request, logger log.Logger) bool {
if !strings.Contains(name, "=") {
return true
}

logger.Debug().Str("name", name).Msg("name contained '='")
errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "name must not contain '='")
return false
}
9 changes: 8 additions & 1 deletion services/graph/pkg/service/v0/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,11 +271,15 @@ func (g Graph) PostUser(w http.ResponseWriter, r *http.Request) {
return
}

if _, ok := u.GetDisplayNameOk(); !ok {
name, ok := u.GetDisplayNameOk()
if !ok {
logger.Debug().Err(err).Interface("user", u).Msg("could not create user: missing required Attribute: 'displayName'")
errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "missing required Attribute: 'displayName'")
return
}
if !nameAccepted(*name, w, r, logger) {
return
}
if accountName, ok := u.GetOnPremisesSamAccountNameOk(); ok {
if !g.isValidUsername(*accountName) {
logger.Debug().Str("username", *accountName).Msg("could not create user: username must be at least the local part of an email")
Expand Down Expand Up @@ -659,6 +663,9 @@ func (g Graph) PatchUser(w http.ResponseWriter, r *http.Request) {
}

if name, ok := changes.GetDisplayNameOk(); ok {
if !nameAccepted(*name, w, r, logger) {
return
}
features = append(features, events.UserFeature{Name: "displayname", Value: *name})
}

Expand Down
1 change: 1 addition & 0 deletions tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ The expected failures in this file are from features in the owncloud/ocis repo.
- [apiGraph/assignRole.feature:33](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/assignRole.feature#L33)

### [Group having percentage (%) can be created but cannot be GET](https://github.com/owncloud/ocis/issues/5083)
- [apiGraph/deleteGroup.feature:34](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/deleteGroup.feature#L34)
- [apiGraph/deleteGroup.feature:49](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/deleteGroup.feature#L49)
- [apiGraph/deleteGroup.feature:50](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/deleteGroup.feature#L50)
- [apiGraph/deleteGroup.feature:51](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/deleteGroup.feature#L51)
Expand Down
12 changes: 6 additions & 6 deletions tests/acceptance/features/apiGraph/addUserToGroup.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Feature: add users to group
| Finance (NP) | Space and brackets |
| Admin&Finance | Ampersand |
| maint+eng | Plus sign |
| $x<=>[y*z^2]! | Maths symbols |
| $x<>[y*z^2]! | Maths symbols |
| 😁 😂 | emoji |
| admin:Pokhara@Nepal | Colon and @ |
When the administrator adds the following users to the following groups using the Graph API
Expand All @@ -49,7 +49,7 @@ Feature: add users to group
| Alice | Finance (NP) |
| Alice | Admin&Finance |
| Alice | maint+eng |
| Alice | $x<=>[y*z^2]! |
| Alice | $x<>[y*z^2]! |
| Alice | 😁 😂 |
| Alice | admin:Pokhara@Nepal |
Then the HTTP status code of responses on all endpoints should be "204"
Expand All @@ -62,7 +62,7 @@ Feature: add users to group
| Alice | Finance (NP) |
| Alice | Admin&Finance |
| Alice | maint+eng |
| Alice | $x<=>[y*z^2]! |
| Alice | $x<>[y*z^2]! |
| Alice | 😁 😂 |
| Alice | admin:Pokhara@Nepal |

Expand All @@ -72,7 +72,7 @@ Feature: add users to group
| groupname | comment |
| maintenance#123 | Hash sign |
| 50%pass | Percent sign (special escaping happens) |
| 50%25=0 | %25 literal looks like an escaped "%" |
| 50%250 | %25 literal looks like an escaped "%" |
| 50%2Eagle | %2E literal looks like an escaped "." |
| 50%2Fix | %2F literal looks like an escaped slash |
| Mgmt\Middle | Backslash |
Expand All @@ -81,7 +81,7 @@ Feature: add users to group
| username | groupname |
| Alice | maintenance#123 |
| Alice | 50%pass |
| Alice | 50%25=0 |
| Alice | 50%250 |
| Alice | 50%2Eagle |
| Alice | 50%2Fix |
| Alice | Mgmt\Middle |
Expand All @@ -91,7 +91,7 @@ Feature: add users to group
| username | groupname |
| Alice | maintenance#123 |
| Alice | 50%pass |
| Alice | 50%25=0 |
| Alice | 50%250 |
| Alice | 50%2Eagle |
| Alice | 50%2Fix |
| Alice | Mgmt\Middle |
Expand Down
2 changes: 1 addition & 1 deletion tests/acceptance/features/apiGraph/createGroup.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ Feature: create group
| simplegroup |
| España§àôœ€ |
| नेपाली |
| $x<=>[y*z^2+1]! |
| $x<>[y*z^2+1]! |
| 😅 😆 |
| comma,grp1 |
| Finance (NP) |
Expand Down
4 changes: 2 additions & 2 deletions tests/acceptance/features/apiGraph/deleteGroup.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ Feature: delete groups
| Admin&Finance | Ampersand |
| admin:Pokhara@Nepal | Colon and @ |
| maint+eng | Plus sign |
| $x<=>[y*z^2]! | Maths symbols |
| $x<>[y*z^2]! | Maths symbols |
| Mgmt\Middle | Backslash |
| 😁 😂 | emoji |
| maintenance#123 | Hash sign |
| 50%25=0 | %25 literal looks like an escaped "%" |
| 50%250 | %25 literal looks like an escaped "%" |
| staff?group | Question mark |
| Mgmt/Sydney | Slash (special escaping happens) |
| Mgmt//NSW/Sydney | Multiple slash |
Expand Down
4 changes: 2 additions & 2 deletions tests/acceptance/features/apiGraph/getGroup.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,8 @@ Feature: get groups and their members
| group |
| España§àôœ€ |
| नेपाली |
| $x<=>[y*z^2+1]! |
| եòɴԪ˯ΗՐΛɔπ |
| $x<>[y*z^2+1]! |
| եòɴԪ˯ΗՐΛɔπ |


Scenario: admin user tries to get group information of non-existing group
Expand Down
16 changes: 8 additions & 8 deletions tests/acceptance/features/apiGraph/removeUserFromGroup.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ Feature: remove a user from a group
| Admin&Finance | Ampersand |
| admin:Pokhara@Nepal | Colon and @ |
| maint+eng | Plus sign |
| $x<=>[y*z^2]! | Maths symbols |
| $x<>[y*z^2]! | Maths symbols |
| Mgmt\Middle | Backslash |
| 😁 😂 | emoji |
And the following users have been added to the following groups
Expand All @@ -56,7 +56,7 @@ Feature: remove a user from a group
| Alice | Admin&Finance |
| Alice | admin:Pokhara@Nepal |
| Alice | maint+eng |
| Alice | $x<=>[y*z^2]! |
| Alice | $x<>[y*z^2]! |
| Alice | Mgmt\Middle |
| Alice | 😁 😂 |
When the administrator removes the following users from the following groups using the Graph API
Expand All @@ -69,7 +69,7 @@ Feature: remove a user from a group
| Alice | Admin&Finance |
| Alice | admin:Pokhara@Nepal |
| Alice | maint+eng |
| Alice | $x<=>[y*z^2]! |
| Alice | $x<>[y*z^2]! |
| Alice | Mgmt\Middle |
| Alice | 😁 😂 |
Then the HTTP status code of responses on all endpoints should be "204"
Expand All @@ -83,7 +83,7 @@ Feature: remove a user from a group
| Alice | Admin&Finance |
| Alice | admin:Pokhara@Nepal |
| Alice | maint+eng |
| Alice | $x<=>[y*z^2]! |
| Alice | $x<>[y*z^2]! |
| Alice | Mgmt\Middle |
| Alice | 😁 😂 |

Expand All @@ -92,23 +92,23 @@ Feature: remove a user from a group
Given these groups have been created:
| groupname | comment |
| maintenance#123 | Hash sign |
| 50%25=0 | %25 literal looks like an escaped "%" |
| 50%250 | %25 literal looks like an escaped "%" |
| staff?group | Question mark |
| 50%pass | Percent sign (special escaping happens) |
| 50%2Eagle | %2E literal looks like an escaped "." |
| 50%2Fix | %2F literal looks like an escaped slash |
And the following users have been added to the following groups
| username | groupname |
| Alice | maintenance#123 |
| Alice | 50%25=0 |
| Alice | 50%250 |
| Alice | staff?group |
| Alice | 50%pass |
| Alice | 50%2Eagle |
| Alice | 50%2Fix |
When the administrator removes the following users from the following groups using the Graph API
| username | groupname |
| Alice | maintenance#123 |
| Alice | 50%25=0 |
| Alice | 50%250 |
| Alice | staff?group |
| Alice | 50%pass |
| Alice | 50%2Eagle |
Expand All @@ -117,7 +117,7 @@ Feature: remove a user from a group
And the following users should not belong to the following groups
| username | groupname |
| Alice | maintenance#123 |
| Alice | 50%25=0 |
| Alice | 50%250 |
| Alice | staff?group |
| Alice | 50%pass |
| Alice | 50%2Eagle |
Expand Down