arshilkxwork commented on code in PR #3290:
URL: https://github.com/apache/hugegraph/pull/3290#discussion_r4241667305
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/AuthenticationFilter.java:
##########
@@ -184,19 +184,16 @@ protected User authenticate(ContainerRequestContext
context) {
if (auth.startsWith(BASIC_AUTH_PREFIX)) {
auth = auth.substring(BASIC_AUTH_PREFIX.length());
- // TODO: decode the Basic credential as UTF-8 and split it on the
first colon only.
- // Decoding as ASCII and splitting on every colon makes a
non-ASCII password answer
- // 401 and a password containing ':' answer 400, although both
were accepted at
- // account creation. The Helm chart (helm/hugegraph) refuses such
admin passwords in
- // its schema and Server wrapper; drop that guard once this is
fixed.
- auth = new String(DatatypeConverter.parseBase64Binary(auth),
Charsets.ASCII_CHARSET);
- String[] values = auth.split(":");
- if (values.length != 2) {
+ // RFC 7617 section 2.1 allows only UTF-8 as the declared charset,
and it bars a
+ // colon from the user-id alone, so split on the first one and
keep the rest
+ auth = new String(DatatypeConverter.parseBase64Binary(auth),
StandardCharsets.UTF_8);
Review Comment:
Thank you for the suggestion. I agree that advertising the expected charset
is the signal RFC 7617 section 2.1 defines, and I would like to confirm the
intended scope before implementing it.
At present the 401 responses do not carry a Basic challenge. The
`WWW-Authenticate` header contains the error text instead: on a local build of
7e048d1 it is `Missing authentication credentials` when no credentials are sent
and `Incorrect username or password` when they are wrong, on both the REST and
the Cypher paths. Advertising the charset therefore means replacing that value
with a standard challenge such as `Basic realm="...", charset="UTF-8"`.
Browsers respond to a Basic challenge by showing their native login dialog,
so this would also change what browser-based clients such as Hubble display
when credentials are missing or a session has expired.
Would you prefer this change in this PR, or in a separate PR together with
the client-side encoding work for Jersey and OkHttp? I can implement it in
either place, with a response-level regression for both entry points and a note
on the remaining client limitations. If it belongs here, please also let me
know which realm value you would like to use.
--
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]
