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

OicUserProperty was discarded after saving the user config page #332

Merged
merged 2 commits into from
Jun 10, 2024

Conversation

jglick
Copy link
Member

@jglick jglick commented Jun 6, 2024

If you log in with OIDC with a groups field defined, then go to your user config page and create an API token, then use that API token to run the CLI command who-am-i, your groups are listed as expected. But if you then Save the same page and try the CLI command again, you are shown as having no groups. This is because saving the page wiped out the OicUserProperty. AFAICT this has been broken since #12. The corrected idiom can be seen in LastGrantedAuthoritiesProperty in core, a very similar property. Actually we could probably use that instead of OicUserProperty by merely calling SecurityListener.fireLoggedIn; I did not try that yet.

@jglick jglick requested a review from a team as a code owner June 6, 2024 19:35
@@ -1,7 +1,5 @@
OicLogoutAction.OicLogout = Oic Logout

OicUserProperty.OpenIdConnectUserProperty = OpenID Connect user property
Copy link
Member Author

Choose a reason for hiding this comment

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

never displayed

Comment on lines +1273 to +1274
assertEquals(
"User should still be in 2 groups", 2, user.getAuthorities().size());
Copy link
Member Author

Choose a reason for hiding this comment

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

Before fix, failed with

java.lang.AssertionError: User should still be in 2 groups expected:<2> but was:<0>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotEquals(Assert.java:835)
	at org.junit.Assert.assertEquals(Assert.java:647)
	at org.jenkinsci.plugins.oic.PluginTest.testOicUserPropertyDescriptor(PluginTest.java:1270)

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 75.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 73.15%. Comparing base (e327bf2) to head (db811f2).
Report is 25 commits behind head on master.

Files Patch % Lines
...ava/org/jenkinsci/plugins/oic/OicUserProperty.java 75.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #332      +/-   ##
============================================
- Coverage     73.35%   73.15%   -0.21%     
+ Complexity      211      209       -2     
============================================
  Files            10       10              
  Lines           882      879       -3     
  Branches        124      124              
============================================
- Hits            647      643       -4     
- Misses          172      173       +1     
  Partials         63       63              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@michael-doubez michael-doubez left a comment

Choose a reason for hiding this comment

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

LGTM

@michael-doubez michael-doubez merged commit cac1e2f into jenkinsci:master Jun 10, 2024
17 of 18 checks passed
@jglick jglick deleted the OicUserProperty branch June 10, 2024 12:18
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.

2 participants