Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

'/' was resulting in '//' in change_url #368

Merged
merged 1 commit into from
Sep 15, 2017
Merged

'/' was resulting in '//' in change_url #368

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/yunohost/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,8 +445,9 @@ def app_change_url(auth, app, domain, path):

# Normalize path and domain format
domain = domain.strip().lower()
old_path = '/' + old_path.strip("/").strip() + '/'
path = '/' + path.strip("/").strip() + '/'

old_path = normalize_url_path(old_path)
path = normalize_url_path(path)

if (domain, path) == (old_domain, old_path):
raise MoulinetteError(errno.EINVAL, m18n.n("app_change_url_identical_domains", domain=domain, path=path))
Expand Down Expand Up @@ -2105,3 +2106,10 @@ def random_password(length=8):

char_set = string.ascii_uppercase + string.digits + string.ascii_lowercase
return ''.join([random.SystemRandom().choice(char_set) for x in range(length)])


def normalize_url_path(url_path):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

probably nitpicking, but this helper could be in a file within the https://github.com/YunoHost/yunohost/tree/unstable/src/yunohost/utils folder ? (maybe out of scope of this PR)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably. It would be ever greater in moulinette utils I think but that's mostly a "quick fix" PR to unblock the app group situation. I don't mind if we do those kind of modifications (as long as it don't block this PR for the next release) but I'm not super available for that now :x

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like this one ? 😉

yunohost/data/helpers.d/network

Lines 1 to 23 in 3571747

# Normalize the url path syntax
# Handle the slash at the beginning of path and its absence at ending
# Return a normalized url path
#
# example: url_path=$(ynh_normalize_url_path $url_path)
# ynh_normalize_url_path example -> /example
# ynh_normalize_url_path /example -> /example
# ynh_normalize_url_path /example/ -> /example
# ynh_normalize_url_path / -> /
#
# usage: ynh_normalize_url_path path_to_normalize
# | arg: url_path_to_normalize - URL path to normalize before using it
ynh_normalize_url_path () {
path_url=$1
test -n "$path_url" || ynh_die "ynh_normalize_url_path expect a URL path as first argument and received nothing."
if [ "${path_url:0:1}" != "/" ]; then # If the first character is not a /
path_url="/$path_url" # Add / at begin of path variable
fi
if [ "${path_url:${#path_url}-1}" == "/" ] && [ ${#path_url} -gt 1 ]; then # If the last character is a / and that not the only character.
path_url="${path_url:0:${#path_url}-1}" # Delete the last character
fi
echo $path_url
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't great to call that from python :x

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah nvm I was thinking @opi wanted to make this available for apps

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, but not scoped in app.py ; But let's merge it now, and we'll cleanup later.

if url_path.strip("/").strip():
return '/' + url_path.strip("/").strip() + '/'

return "/"