smolnar82 commented on PR #1454: URL: https://github.com/apache/knox/pull/1454#issuecomment-6189394952
> @smolnar82 What do you think of the requirement to add the new endpoints? Should I worry about making that backwards compatible (e.g.,, deriving a default value from the lookup endpoint)? Thanks for raising this. Short answer: I don't think backward compatibility is needed here, and I'd avoid deriving defaults from the lookup endpoint. **Backward compatibility**: Upgrades are out of scope for this change, so it's fine for the rest strategy to require all three endpoints (lookup, list, get). Failing fast at startup with a clear error when one is missing is the right behavior. It's better than starting with role listing or reverse lookup silently disabled. **Deriving defaults**: I'd skip this. It means guessing how the roles service lays out its URLs, and a wrong guess sends Knox to the wrong endpoint without any obvious error. The docs example in this PR shows how easily that happens: the lookup URL moved from /auth/roles to /auth/roles/lookup, and /auth/roles became the list endpoint. Separate endpoints also make sense on their own terms, since list and get-by-id are different resources from the POST lookup. What we do need is documentation, because anyone implementing the roles service has to know exactly what Knox calls and what it expects back. Could you add these to `service_ldap_server.md`? 1. **Common Properties table**: say that all three `gateway.ldap.roles.*.rest.api.endpoint` properties are required when `gateway.ldap.roles.lookup.strategy=rest`, and that the gateway fails to start if one is missing. For the `get` endpoint, say that Knox requests `<endpoint>/<URL-encoded role id>`, so it should be configured without a trailing slash. 2. **Usage example**: remove the trailing slash from the get endpoint (`http://localhost:8080/auth/roles/`). As written, Knox requests .`../roles//<id>`. 3. A "**Roles REST API contract**" subsection under the `Roles Lookup Interceptor`, covering all three calls: - _Lookup_: `POST`, request `{"user_id": "...", "groups": [...]}`, response `{"user_id": "...", "roles": [{"scope": "...", "name": "..."}]}`. This one was never documented either. - _List_: `GET ?offset=N`, plus `name/scope` query params when filtering. Response `{"data": [{"id", "name", "scope"}], "pagination": {"limit", "offset", "total"}}`. Knox keeps advancing offset until it reaches total, and it doesn't send limit. - _Get_: `GET /<id>`, response `{"id", "name", "scope", "members": [{"id", "member_type": "user"|"group", "member_id"}]}`. - _Common rules_: any status other than `200` fails the LDAP search, and role names match case-insensitively. A role appears in LDAP as `cn=<name>` when unscoped and `cn=<scope>:<name>` when scoped. 4. **File strategy format** a `roles.json` example (the CI one works). Note that listing only returns roles with at least one assignment. 5. **Role listing and reverse lookup behavior**: - which searches count as "all groups"; - that listed role entries have no member attribute; - how (`memberOf=cn=<role>,ou=groups,<baseDn>`) is rewritten: users are matched on `uid`, and groups are matched through `memberOf`; - how this interacts with u`seMemberOf` and r`ecursiveGroupResolution`; - that each `memberOf` condition costs extra roles-service calls; - an `ldapsearch` example matching the one in [KNOX-3487](https://issues.apache.org/jira/browse/KNOX-3487). 6. **maxFilterLength**: the docs say the default is `4096`, but the code uses `2000`. Please make them match. 7. **Troubleshooting**: the `DEBUG` log messages for list/get requests and responses, the `INFO` message logged when a filter is split, and using the bypass control to compare results with and without role mapping. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
