search

hkamel / sonar-auth-aad

Azure Active Directory Authentication for SonarQube
Other
68 stars 35 forks source link

Implement Azure AD App Roles #149

Open cRaZyT1807 opened 1 year ago

cRaZyT1807 commented 1 year ago

Hi,

I've tried pretty much everything to get the plugin to work with Azure AD app roles but it doesn't seem to work in the current version. It would be nice if this worked in the plugin the same way as with the SAML login, Azure AD App Roles are also supported here.

SAML group attribute For mapping with the Azure AD app roles, an application role should be assigned to the user. Azure AD sends the role claim automatically with http://schemas.microsoft.com/ws/2008/06/identity/claims/role as a key.

Regards Thorsten

srvrguy commented 1 year ago

It seems interesting, but the setup and handling is rather different between SAML and OIDC.

This plugin works off the slightly easier mechanism of matching AAD group names with SQ group names, as documented in the group sync of the wiki here.

Perhaps in a future major release this could be added, but I'm not sure why you'd then pick this plugin over the SAML option if you're going to do all the setup needed for App Roles and that functionality is already supported there.

cRaZyT1807 commented 1 year ago

I found this plugin smarter than the SAML plugin.

As far as setting up the app roles is concerned, it is not a big effort, since in principle only 2 app roles (admin & user) are sufficient. These roles are included in the token. In principle, it should be enough for this plugin to process the "Admin" role and assign accounts / groups to the SQ Admin group if this has been set accordingly in Azure AD.

srvrguy commented 1 year ago

It's good to know that information can be sent across in the token, but that's a long way from being "not a big effort". For your use-case, two roles may be sufficient, but the whole point of "group sync" is to allow for more complex memberships. A solid implementation would need to support an arbitrary number of passed roles and handle mapping correctly for that. Code would also need to be added to support group mapping/sync via this alternate method and testing would have to be done to make sure it works reliably.

While this alternate method for group sync can be looked at and eventually added, it's not going to be something I can do immediately.