Skip to content

Support "Microsoft Entra ID"-plugin by displaying readable group names.#172

Closed
cvberg wants to merge 1 commit intojenkinsci:mainfrom
cvberg:cvberg/fix-azure-group-display
Closed

Support "Microsoft Entra ID"-plugin by displaying readable group names.#172
cvberg wants to merge 1 commit intojenkinsci:mainfrom
cvberg:cvberg/fix-azure-group-display

Conversation

@cvberg
Copy link

@cvberg cvberg commented Jun 15, 2025

When using this plugin in combination with the "Microsoft Entra ID" plugin, users specified using an OID
will be shown with a readable name. However, groups do not get the same treatment and will always show an OID.
This PR will solve that issue.
It's a literal copy of a change made to the "role-strategy-plugin": https://github.com/jenkinsci/role-strategy-plugin/pull/355/files
Jenkins issue: https://issues.jenkins.io/browse/JENKINS-75726

Using OIDs is unfortunately the only way to make the Microsoft Entra ID plugin and this plugin work together. This PR will make this limitation less problematic.
Also, only the matrix-auth-plugin supports inheritance of permissions in folders and jobs, which is essential for me.

Testing done

  • I've tested this with the Entra ID plugin installed and the "Azure AD" authentication enabled. Before groups entered with a OID, will now show their Display Name instead.
  • I've also tested it with Active Directory authentication enabled. For AD, OIDs are not supported, so you must specify the group using the Display Name and that name will be shown unchanged.
  • I've repeated the previous test with the Entra ID plugin removed.

These tests have been done manually because I don't know how to add automated tests without having access to a real Entra ID system. I tested this at my workplace, where we do have this infrastructure.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests that demonstrate the feature works or the issue is fixed

@cvberg cvberg requested a review from a team as a code owner June 15, 2025 05:31
@cvberg
Copy link
Author

cvberg commented Jun 15, 2025

Some comments on the usability of the Entra ID plugin regarding the usability of its own "azure-matrix" implementation.
Not only does it not support inheritance, it's not even properly implemented: jenkinsci/azure-ad-plugin#185
This is why my team decided to use Entra ID in combination with the matrix-auth-plugin and just live with OIDs. For this, the current PR would be very welcome. In fact, we're already using it with my patch.

@daniel-beck
Copy link
Member

I doubt display names are unique, so this change looks like it would create ambiguity. Would it not make sense to apply the same check as in

sr.loadUserByUsername2(userName);
User u = User.getById(userName, true);
if (userName.equals(u.getFullName())) {
// Sid and full name are identical, no need for tooltip
to groups, and implement the corresponding else block similarly, so that the UI allows access to OID via tooltip?

@daniel-beck
Copy link
Member

I don't know how to add automated tests without having access to a real Entra ID system.

Doesn't need full Entra ID, "just" a security realm whose groups have different #getName and #getDisplayName. JenkinsRule.DummySecurityRealm does not support such groups, so that would require a custom implementation in the test here.

@cvberg
Copy link
Author

cvberg commented Jun 15, 2025

Makes sense to suppress the tooltip if it doesn't add new information.
To reduce ambiguity, showing the UPN would even be better, I'll see what I can do.
May take some time, because I'm under pressure to get a working solution that makes Entra ID possible without having only those miserable OIDs. So what I have now is good enough, but should be improved before integration.

@daniel-beck
Copy link
Member

Jenkins issue: https://issues.jenkins.io/browse/JENKINS-75726

Is that the correct issue ID?

@daniel-beck
Copy link
Member

My attempt at fixing this + test coverage is in #172.

@daniel-beck
Copy link
Member

Closing in favor of #183.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants