HTTP_BASIC_AUTH_HANDLER #70
Conversation
Codecov Report
@@ Coverage Diff @@
## master #70 +/- ##
==========================================
- Coverage 78.51% 73.76% -4.76%
==========================================
Files 57 52 -5
Lines 9226 7029 -2197
Branches 908 516 -392
==========================================
- Hits 7244 5185 -2059
+ Misses 1982 1844 -138
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Can you provide more details on how would a user register initially and subsequently? The users are authenticated by the web server and then upon receiving an authenticated request how would the Flood onboard the user in its database? What would be the frontend user experience for this?
config.cli.js
Outdated
| @@ -36,7 +36,7 @@ const {argv} = require('yargs') | |||
| }) | |||
| .option('auth', { | |||
| describe: 'Access control and user management method', | |||
| choices: ['default', 'none'], | |||
| choices: ['default', 'httpbasic', 'none'], | |||
There was a problem hiding this comment.
header would be ambigous: what if tomorrow we add http digest, or any other bearer token?
There was a problem hiding this comment.
With logics of this PR, any header authentication method shall only take the username. So if digest method needs to be supported, we simply parse the username from that type of header without changing other logics. Headers which don't supply username such as Bearer can't be supported.
There was a problem hiding this comment.
the logic is executed only when type httpbasic, and in case of digest the authorization header will be parsed differently also.
Bearer could provide the username in the payload, and we will have a method bearerSomething and add a branch in the switch.
or the bearer could provided metadata needed to extract the username for in other way and finally compare with the jwt token username, adding extra logic.
that's why reducing everything to header method it is not the correct solution in my opinion.
There was a problem hiding this comment.
For all other types which username can be parsed, the authentication and user management logics are the same. The only difference is how to parse the username which can be handled by a simple switch case. I prefer that we use header to be more general.
There was a problem hiding this comment.
The only difference is how to parse the username which can be handled by a simple switch case. I prefer that we use header to be more general.
that's what currently happens: a switch case on the auth value for different logic how to parse the username.
if we have a generic header value the switch should need extra information (we could have a headertype settings, it won't change much indeed). if we don't have this extra information we have to inspect the header and apply the parse method that will apply. it would be nice, but I doubt that there could be the cases where the same header inspection won't give enough information on how to actually parse it: then I don't see any other solution than applying multiple ambigous parsing until one won't succed.
what do you think?
There was a problem hiding this comment.
For now, we can enforce the Basic type.
There was a problem hiding this comment.
so auth value header that will assume only Basic type?
then it's only a matter of replacing httpbasic with header
I'm still not convinved this is the best way to easy future support to different methods, but as you prefer
There was a problem hiding this comment.
Yeah. It would be very straightforward to add support for other auth header types in the future. After all, we only want the username.
server/config/passport.ts
Outdated
| } | ||
|
|
||
| credentials = parsedResult.data; | ||
| token = getAuthToken(credentials.username); |
There was a problem hiding this comment.
No. We shall not unconditionally bypass the token in cookie validation here. We need cookie to utilize browser's protections against session hijacking.
For example, if there is no protection against session hijacking, an attacker can POST something like {"urls":["https://example.virus/malicious.torrent"],"destination":"/your/most/important/folder"} to API endpoint /api/torrents/add-urls of your Flood instance FROM their own site while utilizing your (already authenticated) credentials.
HTTP basic auth and disabled auth are especially vulnerable to this type of attacks. Browser always include basic auth header when user is authenticated no matter where the request comes from. auth=none sent cookie with token unconditionally when /authenticate or /verify is accessed. You may send cookie according to your design in /authenticate and /verify endpoints.
More info about the session hijacking:
https://security.stackexchange.com/questions/234341/http-basic-auth-and-csrf
There was a problem hiding this comment.
I wanted to rely only on the http basic credentials, but actually I tried another implementation before this final:
- the
jwtFromRequestwill remain as it is in master - I added
passReqToCallback: trueto the passport options and the on https://github.com/jesec/flood/blob/master/server/config/passport.ts#L26 I will comparejwtPayload.usernamewith http basic username and continue only if they match.
please, let me know this is the accepted solution so I can implement
There was a problem hiding this comment.
There is no need to modify this file. You only need to add logics to /verify and /authenticate endpoints. Simply parse the header to get the username and then reply with a generated token.
There was a problem hiding this comment.
and then for all the other endpoints you could send different credentials between jwt cookie and http basic.
that's really a mess, in my opinion.
There was a problem hiding this comment.
Other endpoints simply don't handle header. They only validate the cookie. Only selected endpoints handle alternative auth methods. This allows uniformity in auth enforcement and prevents errors in authentication system which can be disastrous.
There was a problem hiding this comment.
Makes sure this added logic is an added step. Don't replace other logics for simplicity and the peace of mind that this won't hurt security.
There was a problem hiding this comment.
there is indeed a possibility of inconsistent header/cookie authentication state.
indeed: imagine the following setup (all behind a nginx proxy that forward to related upstream and different httpasswd by path):
- http://adomain.com/flood (users in httpasswd:
guest,aFloodUser) - http://adomain.com/grafana (users in httpasswd:
guest,admin)
I login to the first as aFloodUser
I open a new tab for the second with guest login
I would not receive a 401 from /flood but still the username will differ from the one in the jwt token.
My changes will force a 401 from flood
Makes sure this added logic is an added step. Don't replace other logics for simplicity and the peace of mind that this won't hurt security.
sure, that's the whole point: inject new logic only in the switch branch for httpbasic. please validate during review that I didn't change the current behaviour in different/default branches
There was a problem hiding this comment.
Great. The logics look OK to me. However, the organization of functions needs some improvements in my opinion. May I push directly to the PR to tweak it a bit?
There was a problem hiding this comment.
great. let me push the last changes with frontend handling the prefilled registration and then feel free to push any tweak to the PR.
There was a problem hiding this comment.
I posted the patch: #70 (comment). You can apply it by yourself. Or would you want me to apply it?
server/routes/api/auth.ts
Outdated
| * @return {} 200 - success response | ||
| */ | ||
| router.get('/logout', (_req, res) => { | ||
| res.clearCookie('jwt').send(); | ||
| switch (preloadConfigs.authMethod) { |
There was a problem hiding this comment.
We can make the response 401 for all auth methods.
There was a problem hiding this comment.
I didn't want to change the API, much better returning 401 for everyone
shared/schema/api/auth.ts
Outdated
| @@ -5,10 +5,32 @@ import {credentialsSchema} from '../Auth'; | |||
|
|
|||
| import type {AuthMethod} from '../Auth'; | |||
|
|
|||
| export const httpBasicAuth = (authorization: string) => { | |||
There was a problem hiding this comment.
No function in this file. The file is for schema only.
There was a problem hiding this comment.
I will move to util/authUtil.ts
shared/schema/api/auth.ts
Outdated
| // All auth requests are schema validated to ensure security. | ||
|
|
||
| // POST /api/auth/authenticate | ||
| export const authAuthenticationSchema = credentialsSchema.pick({username: true, password: true}); | ||
| export const authHTTPBasicAuthenticationSchema = (authorization?: string) => { |
There was a problem hiding this comment.
I will move to util/authUtil.ts
server/routes/api/auth.ts
Outdated
| @@ -103,7 +89,15 @@ router.post<unknown, unknown, AuthAuthenticationOptions>('/authenticate', (req, | |||
| return; | |||
| } | |||
|
|
|||
| const parsedResult = authAuthenticationSchema.safeParse(req.body); | |||
| let parsedResult = authAuthenticationSchema.safeParse(null); | |||
There was a problem hiding this comment.
No need to safe parse and then go through the password authentication procedures and there is no need for schema validation for safe inputs as incoming requests are already authenticated by the web server.
There was a problem hiding this comment.
I still need to compare password. I agree that I don't need to authAuthenticationSchema.safeParse(null) but then I'm not able to understand what type I should declare parsedResult
if no user is registered yet after inserting credentials for the http authentication the registration page would be shown in frontend. you can then register yourself with the same credentials as the http authentication. you can also register someone else but then you won't be able to login yourself. also if you register a user with a different password from the one in the http basic credentials you won't be able to login. |
Looks generally OK to me. However, I don't think the user should have to type the username and password again. Server can send the parsed username along with preload configs. Frontend will then greyed the username and password input out with the username input pre-filled with parsed username. |
oki for me, so we pre-fill pasrsed username, disable both username and password fields, and in the backend endpoint we read username and password from http basic credentials. is it confirmed? |
That's right. A small difference: we don't read or store the password as we don't use it after all. Storing password unnecessarily can increase the data protection liabilities and nullify the benefits of header authentication method. Frontend can fill the password box with bunch of **** to give the impression that there is a password. (user can't edit it anyways) while the backend can either store a consistent or randomly generated string to database. |
sure, I will disabled both user and password but prefill only username from the data returned by the backend. |
I made a patch that hopefully makes the logics cleaner. Please apply if possible.diff --git a/config.cli.js b/config.cli.js
index 9dcb4fb9..23159e19 100644
--- a/config.cli.js
+++ b/config.cli.js
@@ -36,7 +36,8 @@ const {argv} = require('yargs')
})
.option('auth', {
describe: 'Access control and user management method',
- choices: ['default', 'httpbasic', 'none'],
+ default: 'default',
+ choices: ['default', 'header', 'none'],
})
.option('noauth', {
alias: 'n',
@@ -202,11 +203,9 @@ if (argv.rtsocket != null || argv.rthost != null) {
};
}
-let authMethod = 'default';
-if (argv.noauth || argv.auth === 'none') {
+let authMethod = argv.auth;
+if (argv.noauth) {
authMethod = 'none';
-} else {
- authMethod = argv.auth;
}
let allowedPaths = [];
diff --git a/jest.config.js b/jest.config.js
index 2beffb35..d45c494b 100644
--- a/jest.config.js
+++ b/jest.config.js
@@ -4,7 +4,7 @@ module.exports = {
coverageProvider: 'v8',
projects: [
'<rootDir>/server/.jest/auth.config.js',
- '<rootDir>/server/.jest/httpauth.config.js',
+ '<rootDir>/server/.jest/auth.header.config.js',
'<rootDir>/server/.jest/rtorrent.config.js',
// TODO: qBittorrent tests are disabled at the moment.
// '<rootDir>/server/.jest/qbittorrent.config.js',
diff --git a/server/.jest/httpauth.config.js b/server/.jest/auth.header.config.js
similarity index 54%
rename from server/.jest/httpauth.config.js
rename to server/.jest/auth.header.config.js
index 28634b6e..541b1d8f 100644
--- a/server/.jest/httpauth.config.js
+++ b/server/.jest/auth.header.config.js
@@ -1,10 +1,10 @@
module.exports = {
- displayName: 'auth',
+ displayName: 'auth.header',
preset: 'ts-jest/presets/js-with-babel',
rootDir: './../',
testEnvironment: 'node',
- testMatch: ['<rootDir>/routes/api/httpauth.test.ts'],
- setupFilesAfterEnv: ['<rootDir>/.jest/httpauth.setup.js'],
+ testMatch: ['<rootDir>/routes/api/auth.header.test.ts'],
+ setupFilesAfterEnv: ['<rootDir>/.jest/auth.header.setup.js'],
globals: {
'ts-jest': {
isolatedModules: true,
diff --git a/server/.jest/httpauth.setup.js b/server/.jest/auth.header.setup.js
similarity index 92%
rename from server/.jest/httpauth.setup.js
rename to server/.jest/auth.header.setup.js
index 1f27cf81..cb87a672 100644
--- a/server/.jest/httpauth.setup.js
+++ b/server/.jest/auth.header.setup.js
@@ -7,7 +7,7 @@ const temporaryRuntimeDirectory = path.resolve(os.tmpdir(), `flood.test.${crypto
process.argv = ['node', 'flood'];
process.argv.push('--rundir', temporaryRuntimeDirectory);
-process.argv.push('--auth', 'httpbasic');
+process.argv.push('--auth', 'header');
afterAll(() => {
if (process.env.CI !== 'true') {
diff --git a/server/config/passport.ts b/server/config/passport.ts
index 11170f83..7bcc236b 100644
--- a/server/config/passport.ts
+++ b/server/config/passport.ts
@@ -2,19 +2,18 @@ import {Strategy, VerifiedCallback} from 'passport-jwt';
import type {PassportStatic} from 'passport';
import type {Request} from 'express';
-import {infer as zodInfer, ZodError} from 'zod';
import config from '../../config';
-import Users from '../models/Users';
import {Credentials} from '../../shared/schema/Auth';
-import {authAuthenticationSchema} from '../../shared/schema/api/auth';
-import {authHTTPBasicAuthenticationSchema} from '../util/authUtil';
+import {parseAuthorizationHeader} from '../util/authUtil';
+import Users from '../models/Users';
// Setup work and export for the JWT passport strategy.
export default (passport: PassportStatic) => {
const options = {
jwtFromRequest: (req: Request) => {
let token = null;
+
if (req && req.cookies) {
token = req.cookies.jwt;
}
@@ -27,39 +26,20 @@ export default (passport: PassportStatic) => {
passport.use(
new Strategy(options, (req: Request, jwtPayload: Pick<Credentials, 'username'>, callback: VerifiedCallback) => {
- let parsedResult:
- | {
- success: true;
- data: Required<zodInfer<typeof authAuthenticationSchema>>;
- }
- | {
- success: false;
- error: ZodError;
- };
-
- switch (config.authMethod) {
- case 'httpbasic':
- parsedResult = authHTTPBasicAuthenticationSchema(req.header('authorization'));
- if (!parsedResult.success) {
- callback(null, false);
- return;
- }
-
- if (jwtPayload.username !== parsedResult.data.username) {
- callback(null, false);
- return;
- }
- break;
- case 'default':
- default:
- }
-
Users.lookupUser(jwtPayload.username, (err, user) => {
if (err) {
return callback(err, false);
}
if (user) {
+ if (config.authMethod === 'header') {
+ const {username} = parseAuthorizationHeader(req.headers.authorization) || {};
+
+ if (username !== user.username) {
+ return callback(null, false);
+ }
+ }
+
return callback(null, user);
}
diff --git a/server/routes/api/httpauth.test.ts b/server/routes/api/auth.header.test.ts
similarity index 99%
rename from server/routes/api/httpauth.test.ts
rename to server/routes/api/auth.header.test.ts
index 0e56c5c2..af09c68a 100644
--- a/server/routes/api/httpauth.test.ts
+++ b/server/routes/api/auth.header.test.ts
@@ -33,7 +33,7 @@ const testAdminHTTPBasicAuth = `Basic ${Buffer.from(`${testAdminUser.username}:$
'base64',
)}`;
-const testNotExistingHTTPBasicAuth = `Basic ${Buffer.from('notExstingUser:password').toString('base64')}`;
+const testNotExistingHTTPBasicAuth = `Basic ${Buffer.from('notExistingUser:password').toString('base64')}`;
const testNonAdminUser = {
username: crypto.randomBytes(8).toString('hex'),
diff --git a/server/routes/api/auth.test.ts b/server/routes/api/auth.test.ts
index e4959318..c42ab12b 100644
--- a/server/routes/api/auth.test.ts
+++ b/server/routes/api/auth.test.ts
@@ -267,18 +267,6 @@ describe('GET /api/auth/logout', () => {
done();
});
});
-
- it('Logouts without credential', (done) => {
- request
- .get('/api/auth/logout')
- .send()
- .set('Accept', 'application/json')
- .expect(401)
- .end((err, _res) => {
- if (err) done(err);
- done();
- });
- });
});
describe('POST /api/auth/authenticate', () => {
diff --git a/server/routes/api/auth.ts b/server/routes/api/auth.ts
index d9e4f4ab..976fbe86 100644
--- a/server/routes/api/auth.ts
+++ b/server/routes/api/auth.ts
@@ -4,11 +4,8 @@ import rateLimit from 'express-rate-limit';
import type {Response} from 'express';
-import {infer as zodInfer, ZodError} from 'zod';
-
+import {AccessLevel} from '../../../shared/schema/constants/Auth';
import ajaxUtil from '../../util/ajaxUtil';
-import {getAuthToken, authHTTPBasicAuthenticationSchema} from '../../util/authUtil';
-
import {
authAuthenticationSchema,
authRegistrationSchema,
@@ -16,6 +13,7 @@ import {
AuthVerificationPreloadConfigs,
} from '../../../shared/schema/api/auth';
import config from '../../../config';
+import {getAuthToken, parseAuthorizationHeader} from '../../util/authUtil';
import requireAdmin from '../../middleware/requireAdmin';
import services from '../../services';
import Users from '../../models/Users';
@@ -90,25 +88,19 @@ router.post<unknown, unknown, AuthAuthenticationOptions>('/authenticate', (req,
return;
}
- let parsedResult:
- | {
- success: true;
- data: Required<zodInfer<typeof authAuthenticationSchema>>;
- }
- | {
- success: false;
- error: ZodError;
- };
+ if (config.authMethod === 'header') {
+ const {username} = parseAuthorizationHeader(req.headers.authorization) || {};
+ if (username == null) {
+ res.json(403).send();
+ return;
+ }
- switch (preloadConfigs.authMethod) {
- case 'httpbasic':
- parsedResult = authHTTPBasicAuthenticationSchema(req.header('authorization'));
- break;
- case 'default':
- default:
- parsedResult = authAuthenticationSchema.safeParse(req.body);
+ sendAuthenticationResponse(res, {username, level: AccessLevel.USER});
+ return;
}
+ const parsedResult = authAuthenticationSchema.safeParse(req.body);
+
if (!parsedResult.success) {
validationError(res, parsedResult.error);
return;
@@ -277,11 +269,10 @@ router.use('/', passport.authenticate('jwt', {session: false}));
* @summary Clears the session cookie
* @tags Auth
* @security User
- * @return {string} 401 - not authenticated or token expired or auth is httpbasic
- * @return {} 200 - success response
+ * @return {string} 401 - success response
*/
router.get('/logout', (_req, res) => {
- res.clearCookie('jwt').status(401).json('Unauthorized').send();
+ res.clearCookie('jwt').status(401).json('Logged out').send();
});
// All subsequent routes need administrator access.
diff --git a/server/util/authUtil.ts b/server/util/authUtil.ts
index 05072cf7..01e68a9c 100644
--- a/server/util/authUtil.ts
+++ b/server/util/authUtil.ts
@@ -1,16 +1,31 @@
import {Response} from 'express';
import jwt from 'jsonwebtoken';
+
+import type {AuthorizationHeader} from '@shared/schema/Auth';
+
import config from '../../config';
-import {authAuthenticationSchema} from '../../shared/schema/api/auth';
-export const httpBasicAuth = (authorization: string) => {
- const base64 = authorization.replace(/basic /i, '');
- const credentials = Buffer.from(base64, 'base64').toString().split(':');
- if (credentials.length !== 2 || credentials[0].length === 0 || credentials[1].length === 0) {
+export const parseAuthorizationHeader = (header?: string): AuthorizationHeader | null => {
+ if (header == null) {
+ return null;
+ }
+
+ const [type, value] = header.split(' ');
+
+ if (type !== 'Basic') {
return null;
}
- return credentials;
+ try {
+ const [username] = Buffer.from(value, 'base64').toString().split(':');
+
+ return {
+ type,
+ username,
+ };
+ } catch (e) {
+ return null;
+ }
};
export const getAuthToken = (username: string, res?: Response): string => {
@@ -28,16 +43,3 @@ export const getAuthToken = (username: string, res?: Response): string => {
return token;
};
-
-export const authHTTPBasicAuthenticationSchema = (authorization?: string) => {
- if (authorization === undefined) {
- return authAuthenticationSchema.safeParse({});
- }
-
- const credentials = httpBasicAuth(authorization);
- if (credentials === null) {
- return authAuthenticationSchema.safeParse({});
- }
-
- return authAuthenticationSchema.safeParse({username: credentials[0], password: credentials[1]});
-};
diff --git a/shared/schema/Auth.ts b/shared/schema/Auth.ts
index c3004789..19a9f9b0 100644
--- a/shared/schema/Auth.ts
+++ b/shared/schema/Auth.ts
@@ -4,7 +4,14 @@ import type {infer as zodInfer} from 'zod';
import {AccessLevel} from './constants/Auth';
import {clientConnectionSettingsSchema} from './ClientConnectionSettings';
-export type AuthMethod = 'default' | 'httpbasic' | 'none';
+export type AuthMethod = 'default' | 'header' | 'none';
+
+interface BasicAuthorizationHeader {
+ type: 'Basic';
+ username: string;
+}
+
+export type AuthorizationHeader = BasicAuthorizationHeader;
export const credentialsSchema = object({
username: string(), |
I applied the patch and as far as I can see it's changing the behaviour of my code. one example for now the user level is hardcoded to I will keep your |
Password is not used and it is guaranteed to fail to validate password. I assume you want to verify that the user exists. You don't have to. It is handled by /verify and once frontend knows that current user is not registered, it won't call /authenticate endpoint. Plus, cookie validation won't succeed with a non-existing user anyways. User level is another design problem that has to be addressed. I think no user should have the privilege to create users in this method as created users won't work if there is no corresponding one in the web server. |
I would rather enforce that the full credentials between http basic and flood db are matching.
Since we cannot create in the flood backend a corresponding user in the web server this step will be always "asynchronous" from the creation of the user in flood. It's part of the user creation process in flood when header authentication is enabled. Also creating a user in flood it is not only about the credententials, but also for client settings. I will push another commit and let me know what do you think. |
|
uhm, I'd still need to change |
|
I don't understand why test are failing when running the whole suite, and not the single one |
It simply doesn't make sense to validate password as the user is supposed to be already authenticated when the request reaches Flood. It also creates unnecessary data protection liability (one of the major benefits of this method is that Flood doesn't handle or store the password) and will certainly cause issues when the password of user changes in the web server. For the user creation, per the design, if the user is not configured, the registration screen will show and let the user fill out the connection settings. There is no point to allow other users to create users which won't work anyways if the user is not authenticated by the web server. |
It's authenticated in the webserver, not in flood. It's not supposed to be authenticated in flood. Flood should ensure its authentication process on the data sent on the header, and not completely delegate to them the authentication process.
It's the whole point to allow other users to create users. They will work as soon as the user will be created also in the webserver. If the admin cannot create users then it's a single user setup. I personally have multiple users and I will use the header authentication so I know exactly what are the customer needs. |
The web server enforces the authentication process. Flood does not. Flood only checks the header and gets the username from it. We don't bypass the cookie because Why should Flood validate the password if Flood doesn't store it? The validation process will fail.
I advise that you either stipulate that only the initial user is administrator and allows it to programmatically create users via API. Or define a special username that be treated as administrator. ( Other users shall not be treated as administrators. You don't want all users to have the authority to create or delete other users. |
I made you already an example where the webserver authentication could not match the flood credentials.
So, I think it is now time for you to tell me if you are open to accept contributions according to real users' needs or if you want to drive the contributions according to your agend regardless what're the needs of the users. All comments and request for changes about code style, technical solution etc I'm happy to abide. |
|
It might not match the credentials but it is not because of the password differences. But because the potential re-authentication may change the logged in user and thus the username in the header may not match the username in the cookie. Flood simply have no need to store the password if it doesn’t authenticate. It should merely assign the token of authenticated user based on its username. The web server does the authentication not the Flood. I would conclude that there is absolutely no benefit if such authentication method requires storing of the password and duplicate authentication. |
that's it. the web server does not do any authentication: that's the business requirements given by the customers' need.
the benefit are avoiding all workarounds and hacks that your implementation requires like the following:
I will repeat again to be clear:
so, the questions:
|
Then who will perform the authentication? Flood? Why not simply use Flood's login/register directly then?
HTTP basic authentication workflow is more than just a header. If you want Flood to authenticate with HTTP basic auth, you have to implement the necessary functions.
Like websites that has "Google" / "Facebook" logins, if there is a new user, Flood might display a "register" page (with username/password fields greyed out) that allows user to set connection settings (similar to "setting up your profile" steps). Alternatively, an administrator may purely use Flood's API and a built-in management user to add/delete/modify users in Flood's database while users are authenticated by a web server. In this case, Flood matches the profile via username in the header and does not try to match the password (as the user is already authenticated when the request reaches Flood). |
because of seamless user experience: flood is behind a proxy that requires http based authentication in order to serve the content. I don't want the users to be prompted twice for the same credentials. while the users management should be kept fully featured in flood regardless of how the credentials were provided
as I said before, and I guess it's where there is the misunderstanding about the feature, is that the the whole http basic authentication workflow should not be involved in the flood authentication process: it's just a matter of forwarding the credentials from the http basic authentication workflow to flood and let flood use those credentials for its own process. the only concern I can see about this model is that other "pre-emptive" authentication methods (let's call them so: I intend authentication method that are required to access the flood content, without setting the authentication state in flood itself) may not provide a set of credentials that once forwarded to flood will allow a login based authentication there.
That's exaclty what I've implemented: https://github.com/jesec/flood/pull/70/files#diff-331447558aa67509abffb409e0b90f2cb6428c66ea602f71084033a4dc67caa1R161-R164 It's true only when there's no user registered yet, since this is exactly the way flood works.
this is still a poor UX experience, while limited to admin users.
I repeat it again :) |
If the user is already authenticated, there is no reason to authenticate them again. Simply extract the username and match the correct profile/services. There is absolutely no reason to introduce unnecessary data liabilities and potential inconsistencies (suppose the password is changed on one side only) by storing and matching the password. |
the user is authenticated to the proxy, not to flood. |
Yes. The proxy already authenticated the incoming request. By the time the request reaches Flood, the user is an authenticated user. There is no reason to authenticate again. |
the request is authenticated in the proxy domain. my question is (I mispelled while for why): |
Because it supposes to. You want to avoid duplicate authentication, that's your goal, no? OK. Suppose we keep the password authentication there, what will happen when there are different passwords in Flood's database and your proxy's database? |
my goal is not to avoid duplicate authentication: my goal is avoiding poor UX experience that requires providing two times the credentials. so, it doesn't suppose to, giving my goal. at the contrary, I strongly believe that the two services involved have far different domain and the authentication process of both should be separated and processed accordingly, only sharing the bit of information the can be applied to both (in this case: username and password) but I'm open to try to understand why it should suppose to from your point of view.
flood won't authenticate. exactly because proxy and flood are two different domains. it's up to the maintainer of the system to keep the credentials in sync. I don't see the feature as delegating authentication/credentials storage to an external source. in the future I could remove the proxy, or I could switch to other mechanism in order to access flood resources and I should still be able to authenticate through flood because it owns his own users management/authentication process. |
Well. There is virtually zero benefit to have such mechanism. Current implementation also greatly increases complexity around the auth codes and confuses users. If you only want something like login with HTTP header, I would advise you to drastically scale down this change. |
I remember that my first implementation was to prefill login and registration form (and autosubmit the first) If you have any other way to scale down this change different from the original frontend implementation please share :) |
|
@jesec any feedback on this? |
|
I am not convinced that this PR is useful. I also have concerns about the increased complexity. |
we are talking about a completely different implementation, reducing the complexity, as you said in your last comment. my question was if the original implementation in frontend (that you asked to change for an implementation in backend) would be low complexity enough as for the usefulness of the feature: it answers to a customer need. |
You can update the PR to show such implementation. Still, I think it is a bad idea to mix two distinct authentication system. Ideally, only one component should do the authentication. |
I tried to recover the squashed commit that I discarded back then. Pushed now. Probably not working, but just to give an idea.
that's exactly my point and what I did initially: just forwarding/prefilling the credentials from http basic auth to the backend and keep the authentication only in flood. it was you that asked for a different solution |
|
@jesec any feedback? |
|
It is still very complicated, and I don't think the potential benefits worth it. |
then instead of letting me rewrite the PR four times you could have honestly answered, when I asked, that you are not open to accept contributions according to real users' needs and that you want to drive the contributions according to your agenda regardless what're the needs of the users I'm honestly worried that you took over rtorrent development as well have fun with your toys |
Add method
httpbasicto authDescription
The feautre is enabled in case the
authcli arg is set tohttpbasicIn such case
jwtFromRequestinpassport.tswill extract the username from the http basic credentials and compare with the one from the jwt token. if there is a match the process continues as beforeThe
/api/auth/authenticatewill read the credentials from http basic auth instead of body request and continue with the same flow.The
/api/auth/logoutwill send a401 Unathorizedresponse.Related Issue
Motivation and Context
I use flood behind a nginx proxy with http basic auth and I want to use this credentials for logging in in flood for a smoother experience.
How Has This Been Tested?
Tested on a server with multiple users (admin and not) with flood setup behind nginx proxy
Screenshots (if appropriate):
Types of changes
Checklist: