Skip to content

Conversation

rmarinn
Copy link
Contributor

@rmarinn rmarinn commented Apr 5, 2025

Prepare


Description

This PR fixes the issue where Role entities are not getting created in the unsigned interface.

Target issue

closes #11160

Implementation Details

  • Fixed the issue in the unsigned interface where Role entities are not getting created.
New Implementations
  • A new bootstrap property called CEDARLING_UNSIGNED_ROLE_ID_SRC was added so that Cedarling knows what to use when creating a Role entity in the unsigned interface.
  • A new rust example has been added which uses the unsigned interface: authorize_unsigned.rs
Breaking changes
  • EntityData.payload has been renamed to EntityData.attributes.

Test and Document the changes

  • Static code analysis has been run locally and issues have been fixed
  • Relevant unit and integration tests have been added/updated
  • Relevant documentation has been updated if any (i.e. user guides, installation and configuration guides, technical design docs etc)

Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with docs: to indicate documentation changes or if the below checklist is not selected.

  • I confirm that there is no impact on the docs due to the code changes in this PR.

rmarinn added 5 commits April 5, 2025 15:43
**breaking changes**:
- renamed: EntityData.payload to EntityData.attributes

Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
@rmarinn rmarinn requested a review from Copilot April 5, 2025 08:09
@rmarinn rmarinn self-assigned this Apr 5, 2025
@rmarinn rmarinn linked an issue Apr 5, 2025 that may be closed by this pull request
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

@mo-auto mo-auto added comp-jans-cedarling Touching folder /jans-cedarling kind-bug Issue or PR is a bug in existing functionality labels Apr 5, 2025
rmarinn added 2 commits April 5, 2025 16:11
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
@rmarinn rmarinn changed the title fix!: role entity not being created in the unsigned interface fix(jans-cedarling)!: role entity not being created in the unsigned interface Apr 5, 2025
@rmarinn rmarinn marked this pull request as ready for review April 5, 2025 08:30
@mo-auto mo-auto added area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs labels Apr 5, 2025
@rmarinn rmarinn enabled auto-merge (squash) April 5, 2025 08:33
Copy link
Contributor

@olehbozhok olehbozhok left a comment

Choose a reason for hiding this comment

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

How about adding unit tests in cedarling/src/tests/authorize_unsigned.rs to verify that roles work?

attrs.into_iter().fold(HashMap::new(), |mut acc, attrs| {
acc.extend(attrs);
acc
})
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can simplify code to

Suggested change
} else {
HashMap::from_iter(attrs.into_iter().flatten())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in b5a8d69

rmarinn added 3 commits April 11, 2025 16:45
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
@rmarinn
Copy link
Contributor Author

rmarinn commented Apr 11, 2025

How about adding unit tests in cedarling/src/tests/authorize_unsigned.rs to verify that roles work?

There are already tests in jans-cedarling/cedarling/src/entity_builder/build_principal_entity/unsigned.rs that verify role creation.

The authorize_unsigned tests should focus on policy evaluation, like the other tests in that module. Including role creation tests there would blur the separation of concerns and overload the unit tests.

Signed-off-by: rmarinn <34529290+rmarinn@users.noreply.github.com>
@rmarinn rmarinn requested a review from olehbozhok April 11, 2025 09:08
Copy link
Contributor

@SafinWasi SafinWasi left a comment

Choose a reason for hiding this comment

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

Looks good to me. Role creation and RBAC policy works on python example.

@rmarinn rmarinn merged commit 5333c5e into main Apr 12, 2025
1 check passed
@rmarinn rmarinn deleted the jans-cedarling-11160 branch April 12, 2025 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-documentation Documentation needs to change as part of issue or PR comp-docs Touching folder /docs comp-jans-cedarling Touching folder /jans-cedarling kind-bug Issue or PR is a bug in existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix(jans-cedarling): authorize_unsigned not building Role entities
4 participants