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

Custom identity traits not marked as required in self-service flows response #1058

Closed
realStandal opened this issue Feb 10, 2021 · 7 comments
Labels
feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Milestone

Comments

@realStandal
Copy link

Describe the bug

I've installed and configured a Kratos container. I'm not concerned with specifics in terms of the functionality, more just want to test out the software; in-doing so, I've used this identity schema as my example and as Kratos' default.

I've noticed that the response from self-service/X/flows?id=xxx..., at least for the registration endpoint, does not reflect the fields which I have defined as required. Using the linked identity-schema, I should receive a response indicating the traits.email field is required; this is not the case.

Response I'm receiving:

...(remainder of Kratos' response, nothing wrong there)
"method": "POST", (just for reference)
"fields": [
    {
        "name": "csrf_token",
        "type": "hidden",
        "required": true,
        "value": "secretive"
    },
    {
        "name": "password",
        "type": "password",
        "required": true
    },
    {
        "name": "traits.email",
        "type": "email"
    },
    {
        "name": "traits.name.first",
        "type": "text"
    },
    {
        "name": "traits.name.last",
        "type": "text"
    }
]

When building the user-interface to back browser-flows, I'll need to explicitly mark that the traits.email field is required independent logic used for generating that field based on this response.

Hopefully that makes sense, I'm assuming this is a bug; I expect Kratos would remember I've specified a field as being required.

Reproducing the bug

  1. clone this repository into an empty directory.
  2. Use docker-compose to stand-up the Kratos containers.
    • This is done using the quickstart.yml file provided in this repo's root.
    • For my use, I've ignored kratos-selfservice-ui-node
    • Reconfigure selfservice endpoints in Kratos' configuration to point to a new front-end app.
  3. Visit http://127.0.0.1:4433/self-service/registration/browser and get redirected to NEW-URL/auth/register with ?flow= set, as expected.
  4. Use the flowId from the redirected URL to make a request against http://127.0.0.1:4433/self-service/registration/flows?id=FLOWID using a program such as Postman or curl.
  5. The response will return as expected, resembling what I gave above.

Server logs

Don't believe this is applicable as nothing is going wrong, technically.

Server configuration

I'll omit a majority to save-space; let me know if you need additional fields. I changed a few file locations to match my project's structure, but again nothing is broken so its ignorable.

# docker-compose.yml (quickstart.yml)
# No-change
// identity.schema.json
"title": "React User",
  "type": "object",
  "properties": {
    "traits": {
      "type": "object",
      "properties": {
        "email": {
          ...
        }
      },
      "required": ["email"],
      "additionalProperties": false
    }
  }
# kratos-config.yml
identity:
  default_schema_url: file:///etc/config/kratos/identity.schema.json

selfservice:
  default_browser_return_url: http://127.0.0.1:3000/

  ...

Expected behavior

Based on the example kratos-selfservice-ui-node, I can expect to build my UI's components using the response of /self-service/.../flow. I can the expect that Kratos will provide me with all the necessary details to build that UI. Not knowing which fields-that I've defined-are required and which are not complicates this workflow.

I expect fields I define as required to appear similar to how password and csrf_token do.

Environment

  • Docker Engine: 20.10.2
  • Docker-Compose: 1.27.4
    • Compose-Config: 3.9
  • Kratos: v0.5.5-alpha.1 (SQLite)

Additional context

N/a

@aeneasr
Copy link
Member

aeneasr commented Feb 10, 2021

Thank you for the report! It's possible that the name resolution is broken in the required detection.

Could you check if adding traits to the required fields helps?

It's also weird because traits.email is the identifier and thus required always (by the password method)...

@realStandal
Copy link
Author

realStandal commented Feb 10, 2021

Not a problem.

Did just that, here is my updated identity.schema.json:

{
  "$comment": "Taken from https://github.com/ory/kratos/blob/master/contrib/quickstart/kratos/email-password/identity.schema.json",
  "$id": "https://schemas.example.com/kratos/presets/identity.schema.json",
  "$schema": "http://json-schema.org/draft-07/schema#",
  "title": "React User",
  "type": "object",
  "properties": {
    "traits": {
      "type": "object",
      "properties": {
        "email": {
          "type": "string",
          "format": "email",
          "title": "E-Mail",
          "minLength": 3,
          "ory.sh/kratos": {
            "credentials": {
              "password": {
                "identifier": true
              }
            },
            "verification": {
              "via": "email"
            },
            "recovery": {
              "via": "email"
            }
          }
        },
        "name": {
          "type": "object",
          "properties": {
            "first": {
              "type": "string"
            },
            "last": {
              "type": "string"
            }
          }
        }
      },
      "required": ["email"],
      "additionalProperties": false
    }
  },
  "required": ["traits"]
}

Hit the self-service/browser/... endpoint for registration, this is the /flows?... response:

{
    "id": "c7ad7d64-b645-4fc0-a2d4-d52abd6e27b6",
    "type": "browser",
    "expires_at": "2021-02-10T13:44:46.8624055Z",
    "issued_at": "2021-02-10T13:34:46.8624055Z",
    "request_url": "http://127.0.0.1:4433/self-service/registration/browser",
    "messages": null,
    "methods": {
        "password": {
            "method": "password",
            "config": {
                "action": "http://127.0.0.1:4433/self-service/registration/methods/password?flow=c7ad7d64-b645-4fc0-a2d4-d52abd6e27b6",
                "method": "POST",
                "fields": [
                    {
                        "name": "csrf_token",
                        "type": "hidden",
                        "required": true,
                        "value": "shhh"
                    },
                    {
                        "name": "password",
                        "type": "password",
                        "required": true
                    },
                    {
                        "name": "traits.email",
                        "type": "email"
                    },
                    {
                        "name": "traits.name.first",
                        "type": "text"
                    },
                    {
                        "name": "traits.name.last",
                        "type": "text"
                    }
                ]
            }
        }
    }
}

No change far as I can tell.

I'm sure it doesn't help solve the issue, but this seems to be expected functionality based off the docs that goes with ui-selfservice...

Later on in the same document, it describes not filling in the identifier (i.e. e-mail), and still submitting the request as the preferred workflow. In that case, the system responded with "Email must be greater than 3 characters got 0". But, it is kind of a fix-before-it-happens situation imo.

@realStandal
Copy link
Author

realStandal commented Feb 11, 2021

Just wanted to also add the response from /self-service/login/flows?.... Looks to me like it is just the custom trait. In the Login-flow, where it uses Kratos' identifier field and not the one I defined.

{
    "id": "6410d1ab-cbea-4376-9b0c-a3f52fb7522c",
    "type": "browser",
    "expires_at": "2021-02-11T03:09:25.6988815Z",
    "issued_at": "2021-02-11T02:59:25.6988815Z",
    "request_url": "http://127.0.0.1:4433/self-service/login/browser",
    "messages": null,
    "methods": {
        "password": {
            "method": "password",
            "config": {
                "action": "http://127.0.0.1:4433/self-service/login/methods/password?flow=6410d1ab-cbea-4376-9b0c-a3f52fb7522c",
                "method": "POST",
                "fields": [
                    {
                        "name": "identifier",
                        "type": "text",
                        "required": true,
                        "value": ""
                    },
                    {
                        "name": "password",
                        "type": "password",
                        "required": true
                    },
                    {
                        "name": "csrf_token",
                        "type": "hidden",
                        "required": true,
                        "value": "You'll never guess"
                    }
                ]
            }
        }
    },
    "forced": false
}

@aeneasr
Copy link
Member

aeneasr commented Feb 12, 2021

Ok, this is definitely a bug then! I am reworking the whole ui stuff anyways for kratos 0.6 so I will tag it for there to be fixed!

@aeneasr aeneasr self-assigned this Feb 12, 2021
@aeneasr aeneasr added this to To do in Maintainer's Board via automation Feb 12, 2021
@aeneasr aeneasr added this to the v0.6.0-alpha.1 milestone Feb 12, 2021
@aeneasr
Copy link
Member

aeneasr commented Jun 15, 2021

I looked into this again. Basically jsonschemax.Path currently does not have a Required attribute which is why we can not detect in the form builder if a field is required or not.

This boils down to jsonschema which has required as a property of the path's parent. Meaning that we would need to keep track of required chains.

This is currently probably overkill in terms of complexity when compared to the UI advantage we get (setting a HTML form hint correctly). Basically we would need to refactor

https://github.com/ory/x/blob/9964be3af0503b7279e4604bb75fc3779f4b0ad6/jsonschemax/keys.go#L180

and add e.g. schemaParent *jsonschema.Schema to the arguments list which we would then use to compute the required value by checking schemaParent.Required.

That would of course need some tests and tests adjustments. Integrating it in Ory Kratos should be straight forward but we probably have to update a couple of fixtures.

I'm moving this to unplanned and tagging as help wanted.

@aeneasr aeneasr added the feat New feature or request. label Jun 15, 2021
@aeneasr aeneasr removed their assignment Jun 15, 2021
@aeneasr aeneasr modified the milestones: v0.7.0-alpha.1, unplanned Jun 15, 2021
@aeneasr aeneasr removed this from To do in Maintainer's Board Jun 15, 2021
@aeneasr aeneasr added good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one. labels Jun 15, 2021
@vinckr
Copy link
Member

vinckr commented Aug 4, 2021

Is this a duplicate of #400 ?

@aeneasr
Copy link
Member

aeneasr commented Aug 4, 2021

yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Projects
None yet
Development

No branches or pull requests

3 participants