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

Jump to bottom

OpenID application with the Administrator role isn't authorized for all permissions anymore #17083

Closed
Piedone opened this issue Nov 27, 2024 · 2 comments · Fixed by #17087
Closed

OpenID application with the Administrator role isn't authorized for all permissions anymore #17083

Piedone opened this issue Nov 27, 2024 · 2 comments · Fixed by #17087
Labels
bug 🐛 OpenId Roles

Comments

@Piedone
Copy link
Member

Piedone commented Nov 27, 2024

Describe the bug

When an OpendID application is configured for the Administrator Client Credentials Role, it's not authorizing as if it had all the permissions and thus authorization fails.

I suspect this is coming from #16781 @MikeAlhayek.

Orchard Core version

v2.1.1 but latest main too.

To Reproduce

  1. Set up a site with the SaaS recipe and enable the Deployment feature.
  2. Import this recipe to set up an OpenID application.
  3. Run this app after changing the port in Program to your OC instance's. For your convenience, I'm including a Windows executable below, configured for the vanilla OC from main (just run Lombiq.OrchardCoreApiClient.Tester.exe from the CLI). This app accesses the OC tenant and content management APIs.
  4. Observe an exception that the tenant creation failed with unauthorized:
    {6470DA25-2D54-486B-BFFF-32FDA2BCBC87}

This is because this authorization fails, what you can also observe from the debugger:

OrchardCore/src/OrchardCore.Modules/OrchardCore.Tenants/Controllers/TenantApiController.cs

Line 92 in 2355c10

if (!await _authorizationService.AuthorizeAsync(User, Permissions.ManageTenants))

The executable:

net8.0.zip

If you instead use a role that explicitly has the Manage Tenants permission, then the authorization works.

Expected behavior

The API authorization, and thus tenant creation, continues to work with the Administrator role.

Logs and screenshots
@Piedone Piedone changed the title OpenID application with the Administrator isn't authorized for all permissions anymore OpenID application with the Administrator role isn't authorized for all permissions anymore Nov 27, 2024
@MikeAlhayek
Copy link
Member

@kevinchalet I'll try to look into this in the morning, but do we handle permissions check using OpenId implementation different that what we do in OC? With the latest update, we don't store permission claims when a user with Administrator role logs in. In an Authorization handler we added this check

OrchardCore/src/OrchardCore.Modules/OrchardCore.Settings/Services/SuperUserHandler.cs

Line 28 in 2355c10

if (user.HasClaim(StandardClaims.SiteOwner.Type, StandardClaims.SiteOwner.Value))

When an Admin user logs in we grant them this Owner permission

OrchardCore/src/OrchardCore.Modules/OrchardCore.Roles/Services/RoleClaimsProvider.cs

Line 40 in 2355c10

claims.AddClaim(StandardClaims.SiteOwner);

I don't think OpenId does anything different and handler permission just like we do in other places

@kevinchalet
Copy link
Member

@kevinchalet I'll try to look into this in the morning, but do we handle permissions check using OpenId implementation different that what we do in OC?

No.

The issue described here is likely caused by the fact we're not injecting that new special claim for the client credentials grant, which is a user-less flow but for which we nevertheless allow attaching "role claims" to the access token as a way to control permissions:

OrchardCore/src/OrchardCore.Modules/OrchardCore.OpenId/Controllers/AccessController.cs

Lines 454 to 471 in 2355c10

var roleService = HttpContext.RequestServices.GetService<IRoleService>();
foreach (var role in await _applicationManager.GetRolesAsync(application))
{
// Since the claims added in this block have a dynamic name, directly set the destinations
// here instead of relying on the GetDestination() helper that only works with static claims.
identity.AddClaim(new Claim(identity.RoleClaimType, role)
.SetDestinations(Destinations.AccessToken, Destinations.IdentityToken));
if (roleService != null)
{
foreach (var claim in await roleService.GetRoleClaimsAsync(role))
{
identity.AddClaim(claim.SetDestinations(Destinations.AccessToken, Destinations.IdentityToken));
}
}
}

Now... it's not clear to me why this "site owner" has a special treatment and isn't exposed as a regular role...

This was referenced Nov 27, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug 🐛 OpenId Roles
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix SiteOwner claim for OpenId OrchardCMS/OrchardCore
3 participants
@Piedone @kevinchalet @MikeAlhayek