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

add: User identity negotiation #402

Merged
merged 5 commits into from
Apr 13, 2024

Conversation

nicholasrussell
Copy link
Contributor

Hello,

Looking for your feedback in adding some of the user identity negotiation options.

In order to keep this minimal, I added the entrypoint into retrieving this data into the existing AccessControl trait. The downside being that it breaks the interface.

Thanks

@Enet4 Enet4 added A-lib Area: library A-tool Area: tooling C-ul Crate: dicom-ul C-storescu Crate: dicom-storescu labels Aug 22, 2023
@Enet4
Copy link
Owner

Enet4 commented Aug 22, 2023

Hello! Thank you for the pull request! It is great to see an initiative towards the integration on authorization mechanisms in DICOM-rs.

@Enet4 Enet4 added the breaking change Hint that this may require a major version bump on release label Aug 22, 2023
@Enet4 Enet4 self-requested a review November 28, 2023 11:21
@Enet4 Enet4 added this to the DICOM-rs 0.7 milestone Mar 13, 2024
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Hello @nicholasrussel! I am adding this contribution as a candidate for DICOM-rs 0.7.0, but we may need a reiteration of the code first. Please see the suggestions inline, and let me know your availability to work on this.

If you also have any ideas for testing the changes in a real scenario, I would be grateful to know. The PACS archives I usually work with do not feature user identity negotiation.

ul/src/association/client.rs Show resolved Hide resolved
ul/tests/pdu.rs Outdated Show resolved Hide resolved
ul/src/pdu/mod.rs Outdated Show resolved Hide resolved
ul/src/pdu/mod.rs Outdated Show resolved Hide resolved
ul/src/pdu/mod.rs Show resolved Hide resolved
ul/src/association/client.rs Show resolved Hide resolved
ul/src/pdu/mod.rs Outdated Show resolved Hide resolved
ul/src/pdu/mod.rs Outdated Show resolved Hide resolved
ul/src/pdu/reader.rs Outdated Show resolved Hide resolved
nicholasrussell and others added 2 commits March 24, 2024 09:59
- Remove `Unsupported`
- rename SAMLAssertion to `SamlAssertion`
- rename JWT to `Jwt`
- make UserIdentityType non-exhaustive
@Enet4 Enet4 force-pushed the user-identity-negotiation branch from 1591a42 to 8057259 Compare March 24, 2024 09:59
Enet4 added 3 commits March 24, 2024 10:01
- constrain to at most one user identity negotiation variable set
- declare conflict with other user identity forms
- + format code
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Despite there being gaps in the ecosystem for full authentication support, there is interest in expanding the ul crate with the knowledge of user identity parts, plus the extra of making these available in the dicom-storescu tool. This appears to be in order, so I will merge it onto the 0.7 milestone. Much appreciated! 👍

@Enet4 Enet4 merged commit 96f12d7 into Enet4:master Apr 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lib Area: library A-tool Area: tooling breaking change Hint that this may require a major version bump on release C-storescu Crate: dicom-storescu C-ul Crate: dicom-ul
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants