chore: update documentation - #103
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #103 +/- ##
=======================================
Coverage 77.26% 77.26%
=======================================
Files 13 13
Lines 884 884
Branches 160 160
=======================================
Hits 683 683
Misses 126 126
Partials 75 75
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
6a13dc2 to
9b493f1
Compare
9b493f1 to
c175314
Compare
jason-famedly
left a comment
jason-famedly
left a comment
There was a problem hiding this comment.
Not sure if you were ready for this or not, but had some time and I'll be off on monday so here's something to work with. Looks much better in a lot of places, I just have questions and curiosities 😄
|
|
||
| | Parameter | Type | | ||
| | --------- | --------- | | ||
| | `type` | `'basic'` | |
There was a problem hiding this comment.
Was this supposed to be missing bearer like was there previously?
| * [NotifyOnRegistration](#notifyonregistration) | ||
| * [ePAConfig](#epaconfig) | ||
| * [ConfigTypes](#types) | ||
| * [Path](#path) |
There was a problem hiding this comment.
I see Path. There is a heading below for PathList, is that ok to skip?
| * [JwtValidationConfig](#jwtvalidationconfig) | ||
| * [IntrospectionValidationConfig](#introspectionvalidationconfig) | ||
| * [NotifyOnRegistration](#notifyonregistration) | ||
| * [ePAConfig](#epaconfig) |
There was a problem hiding this comment.
ePAConfig is linked here twice, is that intended? (once up here and another down near "usage" links).
I assume for the links that there is no difference in that the lower one of them has the capital letters in it and this one up here is all lower case
| - `'localpart'` — must be localpart, e.g. `alice` | ||
| - `'user_id'` — could be localpart or fully qualified username | ||
| - `null` — the username is ignored, it will be source from the token or introspection response | ||
| `jwt_validation` and `introspection_validation` contain a bunch of `*_path` optional fields. Each of these, if specified will be used to source either localpart, user id, fully qualified user id, admin permission, or email from jwt claims and introspection response. They values are going to be compared for equality, if they differ, authentication would fail. Be careful with these, as it is possible to configure in such a way that authentication would always fail, or, if `username_type` is `null`, no user id data can be sourced, thus also leading to failure. |
There was a problem hiding this comment.
| `jwt_validation` and `introspection_validation` contain a bunch of `*_path` optional fields. Each of these, if specified will be used to source either localpart, user id, fully qualified user id, admin permission, or email from jwt claims and introspection response. They values are going to be compared for equality, if they differ, authentication would fail. Be careful with these, as it is possible to configure in such a way that authentication would always fail, or, if `username_type` is `null`, no user id data can be sourced, thus also leading to failure. | |
| `jwt_validation` and `introspection_validation` contain several `*_path` optional fields. Each of these, if specified, will be used to source either localpart, user id, fully qualified user id, admin permission, or email address from jwt claims and introspection response. The values will be compared for equality. If they differ, authentication will fail. WARNING: it is possible to configure in such a way that authentication would always fail, or, if `username_type` is `null`, no user id data can be sourced, thus also leading to failure. |
I don't like that last sentence much but I'm also not sure yet how to word it better. Maybe reorganize it but without changing the intent?
There was a problem hiding this comment.
Taking a step back makes me wonder if some of it(that last sentence) can just be deleted, and instead the table section just above be changed for username_type to say (optional, required if using jwt_validation and/or introspection_validation or such. It looks better, but since I do not know if it is factually correct, I'm not certain it would be better.
In that same area(the table above), I'm wondering if some clarification needs to be added. Is there any circumstance where the optional jwt_validation and the introspection_validation configuration can both be not filled in? Looks like further down it has to be one or the other. Perhaps that can be wrapped into the table? What do you think?
There was a problem hiding this comment.
That last sentence must really be bothering me 🤣 I keep going back to it. Further down it looks like the username_type is mentioned in the null context again. I feel like this all needs a good straightening out on what it is supposed to mean.
| **Table of Contents** | ||
|
|
||
| * [Installation](#installation) | ||
| * [Configuration](#configuration) |
There was a problem hiding this comment.
I may have missed it, but is the OIDCConfig linked somewhere?
| * [Path](#path) | ||
| * [BasicAuth](#basicauth) | ||
| * [BearerAuth](#bearerauth) | ||
| * [HttpAuth](#httpauth) |
There was a problem hiding this comment.
Perhaps let's put the HttpAuth above the related sub types and then nest them? Is that to much indentation? (Trying to match how it's laid out below in the relevant sections). Should we mention the NoAuth option? Undocumented things tend to be discovered eventually. Perhaps we should remove that option?
There was a problem hiding this comment.
I agree that we actually don't need the NoAuth type.
|
|
||
| `project_id` and `organization_id` accept both String and Integer types. Integer will automatically converted into String. | ||
|
|
||
| `allowed_client_ids` accepts both a list or a space-separted string. A space-separted string will be converted into a list. |
There was a problem hiding this comment.
| `allowed_client_ids` accepts both a list or a space-separted string. A space-separted string will be converted into a list. | |
| `allowed_client_ids` accepts both a list or a space-separated string. A space-separated string will be converted into a list. |
| It is recommended to have `require_expiry` set to `true` (default). As for `allow_registration`, it depends on usecase: If you only want to be able to log in *existing* users, leave it at `false` (default). If nonexistant users should be simply registered upon hitting the login endpoint, set it to `true`. | ||
|
|
There was a problem hiding this comment.
Is this section for the JwtConfig? Let's move it down there instead of up here where it is orphaned(unless it belongs to two sections in which case we will need a different option)
| It is recommended to have `require_expiry` set to `true` (default). As for `allow_registration`, it depends on usecase: If you only want to be able to log in *existing* users, leave it at `false` (default). If nonexistant users should be simply registered upon hitting the login endpoint, set it to `true`. | |
| #### Recommendations | |
| - Leave `require_expiry` set to `true` (default). | |
| - Leave `allow_registration` set to `false` (default) or any user can register simply by logging in rather they already exist or not. | |
Is that to strongly worded? Feel free to soften it up a bit. These options feel dangerous.
Can we include some extra bit of data of why require_expiry is recommended to be left on true? Do we know why? 🤣
| | `allow_registration` | Bool (defaults to `false`) | | ||
|
|
||
|
|
||
| `project_id` and `organization_id` accept both String and Integer types. Integer will automatically converted into String. |
There was a problem hiding this comment.
I was going to ask the other day, but it slipped my mind. Why is this allowed? Is there some reason that the yaml doesn't just auto convert an str into an int? I mean, if we are pushing it into a int anyway, would that raise a TypeError if they put the wrong thing? Why don't we just document that it has to be an int to begin with?
(Yes yes, breaking changes, but is it?)
There was a problem hiding this comment.
it is normally typed as "string" in our helm chart, but in this README, it is int
organization_id: 1234
# Zitadel Project ID, used for validating the audience of the returned token.
project_id: 5678
so in the last pr, cursor review pointed this out
#101 (comment)
| | Parameter | Type | | ||
| | ------------------ | ---------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | `validator` | [`Validator`](#validator) (defaults to [`Exist`](#exist)) | | ||
| | `require_expiry` | Bool (defaults to `false`) | |
There was a problem hiding this comment.
I wonder why this require_expiry defaults to false while the one above in the other section defaults to true?
There was a problem hiding this comment.
As far as I know it is also recommended to be true, but it was like this previously.
2 participants
SYN-121
Rendered