Skip to content

Commit

Permalink
UI: Make sure URL's are in correct shape
Browse files Browse the repository at this point in the history
Fixes: https://github.com/aquarist-labs/s3gw/issues/849

Signed-off-by: Volker Theile <vtheile@suse.com>
  • Loading branch information
votdev committed Nov 30, 2023
1 parent 02712b5 commit f2c60c8
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 33 deletions.
25 changes: 12 additions & 13 deletions src/backend/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,34 +112,33 @@ def post_process(key: str, value: str | None) -> str:
return url


def get_ui_path() -> str:
def get_ui_path(default: str = "/") -> str:
"""
Obtain the path under which the UI should be served, e.g. `/ui`.
"""

def post_process(key: str, value: str | None) -> str:
if value is None or value == "/":
return "/"
return default
# TODO: The path should be validated here
match = re.fullmatch(r".*", value)
if match is None:
logger.error(
f"The value of the environment variable {key} is malformed: {value}" # noqa: E501
)
raise EnvironMalformedError(key)
return value if value.startswith("/") else f"/{value}"
return f"/{value.strip('/')}"

path = get_environ_str("S3GW_UI_PATH", cb=post_process)
return path
return get_environ_str("S3GW_UI_PATH", cb=post_process)


def get_api_path(ui_path: str) -> str:
"""
Obtain the path under which the API for the UI should be served from the
path of the UI itself. E.g. when the UI path is `/ui`, this will be
`/ui/api`
"""
return f"{ui_path.rstrip('/')}/api"
def get_api_path(default: str = "/api") -> str:
def post_process(_: str, value: str | None) -> str:
if value is None or value == "":
value = default
return f"/{value.strip('/')}"

return get_environ_str("S3GW_API_PATH", cb=post_process)


class Config:
Expand All @@ -164,7 +163,7 @@ def __init__(self) -> None:
"S3GW_S3_PREFIX_DELIMITER", "/"
)
self._ui_path = get_ui_path()
self._api_path = get_api_path(self._ui_path)
self._api_path = get_api_path()
self._instance_id = get_environ_str("S3GW_INSTANCE_ID")

@property
Expand Down
39 changes: 25 additions & 14 deletions src/backend/tests/unit/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
Config,
EnvironMalformedError,
S3AddressingStyle,
get_api_path,
get_environ_enum,
get_environ_str,
get_s3gw_address,
Expand Down Expand Up @@ -130,12 +131,12 @@ def test_good_ui_path_2() -> None:

def test_good_ui_path_3() -> None:
os.environ["S3GW_UI_PATH"] = "/foo-bar/baz/"
assert "/foo-bar/baz/" == get_ui_path()
assert "/foo-bar/baz" == get_ui_path()


def test_good_ui_path_4() -> None:
os.environ["S3GW_UI_PATH"] = "/foo-bar/foo-bar/"
assert "/foo-bar/foo-bar/" == get_ui_path()
os.environ["S3GW_UI_PATH"] = "foo-bar/foo-bar"
assert "/foo-bar/foo-bar" == get_ui_path()


def test_no_ui_path() -> None:
Expand All @@ -152,17 +153,7 @@ def test_good_ui_api_path() -> None:
os.environ["S3GW_UI_PATH"] = path
try:
cfg = Config()
assert cfg.api_path == "/s3store/api"
except Exception as e:
pytest.fail(str(e))


def test_api_path_with_trailing_slash() -> None:
path = "/s3store/"
os.environ["S3GW_UI_PATH"] = path
try:
cfg = Config()
assert cfg.api_path == "/s3store/api"
assert cfg.api_path == "/api"
except Exception as e:
pytest.fail(str(e))

Expand Down Expand Up @@ -201,3 +192,23 @@ def test_get_environ_str_2() -> None:
def test_get_environ_str_3() -> None:
os.environ.pop("S3GW_INSTANCE_ID", None)
assert "" == get_environ_str("S3GW_INSTANCE_ID")


def test_api_path_1() -> None:
os.environ["S3GW_API_PATH"] = "/bar"
assert "/bar" == get_api_path()


def test_api_path_2() -> None:
os.environ["S3GW_API_PATH"] = "foo/bar/"
assert "/foo/bar" == get_api_path()


def test_api_path_3() -> None:
os.environ.pop("S3GW_API_PATH", None)
assert "/api" == get_api_path()


def test_api_path_4() -> None:
os.environ["S3GW_API_PATH"] = ""
assert "/api" == get_api_path()
12 changes: 12 additions & 0 deletions src/frontend/src/app/shared/services/api/s3gw-api.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ describe('S3gwApiService', () => {
expect(service.buildUrl('/foo/bar/')).toBe('http://localhost:8080/api/foo/bar/');
});

it('should build valid URL [2]', () => {
service.config.ApiPath = 'api';
// @ts-ignore
expect(service.buildUrl('/foo/bar/')).toBe('api/foo/bar/');
});

it('should build valid URL [3]', () => {
service.config.ApiPath = 'aaa/api';
// @ts-ignore
expect(service.buildUrl('xyz')).toBe('aaa/api/xyz');
});

it('should call bucket list', () => {
service.config.ApiPath = 'https://localhost:8080/api';
service.get('buckets/', { credentials: { accessKey: 'foo', secretKey: 'bar' } }).subscribe();
Expand Down
5 changes: 1 addition & 4 deletions src/frontend/src/app/shared/services/api/s3gw-api.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,7 @@ export class S3gwApiService {
}

private buildUrl(url: string): string {
return `${_.trimEnd(this.config.ApiPath, this.config.Delimiter)}/${_.trimStart(
url,
this.config.Delimiter
)}`;
return `${this.config.ApiPath}/${_.trimStart(url, '/')}`;
}

private buildHeaders(credentials: Credentials): HttpHeaders {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { HttpClient } from '@angular/common/http';
import { Injectable } from '@angular/core';
import * as _ from 'lodash';
import { EMPTY, Observable } from 'rxjs';
import { catchError, tap } from 'rxjs/operators';

Expand All @@ -18,7 +19,7 @@ export type AppMainConfig = {
export class AppMainConfigService {
private _config: AppMainConfig = {
/* eslint-disable @typescript-eslint/naming-convention */
ApiPath: 'api/',
ApiPath: 'api',
Delimiter: '/',
Endpoint: '',
InstanceId: ''
Expand Down Expand Up @@ -49,6 +50,9 @@ export class AppMainConfigService {
return EMPTY;
}),
tap((config: AppMainConfig) => {
// Make sure the `ApiPath` is in a valid form, so it can be
// used without worries and adjustments.
config.ApiPath = _.trim(config.ApiPath, '/');
// eslint-disable-next-line no-underscore-dangle
this._config = config;
})
Expand Down
6 changes: 5 additions & 1 deletion src/s3gw_ui_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,11 @@ def s3gw_factory(static_dir: str | None = None) -> FastAPI:
s3gw_api.include_router(objects.router)
s3gw_api.include_router(config.router)

s3gw_app.mount(s3gw_api.state.config.api_path, s3gw_api, name="api")
s3gw_app.mount(
f"{s3gw_api.state.config.ui_path}{s3gw_api.state.config.api_path.lstrip('/')}",
s3gw_api,
name="api",
)

if static_dir is not None:
# Disable caching of `index.html` on purpose so that the browser
Expand Down

0 comments on commit f2c60c8

Please sign in to comment.