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

Nicer JCasC syntax for defining permissions #145

Merged
merged 8 commits into from
Aug 22, 2023

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Jul 4, 2023

Alternative to #111 with (IMO) nicer structure.

Complements #144 (which is for Job DSL / Pipeline).

- group:
    name: foo
    permissions:
    - Overall/Administer
    - …

Feedback welcome.

@daniel-beck
Copy link
Member Author

@timja Just in case, any thoughts about this?

@timja
Copy link
Member

timja commented Jul 18, 2023

It looks good to me, just some tests to sort out by the looks of it? (a bunch of commented unit tests)

@daniel-beck daniel-beck marked this pull request as ready for review August 22, 2023 08:51
assertEquals(
toStringFromYamlFile(
this,
"/org/jenkinsci/plugins/matrixauth/integrations/casc/ExportTest/ExportTest-exportTestLegacy-global.yml"),
Copy link
Member

Choose a reason for hiding this comment

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

the classpath entry should be relative, I would have thought ExportTest-exportTestLegacy-global.yml would work but this stuff can be a real pain sometimes and very hard to debug

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 wanted these files in the folder corresponding to the class, not in the folder for the package. (Kinda redundant with the file name now though 🤔 )

Copy link
Member

Choose a reason for hiding this comment

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

ExportTest/exportTestLegacy-global.yml would work then I think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh. Never tried that. Javadoc indicates this could work.

@daniel-beck daniel-beck merged commit 47a3280 into jenkinsci:master Aug 22, 2023
17 checks passed
@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Aug 23, 2023

Hi, I saw the breaking-change warning in the 3.2 release notes.
Do you know if the breaking change also applies to JCasC settings for the Azure AD plugin, which depends on matrix-auth? It has used permissions until now:

jenkins:
  authorizationStrategy:
    azureAdMatrix:
      permissions:
      - "USER:Overall/Administer:My Account (356726a4-2127-4e27-a763-483511f9aee1)"

I think JCasC for node permissions might even break completely until azure-ad-plugin is updated, as AzureAdAuthorizationMatrixNodePropertyConfigurator uses .setter(MatrixAuthorizationStrategyConfigurator::setPermissions), which was renamed to setLegacyPermissions in this PR. So the azure-ad-plugin incompatibility could deserve a mention in the release notes.

OpenShift Login also depends on matrix-auth but I don't know what kind of JCasC structure it uses or what methods it calls.

@daniel-beck
Copy link
Member Author

daniel-beck commented Aug 23, 2023

FWIW I've attempted to maintain backward compatibility with older versions of the JCasC syntax (the YAML part, not the Job DSL Groovy part) and it should work with those older configs, but I still recommend caution when updating. The Job DSL and Pipeline syntaxes aren't as configurable and those are fully incompatible.

Do you know if the breaking change also applies to JCasC settings for the Azure AD plugin, which depends on matrix-auth? … the azure-ad-plugin incompatibility could deserve a mention in the release notes

With regards to azure-ad, it ignores restrictions declaring certain APIs in this plugin to be inaccessible to other plugins, which allows me to, e.g., freely rename methods, so any problems exclusive to azure-ad I do not care about at all. It's that plugin's maintainers' responsibility to maintain compatibility. FWIW Tim, an azure-ad maintainer, is aware of these PRs, he provided the inspiration and a first attempt in #111 and some feedback/assistance for what ultimately got integrated (scroll up a bit).

@KalleOlaviNiemitalo
Copy link

I see; then the referenced classes of matrix-auth will need to be essentially forked into azure-ad so that it doesn't rely on private APIs.

@PiersRBME
Copy link

@daniel-beck Do you happen to know if any other plugins, in particular oic-auth and saml, might be relying on private APIs in the same way?

(What's that "law" that anything will become depended upon by someone 😓.)

@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Aug 23, 2023

@PiersRBME, there is no "SuppressRestrictedWarnings" in the https://github.com/jenkinsci/oic-auth-plugin/ and https://github.com/jenkinsci/saml-plugin/ repositories, not even in history.

@daniel-beck
Copy link
Member Author

@PiersRBME They don't show up on https://plugins.jenkins.io/matrix-auth/dependencies/ as dependants, so do not use any APIs of this plugin, except of those defined in core.

Looks to me like y'all need staging environments 😉

@timja
Copy link
Member

timja commented Aug 23, 2023

(in progress for AzureAD)

@PiersRBME
Copy link

PiersRBME commented Aug 23, 2023

Yeah, have non-prod envs and was able to check and update the CASC format - it is clearer.

But, yeah backwards compat is hard. Someone somewhere will come to rely on everything, private or not. To an outsider a change in one plugin that breaks another is an irritation (at best) -- that the dev of one doesn't care about breaking the other at all is not much comfort, whatever the reason!

@timja
Copy link
Member

timja commented Aug 23, 2023

Not done yet but I've opened a draft PR: jenkinsci/azure-ad-plugin#460

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.

4 participants