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

correct msgraph search query - fixes #189 #196

Closed
wants to merge 1 commit into from

Conversation

sspans-sbp
Copy link

@sspans-sbp sspans-sbp commented Apr 24, 2024

Overview

The refactored validation for static service principals was checking the application_object_id by searching for an application_id. This updates the search to search for application_object_id instead.

Design of Change

Only the search filter was updated.
The lookup could possibly be improved by fetching the application object directly rather than searching. But in this case I've opted for the smallest change to resolve the regression.

Related Issues/Pull Requests

[x] Issue #189

Contributor Checklist

[ ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
My Docs PR Link
Example
[ ] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
[x] Backwards compatible

Copy link

hashicorp-cla-app bot commented Apr 24, 2024

CLA assistant check
All committers have signed the CLA.

@sspans-sbp
Copy link
Author

@vinay-gopalan any chance you could review / merge this one?

@gsantos-hc
Copy link
Contributor

@sspans-sbp I found the same bug while working on something else. Agree that it's only in Vault 1.16+.

There's also a GetApplication interface at the top of api/applications.go. The interface's variable name is still clientId, which will be confusing in IDEs. Would you also update that to objectId?

@sspans-sbp
Copy link
Author

A better solution was merged in #200

@sspans-sbp sspans-sbp closed this May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants