arshilkxwork opened a new pull request, #3290:
URL: https://github.com/apache/hugegraph/pull/3290
## Purpose of the PR
- close #3284
`AuthenticationFilter` decoded the Basic credential as US-ASCII and split it
on every colon. The user API accepts passwords with non-ASCII characters or a
colon, so once an account had such a password it could no longer use Basic
auth: the non-ASCII case got 401 and the colon case got 400. The admin account
is affected too.
## Main Changes
The filter now decodes the credential as UTF-8 (RFC 7617 section 2.1) and
splits it at the first colon. Everything after that colon is the password. A
credential with no colon, an empty user-id or an empty password still gets 400.
The grizzly `Charsets` import is replaced with `StandardCharsets`, and the TODO
that pointed at this issue is gone.
Following the discussion on the issue, two things stay out of this PR:
- No new validation on user creation or password update. A user name
containing a colon still cannot log in with Basic auth, which was already the
case before this change.
- The Helm chart guard on the admin password (`values.schema.json`, the
Server wrapper, the README note) is unchanged. The chart README ties its
removal to a released image carrying this fix and to the separate entrypoint
fix for spaces.
## Verifying these changes
- [x] Need tests and can be verified as follows:
- New `LoginApiTest#testBasicAuthWithNonAsciiOrColonPassword` creates
users with the passwords `ädminpass1` and `new:pass1234`, sends `GET
graphspaces/DEFAULT/graphs` with a UTF-8 Basic header, and expects 200 for the
right password and 401 for a wrong one.
- Run locally against a RocksDB server configured as in
`run-api-test.sh`: `LoginApiTest` and `UserApiTest` pass (9 tests). With the
old filter the new test fails with 401 on `ädminpass1`.
- Checked by hand with curl after the change: `admin:pa` gets 200,
`admin:wrong` 401, and `admin:`, `admin` and `:pa` 400, the same as before.
- The full api-test suite was not run locally.
## Does this PR potentially affect the following parts?
- [ ] Dependencies ([add/update
license](https://hugegraph.apache.org/docs/contribution-guidelines/contribute/#321-check-licenses)
info &
[regenerate_known_dependencies.sh](../install-dist/scripts/dependency/regenerate_known_dependencies.sh))
- [ ] Modify configurations
- [ ] The public API
- [x] Other affects (typed here): REST Basic auth now accepts passwords
that contain non-ASCII characters or `:`
- [ ] Nope
## Documentation Status
- [ ] `Doc - TODO`: required documentation is pending; complete it before
merging.
- [ ] `Doc - Done`: documentation is included here or linked below.
- [x] `Doc - No Need`: no user-visible documentation is affected.
The Helm README still describes the ASCII decoding, but it documents the
released `:latest` image. That text should change together with the chart guard
once a release ships this fix.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
