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

refactor(jans-cedarling): enhance schema parser and entity creation implementation #10549

Open
wants to merge 64 commits into
base: main
Choose a base branch
from

Conversation

rmarinn
Copy link
Contributor

@rmarinn rmarinn commented Jan 4, 2025

Prepare


Description

This PR refactors the JSON schema struct and the Cedar entity-building logic to improve code readability and maintainability.

Target issue

target issue: #10513

closes #10513

Implementation Details

  1. Introduction of EntityBuilder Struct

  2. Refactoring CedarSchemaJson

    • The CedarSchemaJson struct has been redesigned for better usability.
    • Deserialization and JSON schema parsing logic is now self-contained, reducing complexity for other callers.
  3. Unit Tests

    • Added unit tests for the refactored components.
    • Future test cases will be easier to implement due to reduced coupling and improved struct/function design.
  4. Improved Automatically adding entities to the context

    • We don't need to do the "lower_snake_case convention" for entity names anymore to automatically add them to the context.
Entity Builder

The EntityBuilder struct centralizes entity creation logic and holds configuration on initialization.

Usage:

pub struct EntityBuilder {
    schema: CedarSchemaJson,
    entity_names: EntityNames,
    build_workload: bool,
    build_user: bool,
}

Function implementations on this struct will have the entity creation code. Other modules will only have to use these functions.

impl EntityBuilder {
    pub fn new(
        schema: CedarSchemaJson,
        entity_names: EntityNames,
        build_workload: bool,
        build_user: bool,
    ) -> Self {
        // ...
    }

    pub fn build_entities(
        &self,
        tokens: &DecodedTokens,
        resource: &ResourceData,
    ) -> Result<AuthorizeEntitiesData, BuildCedarlingEntityError> {
        // ...
    }
}

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.

- implement an entity builder can can make workload entities

Signed-off-by: rmarinn <[email protected]>
- implement EntityBuilder::build_entities which builds all the
  cedarling-specific entities

Signed-off-by: rmarinn <[email protected]>
- start using the new CedarJsonSchema
- start using EntityBuilder to build entities

Signed-off-by: rmarinn <[email protected]>
- make the default type "EntityOrCommon" for unknown variants instead of
  failing desrialization.

Signed-off-by: rmarinn <[email protected]>
- fix the bug where the access_token is being used to create all token
  entities

Signed-off-by: rmarinn <[email protected]>
- fix CommonType contexts not being handled properly

Signed-off-by: rmarinn <[email protected]>
- fix entity references within entities not being qualified; i.e. the
  namespace is not included in the reference... which causes problems
  down the line

Signed-off-by: rmarinn <[email protected]>
- refactor role entities creation to not fail if no role entities were
  created but just return an empty Vec

Signed-off-by: rmarinn <[email protected]>
- silently fail non-required attr creation errors since it was making an
  existing test fail: "check_mapping_tokens_data"

Signed-off-by: rmarinn <[email protected]>
rmarinn added 18 commits January 8, 2025 17:14
- reduce the amount of lookups done in merge_json_values to improve
  performance

Signed-off-by: rmarinn <[email protected]>
- when calling merge_json_values, use the original context as the first
  param

Signed-off-by: rmarinn <[email protected]>
- implement a struct to describe claim aliases for better readability

Signed-off-by: rmarinn <[email protected]>
- return an error instead of panicking in try_build_role_entities

Signed-off-by: rmarinn <[email protected]>
- return an error when given a wrong kind of token when building token
  entities

Signed-off-by: rmarinn <[email protected]>
- add test for creating id_token entity
- add test for creating userinfo_token entity

Signed-off-by: rmarinn <[email protected]>
nynymike
nynymike previously approved these changes Jan 8, 2025
impl EntityBuilder {
/// Tries to build role entities using each given token. Will return an empty Vec
/// if no entities were created.
pub fn try_build_role_entities(
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's a suggestion to clean this up

18326b7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks good! i didn't realize we can use #[serde(untagged)] like that. i merged your suggestion in 98f99b6. thanks

{
serde_json::from_value::<String>(value).map_err(|e| {
de::Error::custom(format!(
"error while desrializing JSON Value to a String: {e}"

Choose a reason for hiding this comment

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

"desrializing" should be corrected to "deserializing"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 821958b

Comment on lines 12 to 18
pub fn deserialize_to_string<'de, D>(value: Value) -> Result<String, D::Error>
where
D: serde::Deserializer<'de>,
{
serde_json::from_value::<String>(value).map_err(|e| {
de::Error::custom(format!(
"error while desrializing JSON Value to a String: {e}"

Choose a reason for hiding this comment

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

as your function explicitly depend on serde_json::Value, maybe you can simplify it directly to

pub fn deserialize_to_string(value: Value) -> Result<String, String> {
    serde_json::from_value::<String>(value).map_err(|e| format!("Error: {e}"))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i tried this and this requires me to map the error into a serde::de::Error on all the places it's called.

        let kind = deserialize_to_string(kind).map_err(|msg| de::Error::custom(msg))?;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think quite a nice way is in Attribute::deserialize and other call-sites of deserialize_to_string to have

let kind = String::deserialize(&kind).map_err(de::Error::custom)?;

then deserialize_to_string is no longer necessary, and its value parameter does not have to be passed by copy.


assert_eq!(entity.len(), 2);
assert_eq!(entity[0].uid().to_string(), "Jans::Role::\"admin\"");
assert_eq!(entity[1].uid().to_string(), "Jans::Role::\"user\"");
Copy link
Contributor

@djellemah djellemah Jan 9, 2025

Choose a reason for hiding this comment

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

The ordering of entity here causes an intermittent test failure.
That's solvable using
entity.sort_by(|a,b| a.uid().cmp(&b.uid()) );
and obviously changing to let mut entitity = ...

I suspect that's a result of my suggested UnifyClaims change from yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-jans-cedarling Touching folder /jans-cedarling
Projects
None yet
6 participants