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

fix(jans-config-api): user attributes not updated #2753

Closed
moabu opened this issue Oct 28, 2022 · 19 comments · Fixed by #4123
Closed

fix(jans-config-api): user attributes not updated #2753

moabu opened this issue Oct 28, 2022 · 19 comments · Fixed by #4123
Assignees
Labels
comp-jans-config-api Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality
Milestone

Comments

@moabu
Copy link
Member

moabu commented Oct 28, 2022

When creating a new user and providing an array for the attribute CustomObjectClasses, the request is processed, however, after loading the newly created user, the attribute is empty.

@moabu moabu assigned moabu and pujavs and unassigned moabu Oct 28, 2022
@mo-auto mo-auto added comp-jans-config-api Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality labels Oct 28, 2022
@pujavs
Copy link
Contributor

pujavs commented Nov 3, 2022

In LDAP DB the CustomObjectClasses is returned in response but not in My-SQL or Postgresql.
assigning to @yurem
LDAP-Screenshot:
image
MySQL-Screenshot:
image

@moabu moabu added this to the 1.0.4 milestone Nov 3, 2022
@moabu moabu modified the milestones: 1.0.4, 1.0.5 Nov 14, 2022
@yurem
Copy link
Contributor

yurem commented Nov 18, 2022

It works as expected. Only LDAP ORM stores all provided OC because it's LDAP specific approach. In CB/Spanner/MySQL/PostgreSQL ORM uses only first OC to determine table name. I takes it from bean definition:

@DataEntry
@ObjectClass(value = "jansPerson")
public class User extends SimpleUser {

Why do you think we need to support storing OCs if DB not really need it?

@pujavs
Copy link
Contributor

pujavs commented Nov 18, 2022

@moabu, request you to please provide your feedback on @yurem query.

@yurem
Copy link
Contributor

yurem commented Nov 18, 2022

Also, please update issue description to specify that the question is about CustomObjectClasses

@moabu
Copy link
Member Author

moabu commented Nov 21, 2022

I understand, but then it would be great if the backend would return an error, when the storage is not LDAP and custom OCs are provided.Now it suggests that all provided attributes were persisted, however, that one wasn't. This affects state management of the configuration ( which is saved externally outside the Jans deployment ) in our case Terraform. This would cause the Terraform state never to converge, as there would always be a delta between what Terraform believes the state of user should be (include the custom OCs) and what the backend reports is the state of the user (namely no OCs in case of MySQL DB).

Please note that you need to think about this in terms of the last applied state of the config. Having no clear response causes the integration to lose track of what actually happened as described above.

yurem added a commit that referenced this issue Nov 29, 2022
Co-authored-by: Yuriy Movchan <Yuriy.Movchan@gmail.com>
@yurem
Copy link
Contributor

yurem commented Nov 29, 2022

I've added explicit check if there are more than one OC in entry for persist/update. Let's see what new test run will show us

@moabu
Copy link
Member Author

moabu commented Dec 5, 2022

Tests went through.

yurem added a commit that referenced this issue Dec 15, 2022
yurem added a commit that referenced this issue Dec 15, 2022
Co-authored-by: Yuriy Movchan <Yuriy.Movchan@gmail.com>
@moabu
Copy link
Member Author

moabu commented Dec 22, 2022

Ok so this works when creating an oidc client as the error is posted but not when creating a a user

This issue is for creating custom users. The payload I tested with is the following:

{
  "jansStatus": "active",
  "userId": "exampleUsr1",
  "customObjectClasses": [
    "top",
    "jansCustomPerson"
  ],
  "mail": "exampleUsr1@jans.io",
  "displayName": "Default Test User",
  "givenName": "exampleUsr1",
  "userPassword": "pwd123"
}

@pujavs
Copy link
Contributor

pujavs commented Dec 23, 2022

@yurem, will this require change in orm for jansPerson as per latest comment from @moabu ?

yurem added a commit that referenced this issue Dec 23, 2022
@yurem
Copy link
Contributor

yurem commented Dec 23, 2022

I added code to PersonService to remove not required OC on person creation
#3403

yurem added a commit that referenced this issue Dec 26, 2022
Co-authored-by: Yuriy Movchan <Yuriy.Movchan@gmail.com>
yurem added a commit that referenced this issue Dec 28, 2022
* fix: user attributes not updated #2753

* fix: fix clnId type definition and token indexes
@moabu moabu modified the milestones: 1.0.6, 1.0.7 Jan 9, 2023
@pujavs
Copy link
Contributor

pujavs commented Jan 13, 2023

@moabu, did you get a chance to test the fix done by @yurem

@moabu
Copy link
Member Author

moabu commented Jan 13, 2023

Yes @pujavs . I'll be closing tickets next week after running final tests

@moabu moabu closed this as completed Feb 15, 2023
@moabu moabu reopened this Feb 15, 2023
@moabu
Copy link
Member Author

moabu commented Feb 15, 2023

This issue is still present

When creating a new user and providing an array for the attribute CustomObjectClasses, the request is processed, however, the newly created user has an empty value for CustomObjectClasses attribute.

The following case was tested with mysql and postgres

Example payload in hcl same can be done via curl on the configapi endpoint to create then get the custom user. :

resource "jans_custom_user" "example" {
  jans_status           = "active"
  user_id               = "exampleUsr1"
  custom_object_classes = ["top", "jansCustomPerson"]
  mail                  = "exampleUsr1@jans.io"
  display_name          = "Default Test User"
  given_name            = "exampleUsr1"
  user_password         = "pwd123"
}

Result of terraform plan:

  # jans_custom_user.example will be updated in-place
  ~ resource "jans_custom_user" "example" {
      ~ custom_object_classes  = [
          + "top",
          + "jansCustomPerson",
        ]
        id                     = "8eb695ea-89c5-45be-b38c-7f6632b8858a"
        # (10 unchanged attributes hidden)
    }

@moabu moabu modified the milestones: 1.0.7, 1.0.8, 1.0.9 Mar 1, 2023
@pujavs
Copy link
Contributor

pujavs commented Mar 7, 2023

Tested on My-SQL custom_object_classes is not saved, which is as expected from orm point of view as confirmed by @yurem -#2753 (comment)

I think only way to handle the discrepancy in input and response is to throw error if custom_object_classes is part of request but DB is not LDAP.

  • Request/Resposne:
    image

  • DB:
    image

@moabu moabu modified the milestones: 1.0.9, 1.0.10 Mar 9, 2023
@yurem
Copy link
Contributor

yurem commented Mar 9, 2023

@pujavs Thank you for checking this. Yes, I also not see issue. But I have question about 2 custom_object_classes. Is config API removes second OC or call ORM with these 2 values?

@pujavs
Copy link
Contributor

pujavs commented Mar 9, 2023

@yurem, yes we had added the code to remove the customObjectClasses if persistence was not LDAP.
See reference
image

Do you want me to remove this?

@yurem
Copy link
Contributor

yurem commented Mar 10, 2023

@moabu If I understand correctly you original idea was next. Data in request and response must match. According to this if there are 2 OC server should return 2 OC. But this possible only with LDAP backed. Is API in this case should return response?

@pujavs
Copy link
Contributor

pujavs commented Mar 10, 2023

Earlier implementation in config-api to handle non-LDAP attributes was to ignore/remove these attributes, however since custom_object_classes(customObjectClasses) is supported only by LDAP and the persistence type here is MySQL, the expected output is to have been an error stating the same.

Following changes made in config-api to handle Terraform requirement:

  1. Introduce boolean parameter removeNonLDAPAttributes in config-api endpoint to indicate if custom attribute to handle. Default value will be true
  2. If value is true remove custom attributes from request not supported by DB and proceed with operation. (This is current flow)
  3. If value is false then do not remove custom attributes from request and ORM will throw error stating the same (this as confirmed by Yuriy Movchan is already happening). For Terraform the new attribute value has to be false.

Testing post implementation
image

@pujavs
Copy link
Contributor

pujavs commented Mar 10, 2023

Changes implemented via PR 4123

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp-jans-config-api Component affected by issue or PR kind-bug Issue or PR is a bug in existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants