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.

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

Use object ID consistently #276

Merged
merged 3 commits into from
Mar 14, 2023
Merged

Conversation

timja
Copy link
Member

@timja timja commented Jul 15, 2022

Relates to #190

Fixes #278

This seems to fix inconsistencies around auth and uses the approach recommended by Microsoft for identifiers:
https://docs.microsoft.com/en-us/azure/active-directory/develop/active-directory-optional-claims

i.e never use UPNs for IDs.

Unfortunately this means that users display names will not show up in the jcasc export.
Groups will continue to have their name (object id) format though

TODO:

  • Port tests from matrix auth
  • Check existing JCasC config works
  • Review docs
  • Check role strategy is okay

@timja timja added the bug label Jul 15, 2022
Comment on lines -800 to +801
+ "\nTenant ID: " + user.getTenantID()
+ "\nGroups: " + user.getGroupOIDs() + "\n";
+ "\nTenant ID: " + user.getTenantID();

Choose a reason for hiding this comment

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

Why do you delete the list of groups from the user description? I recall viewing it in order to figure out whether access would be granted to a user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll revert this, the whoAmI page also displayed it, so I removed as it seemed unneeded, but I see the user profile page doesn't display it.

@andrewlorien
Copy link

This does not solve #253 (and i cannot reproduce #190)

@timja
Copy link
Member Author

timja commented Jul 18, 2022

This does not solve #253 (and i cannot reproduce #190)

Using a guest user for me was how I reproduced #190.

@timja timja mentioned this pull request Aug 17, 2022
6 tasks
@meiswjn meiswjn mentioned this pull request Aug 17, 2022
5 tasks
@timja timja marked this pull request as ready for review March 14, 2023 22:41
@timja timja merged commit d05b013 into jenkinsci:master Mar 14, 2023
@timja timja deleted the fix-api-token-auth branch March 14, 2023 22:41
@meiswjn
Copy link
Contributor

meiswjn commented Mar 22, 2023

Thanks for the fix!

We have noticed two bugs, however, one of them breaking:

  • Favorites have been deleted.
  • In Auth Matrix, users that were added before the update can now be added again - but changing anything in their config has no effect. The old user must be configured.

EDIT: If I enter the UPN of the user as URL (instead of now, the GUID), I still find the favorites.

All user settings have been reset, including themes etc.
The release should likely be marked as "Breaking change"

@timja
Copy link
Member Author

timja commented Mar 22, 2023

I've updated the release notes

@meiswjn
Copy link
Contributor

meiswjn commented Mar 22, 2023

Most importantly, this may be problematic with API Tokens. We are investigating and if we find anything else of relevance, we will post it here :)

@ivy-cst
Copy link

ivy-cst commented Mar 24, 2023

Not so nice, that now all users are duplicated! For our use cases it was nicer to have the e-mail as user id instead of the object id.
I understand that it is technically better to use the object id instead of the UPN.
But in jenkins it ends up that the user sees UPN's and also they are in the urls.
Would it be an option to make this configurable?
For now I have downgraded the plugin.

@timja
Copy link
Member Author

timja commented Mar 24, 2023

Prettiness in URLs won't take precedence over bugs caused by it.

#392
#278
and some of #190

I don't think the complexity of making it configurable is worth it personally

@ivy-cst
Copy link

ivy-cst commented Mar 24, 2023

Would it be possible to auto migrate the users?
Or is there a script which can do that?

In my opinion this update should display a big hint in the pluginManager that it is not backward compatible.

I think everybody has troubles when updating.
All the user settings (e.g. API-Tokens, SSH-KEYS, Own Views, Favorites) are lost ...

@timja
Copy link
Member Author

timja commented Mar 24, 2023

sure warning coming in #396

I don't think it will be possible to auto migrate, a script should be possible though if you write one I'm happy to link in release notes

@andysworkshop
Copy link

This has broken usage of the Jenkins API where previously [email protected]:$TOKEN was working for basic auth. Now you have to use object ID in place of email address for auth. We're going to communicate this to our users and patch our UIs that prompt for 'email address' but would appreciate it if the UPN could again be used for API call auth. IMO GUIDs are an internal ugliness that users should not see.

@timja
Copy link
Member Author

timja commented Apr 26, 2023

it might be possible to have it as an option but there are issues with using it as the UPN, feel free to file an issue / PR for it.

@Louai-Abdelsalam
Copy link

Can the documentation on the plugin's page (specifically this line and this line) be updated to reflect display names no longer showing up? my plugin got updated by someone else and I didn't know it, so I spent quite a bit of time super confused as to what happened, and having had the mentioned parts of the documentation updated would've helped me for sure.

@timja
Copy link
Member Author

timja commented Aug 16, 2023

Updated in #456

Feel free to propose updates yourself too if that's not enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Several bugs with initial node setup
7 participants