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.

Sign up for GitHub

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

Jump to bottom

Add ClientCredentials Authentication with Certificate #1110

Merged
merged 3 commits into from
May 24, 2024
Merged

Add ClientCredentials Authentication with Certificate #1110

merged 3 commits into from
May 24, 2024

Conversation

pri-kise
Copy link
Contributor

@pri-kise pri-kise commented May 14, 2024 •
edited by github-actions bot

Summary

Add Authentication with Certificate+Password to RestClient and GraphClient.

Work Item(s)

Fixes #1093

Fixes AB#534910

@github-actions github-actions bot added AL: System Application From Fork Pull request is coming from a fork labels May 14, 2024
@JesperSchulz JesperSchulz added the Integration GitHub request for Integration area label May 16, 2024
@pri-kise pri-kise force-pushed the feature/1093-client-credentials-with-certificate branch from 626e5a2 to c9d55d2 Compare May 22, 2024 05:56
@github-actions github-actions bot added the Linked Issue is linked to a Azure Boards work item label May 22, 2024
@github-actions github-actions bot added this to the Version 25.0 milestone May 22, 2024
@pri-kise pri-kise changed the title [Draft] Add ClientCredentials Authentication with Certificate Add ClientCredentials Authentication with Certificate May 22, 2024
@pri-kise pri-kise marked this pull request as ready for review May 22, 2024 05:58
@pri-kise pri-kise requested a review from a team as a code owner May 22, 2024 05:58
PeterConijn
PeterConijn approved these changes May 22, 2024
View reviewed changes
Copy link
Contributor

@PeterConijn PeterConijn left a comment •
edited

Choose a reason for hiding this comment

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

Looks good! I was thinking about automated tests, but since this is all external connection, I'm not sure how you'd easily implement that (aside perhaps injection).

@pri-kise
Copy link
Contributor Author

@PeterConijn

Looks good! I was thinking about automated tests, but since this is all external connection, I'm not sure how you'd easily implement that (aside perhaps injection).

For the existing procedures there are already tests without authentication.

And I've added a similiar type of authentication recently for the SharePoint Module.
https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/SharePoint%20Authorization/src/SharePointClientCredentials.Codeunit.al
You could check that I didn't made any copy-paste issues and that I didn't forgot any not or something similar.

@JesperSchulz JesperSchulz requested a review from darjoo May 24, 2024 08:28
@JesperSchulz
Copy link
Contributor

@darjoo, we would need your sign-off on this one. This PR has security impact. Please sign off on attached slice when completed.

@JesperSchulz JesperSchulz enabled auto-merge (squash) May 24, 2024 08:54
@JesperSchulz JesperSchulz merged commit 562fc40 into microsoft:main May 24, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darjoo darjoo darjoo approved these changes

@JesperSchulz JesperSchulz JesperSchulz approved these changes

@PeterConijn PeterConijn PeterConijn approved these changes

Assignees
No one assigned
Labels
AL: System Application From Fork Pull request is coming from a fork Integration GitHub request for Integration area Linked Issue is linked to a Azure Boards work item
Projects
None yet
Milestone
Version 25.0
Development

Successfully merging this pull request may close these issues.

[BC Idea]: Client Credentials Authentication via Certificate+Password for Microsoft Graph Module + RestClient
4 participants
@pri-kise @JesperSchulz @darjoo @PeterConijn