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.

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

Added ROUTE::OPTIONAL_SECURED #1196

Closed
wants to merge 6 commits into from

Conversation

Projects
None yet
9 participants
@vjirovsky
Copy link
Contributor

commented Aug 5, 2013

This option for route allows to access route via both HTTP&HTTPS without
redirection.
Href URLs will be generated in currently used protocol.

Added ROUTE::OPTIONAL_SECURED
This option for route allows to access route via both HTTP&HTTPS without
redirection.
Href URLs will be generated in currently used protocol.
@@ -109,6 +109,8 @@ class Route extends Nette\Object implements Application\IRouter
/** @var int */
private $flags;
/** @var bool */
private $httpRequestSecured = false;

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Aug 5, 2013

Contributor

Wrong coding style, should be in upper case.

This comment has been minimized.

Copy link
@rostenkowski

rostenkowski Aug 5, 2013

Contributor

...FALSE should be in uppercase for clarification

@@ -402,7 +406,7 @@ public function constructUrl(Application\Request $appRequest, Nette\Http\Url $re
return NULL; // TODO: implement counterpart in match() ?
}
$url = ($this->flags & self::SECURED ? 'https:' : 'http:') . $url;
$url = ((($this->flags & self::SECURED) || (($this->flags & self::OPTIONAL_SECURED) && $this->httpRequestSecured)) ? 'https:' : 'http:') . $url;

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Aug 5, 2013

Contributor

So this wouldn't work correctly if match() isn't called? It seems to be wrong. Also I think brackets around bitwise operations are unnecessary.

This comment has been minimized.

Copy link
@vjirovsky

vjirovsky Aug 5, 2013

Author Contributor

Do you know about case when this would be used and match() isn't called? And brackets are for visual diff of conditions

This comment has been minimized.

Copy link
@Majkl578

Majkl578 Aug 5, 2013

Contributor

Imagine you have 5 routes. You open a page which is matched by first route. Other four won't have match() called. Then, in your template, you want to generate a link to any of those four. :)

@Majkl578

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2013

👍 for the idea, 👎 for the implementation.

I currently do the following in my routes factory to achieve this behavior:

if ($request->isSecured()) {
    Route::$defaultFlags |= Route::SECURED;
}
@vjirovsky

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2013

I think this implementation is worse, it has unwanted behavior - it will set to every route this property. In my implementation you can select which routes have this property.

@@ -27,6 +27,9 @@
/** HTTPS route */
const SECURED = 2;
/** HTTP & HTTPS route, depends if secured http request */
const OPTIONAL_SECURED = 4;

This comment has been minimized.

Copy link
@milo

milo Aug 5, 2013

Member

OPTIONALLY_SECURED is gramatically right

This comment has been minimized.

Copy link
@vjirovsky

vjirovsky Aug 5, 2013

Author Contributor

i will commit grammatical fix later

@milo

This comment has been minimized.

Copy link
Member

commented Aug 5, 2013

I like the idea too. I'm using it often in private applications. @Majkl578 solution is often used as:

$optionallySecured = $request->isSecured() ? Route::SECURED : 0;

new Route(..., ..., Route::FLAG | $optionallySecured);
@Majkl578

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2013

I think this implementation is worse, it has unwanted behavior - it will set to every route this property.

You can't generalize it like this. It may or may not be an intended behavior. Since I make apps using Facebook, it's intended. Otherwise I use more-or-less what @milo mentioned:

new Route(..., ..., Route::$defaultFlags | $optionallySecured);
@vjirovsky

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2013

You can't generalize it like this. It may or may not be an intended behavior.

It may be inteded behavior (depends on project), but as default I would like to keep existing state.
In fact this behavior is BC - because if somebody will have routes in this order:

  1. HTTP route index.php ("Go over https!")
  2. HTTPS route index.php ("Hi man!")
    => first route will match even https version of index.php

Solution of @milo is fine, but I think is less userfriendly for beginners (why to explain bit OR to beginner during routing).

@Majkl578

This comment has been minimized.

Copy link
Contributor

commented on f2d7d8d Mar 13, 2014

Caching iterator may be useful also outside Latte... :/

This comment has been minimized.

Copy link
Contributor

replied Mar 13, 2014

I use it a lot, there should be alias or something fixing BC.

This comment has been minimized.

Copy link
Member Author

replied Mar 13, 2014

Yes, it can, maybe little bit smaller variant, without "grid width".

@dg dg force-pushed the nette:master branch 3 times, most recently from 991ba1a to e23de7a Aug 26, 2014

@dg dg force-pushed the nette:master branch from 872d693 to 1467a8d Jan 31, 2015

@@ -251,13 +253,15 @@ public function match(Nette\Http\IRequest $httpRequest)
unset($params[self::PRESENTER_KEY]);
}
$this->httpRequestSecured = $httpRequest->isSecured();

This comment has been minimized.

Copy link
@fprochazka

fprochazka Jan 13, 2016

Contributor

Please do not save the state here, it's not neccessary and makes it harder to test router.

@fprochazka

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

@vjirovsky great idea 👍

@f3l1x

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

I use same code as @Majkl578

if ($request->isSecured()) {
    Route::$defaultFlags |= Route::SECURED;
}
@hrach

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

This issue is mostly problematic because developers realize it quite late - after deploying application into a production. Till then, they expect routing will work as same as it ignores domain names. The route doesn't force any protocol, so this behavior should be the default one.

@f3l1x

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

@hrach I force it at Nginx mainly.

@hrach

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

@f3l1x please follow the text, so do I, so does everyone.

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

The route doesn't force any protocol, so this behavior should be the default one.

👍 Can anyone think of some negative impact?

Current default behavior (correct me please if I'm wrong) is to accept (match) both HTTP and HTTPS requests but generate always HTTP unless SECURED flag is set.

Behavior matrix to summarize

Accept Generates Note
HTTP only HTTP now impossible
HTTP and HTTPS HTTP current default behavior
HTTP and HTTPS HTTPS behavior with SECURED flag
HTTP and HTTPS COPY proposed behavior
HTTPS only HTTPS now impossible
@Majkl578

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2016

I'm not sure whether running both HTTP & HTTPS is still an issue nowadays...
Web is moving towards TLS only -- we have HTTP/2 that requires TLS, we also have free certificates provided by Let's Encrypt.

@milo

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

Idea: What about not to update Route, but:
a) add example code to sandbox's bootstrap commented out, or
b) adjust router factory in sandbox to control this functionality

It will warn people that there is something to configure.

@dg

This comment has been minimized.

Copy link
Member

commented Jan 13, 2016

IMHO default behavior can be changed to copy protocol when route mask doesn't contain domain or when generated domain is the same as current domain (+subdomain).

@dg dg closed this May 13, 2016

@JanTvrdik

This comment has been minimized.

Copy link
Contributor

commented May 13, 2016

IMHO default behavior can be changed to copy protocol

Can this be done in Nette 2.4?

dg added a commit to nette/application that referenced this pull request May 19, 2016

dg added a commit to nette/application that referenced this pull request May 20, 2016

dg added a commit to nette/routing that referenced this pull request Feb 3, 2019

dg added a commit to nette/routing that referenced this pull request Feb 3, 2019

dg added a commit to nette/routing that referenced this pull request Feb 3, 2019

dg added a commit to nette/routing that referenced this pull request Feb 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.