Authorization group lookup GET

kokkerhout commented 3 years ago

Version report

Jenkins and plugins versions report:

Jenkins: 2.287
OS: Linux - 5.4.0-1039-aws
---
blueocean-pipeline-api-impl:1.24.6
pipeline-github-lib:1.0
favorite:2.3.3
workflow-api:2.42
pipeline-build-step:2.13
blueocean-autofavorite:1.2.4
blueocean-rest-impl:1.24.6
ssh-credentials:1.18.2
blueocean-personalization:1.24.6
git-client:3.7.1
structs:1.22
wildfly-deployer:1.0.2
pipeline-stage-view:2.19
plugin-util-api:2.1.0
test-results-analyzer:0.3.5
workflow-multibranch:2.23
run-condition:1.5
snakeyaml-api:1.27.0
github-api:1.123
workflow-durable-task-step:2.38
blueocean-rest:1.24.6
ws-cleanup:0.39
credentials:2.3.17
blueocean-git-pipeline:1.24.6
credentials-binding:1.24
blueocean-core-js:1.24.6
popper-api:1.16.1-2
log-parser:2.1
htmlpublisher:1.25
script-security:1.76
junit-attachments:1.6
font-awesome-api:5.15.2-2
ace-editor:1.1
workflow-basic-steps:2.23
stashNotifier:1.20
git:4.7.1
branch-api:2.6.3
gradle:1.36
pipeline-model-definition:1.8.4
durable-task:1.35
workflow-job:2.40
scm-api:2.6.4
maven-plugin:3.10
handy-uri-templates-2-api:2.1.8-1.0
github:1.33.1
pipeline-npm:0.9.2
cloudbees-bitbucket-branch-source:2.9.7
blueocean-config:1.24.6
pipeline-model-extensions:1.8.4
antisamy-markup-formatter:2.1
matrix-auth:2.6.6
jquery3-api:3.6.0-1
greenballs:1.15.1
trilead-api:1.0.13
resource-disposer:0.15
junit:1.49
cloudbees-folder:6.15
jenkins-design-language:1.24.6
timestamper:1.12
ldap:2.5
pipeline-input-step:2.12
sauce-ondemand:1.194
workflow-step-api:2.23
workflow-scm-step:2.12
blueocean-display-url:2.4.1
aws-global-configuration:1.6
email-ext:2.82
bootstrap4-api:4.6.0-3
plain-credentials:1.7
pipeline-model-api:1.8.4
config-file-provider:3.7.0
git-server:1.9
role-strategy:3.1.1
JiraTestResultReporter:2.0.8
command-launcher:1.5
checks-api:1.7.0
blueocean:1.24.6
variant:1.4
display-url-api:2.3.4
authentication-tokens:1.4
blueocean-events:1.24.6
performance:3.19
blueocean-bitbucket-pipeline:1.24.6
blueocean-web:1.24.6
blueocean-i18n:1.24.6
build-timeout:1.20
gravatar:2.1
nodejs:1.4.0
momentjs:1.1.1
jquery:1.12.4-1
jackson2-api:2.12.2
pipeline-rest-api:2.19
blueocean-jwt:1.24.6
lockable-resources:2.10
pipeline-utility-steps:2.7.1
deployit-plugin:8.0.0
pipeline-graph-analysis:1.10
bitbucket:1.1.27
ssh-slaves:1.31.7
pam-auth:1.6
h2-api:1.4.199
blueocean-pipeline-editor:1.24.6
apache-httpcomponents-client-4-api:4.5.13-1.0
mailer:1.34
pipeline-maven:3.10.0
jsch:0.1.55.2
okhttp-api:3.14.9
aws-credentials:1.29
pipeline-milestone-step:1.3.2
jdk-tool:1.5
javadoc:1.6
sshd:3.0.3
bouncycastle-api:2.20
mercurial:2.14
aws-java-sdk:1.11.995
token-macro:2.15
sonar:2.13
pipeline-stage-tags-metadata:1.8.4
blueocean-dashboard:1.24.6
build-monitor-plugin:1.12+build.201809061734
workflow-support:3.8
handlebars:3.0.8
jjwt-api:0.11.2-9.c8b45b8bb173
blueocean-pipeline-scm-api:1.24.6
blueocean-commons:1.24.6
matrix-project:1.18
echarts-api:5.0.2-1
confluence-publisher:2.0.6
pipeline-stage-step:2.5
workflow-cps:2.90
pipeline-aws:1.43
azure-ad:155.v745ce80af7ea
ant:1.11
xvfb:1.1.3
sse-gateway:1.24
github-branch-source:2.10.2
ansicolor:0.7.5
workflow-aggregator:2.6
windows-slaves:1.7
workflow-cps-global-lib:2.18
pubsub-light:1.13
azure-commons:1.1.3
blueocean-github-pipeline:1.24.6

What Operating System are you using (both controller, and any agents involved in the problem)?

Linux Debian10

Reproduction steps

I use choose Azure Active Directory Matrix-based security under authorization
When I add a group it shows a list of available groups. So it works as expected.
But when I add a group it show an error in the log;

Error message: Resource '242f047a-d011-4cac-9a3e-6f54364ec7ed' does not exist or one of its queried reference-property objects are not present. GET https://graph.microsoft.com/v1.0/users/242f047a-d011-4cac-9a3e-6f54364ec7ed

You can see in the "GET" part the url shows 'users' while it's actually a group.

Shouldn't the GET statement be something like: "GET /groups/{id}/members" ??

Results

Expected result:

GET https://graph.microsoft.com/v1.0/groups/242f047a-d011-4cac-9a3e-6f54364ec7ed/members
I would also expect a group icon in the webinterface. Now it's just showing the name only

Actual result:

group-users (242f047a-d011-4cac-9a3e-6f54364ec7ed)

I am not sure if this is a bug and that there is something wrong with the GET statement.

timja commented 3 years ago

https://github.com/jenkinsci/azure-ad-plugin/issues/120#issuecomment-820613645

If that's in your server logs then that's a known issue where we don't know if it's a user or a group, so we have to lookup both, and it logs a 404 as severe. Either an SDK issue or some customising needs doing. unrelated to this issue

The logs bit can be ignored although I'll leave the issue open as there must be a way to fix it.

The group name bit I tried to fix but I hit this code: https://github.com/jenkinsci/matrix-auth-plugin/blob/7b30c73f7d72d0a94827ac618e61ca7ce4740baf/src/main/java/org/jenkinsci/plugins/matrixauth/AuthorizationContainerDescriptor.java#L140-L145

Which just looked up the user first and I couldn't see a way to make it lookup the group, as I'm not allowed to return null from the loadUserByUsername method.

@daniel-beck am I misunderstanding it? how can I have the group validated in the UI for a matrix authorisation?

daniel-beck commented 3 years ago

@timja Don't know, this seems Azure API internal? If you have a separate API to check whether a user by a given name exists, do that and throw if it doesn't, bypassing the API that causes log messages.

timja commented 3 years ago

@daniel-beck this issue has 2 parts to it,

user exists - this is handled fine, there's just some log spam
group validation - I wasn't able to figure out how to implement this because of the doCheckName_ checking users first and I couldn't see a way to make it to the code that checks groups

daniel-beck commented 3 years ago

@timja Configuration does not explicitly distinguish between object types. It's an improvement I've wanted to do for a while. I recommend you change your plugin to gracefully handle extra requests about users that don't exist. As this is form validation, it'll happen anyway and ideally doesn't result in log spam.

daniel-beck commented 3 years ago

The "older" screenshot looks like it just shows a many years old outdated version of matrix-auth.

timja commented 3 years ago

@timja Configuration does not explicitly distinguish between object types. It's an improvement I've wanted to do for a while. I recommend you change your plugin to gracefully handle extra requests about users that don't exist. As this is form validation, it'll happen anyway and ideally doesn't result in log spam.

The plugin handles it gracefully, but the SDK is spamming the logs, there's probably a way to configure it.

What screenshot are you referring to? This plugin used to use old matrix auth UI, I synced it with the upstream plugin in https://github.com/jenkinsci/azure-ad-plugin/pull/119

daniel-beck commented 3 years ago

The plugin handles it gracefully, but the SDK is spamming the logs, there's probably a way to configure it.

Right, this is similar to what I tried to say in

If you have a separate API to check whether a user by a given name exists, do that and throw if it doesn't, bypassing the API that causes log messages.

What screenshot are you referring to? This plugin used to use old matrix auth UI, I synced it with the upstream plugin in #119

Thanks, did not realize this is just copied in, I saw the dependency and thought it was reused.

timja commented 3 years ago

Thanks, did not realize this is just copied in, I saw the dependency and thought it was reused.

Code is re-used where possible, the UI is a copy though

If you have a separate API to check whether a user by a given name exists, do that and throw if it doesn't, bypassing the API that causes log messages.

I couldn't find one

c4m4 commented 3 years ago

How specify the group using configuration as code plugin? I don't see any documentation and example about this :(

timja commented 3 years ago

How specify the group using configuration as code plugin? I don't see any documentation and example about this :(

configure it in the UI and then export it.

here's an example: Permission:object-id (display name)

https://github.com/jenkinsci/azure-ad-plugin/blob/master/src/test/resources/com/microsoft/jenkins/azuread/integrations/casc/configuration-as-code.yml

c4m4 commented 3 years ago

@timja clicking on jenkins ui, the result is permission:(display name) object-id, why?

timja commented 3 years ago

Historical I guess

c4m4 commented 3 years ago

@timja but what is the correct syntax, the one of the plugin example or the one exported by jenkins?

timja commented 3 years ago

exported will be correct

strobeti commented 3 years ago

Hi @timja, have you found a way to disable SDK logging or do you have another suggestion to prevent log spam?

timja commented 3 years ago

I'm working on a fix but it requires a change in matrix-auth first so will take some time.

This contains some pre-req work: https://github.com/jenkinsci/azure-ad-plugin/pull/136

I'm updating the PR description as I go, but it's getting closer

timja commented 3 years ago

The group icon bit is fixed in https://github.com/jenkinsci/azure-ad-plugin/pull/141

Log spam will be fixed later

kokkerhout commented 3 years ago

Right now I am forced to add the uuid of the group after the name of the group.

BGP-Jenkins-Admin (153ee3ff-699b-4e23-aeaf-a0644a179a50)

timja commented 3 years ago

Right now I am forced to add the uuid of the group after the name of the group.

BGP-Jenkins-Admin (153ee3ff-699b-4e23-aeaf-a0644a179a50)

Group display names aren't unique in AzureAD so you must add the ID anyway

kokkerhout commented 3 years ago

Check. thx

jenkinsci / azure-ad-plugin