Skip to content

feat(graph): allow creating guest users via the provisioning API#12471

Open
dj4oC wants to merge 1 commit into
owncloud:masterfrom
dj4oC:feat/graph/users-allow-guest-usertype
Open

feat(graph): allow creating guest users via the provisioning API#12471
dj4oC wants to merge 1 commit into
owncloud:masterfrom
dj4oC:feat/graph/users-allow-guest-usertype

Conversation

@dj4oC

@dj4oC dj4oC commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Problem

POST /graph/v1.0/users rejected any userType in the request body as a read-only
attribute and always created Member users. There was no way to create a Guest
user through the provisioning API, even though the system already recognizes Guest
as a valid user type (isValidUserType accepts only Member and Guest).

Solution

The create handler now honors an explicit, valid userType and only defaults to
Member when it is omitted:

switch {
case !u.HasUserType():
    u.SetUserType("Member")          // unchanged default
case !isValidUserType(u.GetUserType()):
    // 400 invalid userType
}

Backwards compatibility

  • Omitting userType still yields Member, so existing API consumers are
    completely unaffected.
  • An unknown/empty userType is rejected with 400 (it was already a 400 before).
  • Creating users remains gated by the admin requirement on the route
    (requireAdmin).
  • The update path (PatchUser) is unchangeduserType is still read-only on
    update, so an existing user's type cannot be flipped.

Testing

go test ./services/graph/pkg/service/v0/... passes. The existing "userType is
read-only" create spec was repointed to assert an invalid type is rejected, and a
new spec asserts an explicit Guest is honored.

Risk

Low. The default path (no userTypeMember) is preserved; the only behavioral
change is that explicitly providing a valid userType now succeeds instead of
returning 400.

🤖 Generated with Claude Code

@kw-security

kw-security commented Jun 26, 2026

Copy link
Copy Markdown

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues
Code Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

POST /graph/v1.0/users now honors an explicit userType (e.g. "Guest") on
creation instead of always forcing "Member" and rejecting any supplied type.
Omitting userType still defaults to "Member" (existing consumers unaffected);
an unknown userType is rejected with 400. Creating users stays gated by the
admin requirement on the route.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: David Walter <david.walter@kiteworks.com>
@dj4oC dj4oC force-pushed the feat/graph/users-allow-guest-usertype branch from f9efaf0 to 3f456ff Compare June 26, 2026 08:27

@DeepDiver1975 DeepDiver1975 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.

Thanks for this, David. Reviewed as a maintainer. The core change is clean and I like that it mirrors the existing PostEducationUser pattern (validate explicit userType, default to Member) — good consistency. Route auth, input validation, and the LDAP round-trip of userType all check out. One blocking concern though, around the role a Guest actually ends up with.

Blocking: a Guest created via this path gets the full User role, not the lightweight guest role.

PostUser unconditionally assigns BundleUUIDRoleUser (the full "User" role) to every newly created user when AssignDefaultUserRole is enabled, regardless of userType:

// services/graph/pkg/service/v0/users.go:518-529
if g.roleService \!= nil && g.config.API.AssignDefaultUserRole {
    // All users get the user role by default currently. ...
    if _, err = g.roleService.AssignRoleToUser(r.Context(), &settingssvc.AssignRoleToUserRequest{
        AccountUuid: *u.Id,
        RoleId:      ocissettingssvc.BundleUUIDRoleUser,   // <-- full User role, even for Guest
    }); ...
}

Before this PR that comment held true — every created user was a Member, so the full User role was correct. Now that we can create a Guest, this is a problem:

  • The intended model is that guests run with the lightweight role (BundleUUIDRoleUserLight, the role formerly named "Guest").
  • The proxy's defaultRoleAssigner is the component that assigns that light role to USER_TYPE_GUEST users — but only when the user has no role yet (if len(roleIDs) == 0), see services/proxy/pkg/userroles/defaultrole.go:42-62.
  • Because PostUser has already pre-assigned the full User role, roleIDs is non-empty at login, so the guest→light-role branch never fires. The "Guest" silently ends up with full Member capabilities.

Net effect: an admin creates what they believe is a restricted guest, but it operates with the full User role — the exact privilege boundary userType: Guest is meant to enforce. Since this PR is what first enables guest creation through this endpoint, the gap is introduced here.

Suggested fix: derive the default role from userType in PostUser — assign BundleUUIDRoleUserLight when the userType is Guest, otherwise BundleUUIDRoleUser — so the graph create path agrees with the proxy's guest-role intent. A test asserting a created Guest receives the light role (and a Member the full role) would lock this in.

Minor (non-blocking): the mixed-case input (e.g. gUeSt) is stored verbatim in LDAP and echoed back in responses. Matching is case-insensitive everywhere, so it's not a correctness/security issue, but normalizing to canonical Member/Guest before persisting (e.g. u.SetUserType to the canonical form) would keep stored data and API responses tidy.

Positives: changelog fragment present and well-formed; validation correctly rejects unknown/Federated types with 400; the new "honors explicit Guest" test is a good addition; backwards compatibility (omitted userTypeMember) is preserved.

Happy to approve once the role-assignment-by-userType is addressed. Not merging in the meantime.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants