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

Fix flower login if used behind a proxy via socket #3187

Merged
merged 1 commit into from Feb 12, 2018

Conversation

@bpedersen2
Copy link
Contributor

@bpedersen2 bpedersen2 commented Dec 14, 2017

The login redirect uri was always using the port, even if flower is served
via a proxy server, e.g. nginx.

Run flower as:

indico celery flower --address=<yourhost>  --url-prefix=flower --unix-socket=/var/run/flower/flower.socket 

example nginx snippet:

server { ....
  location /flower {
    rewrite ^/flower/(.*)$ /$1 break;
    proxy_pass http://unix:/var/run/flower/flower.socket;
    proxy_set_header Host $host;
    proxy_redirect off;
    proxy_http_version 1.1;
    proxy_set_header Upgrade $http_upgrade;
    proxy_set_header Connection "upgrade";
    allow 127.0.0.1;
    deny all;
  }
}
options.port)
redirect_uri = 'http{}://{}{}{}/login'.format('s' if 'ssl_options' in settings else '',
options.address or 'localhost',
':' + options.port if not options.unix_socket else '',

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 14, 2017
Member

The port should only be added if it's not the default (80/443 depending on the protocol)

This comment has been minimized.

@bpedersen2

bpedersen2 Dec 14, 2017
Author Contributor

The patch removes adding the port (which is 5555 by default for flower) if flower is actually listening on a unix socket. What's not handled currently is an upstream server on non-default ports in this case. As the options are handled by flower this is currently out of scope.

This comment has been minimized.

@ThiefMaster

ThiefMaster Dec 14, 2017
Member

Oh right we were already always adding the port before. Nevermind then.

@bpedersen2 bpedersen2 force-pushed the bpedersen2:flowerfix branch from eb1fc66 to 6d2dd62 Dec 15, 2017
@ThiefMaster
Copy link
Member

@ThiefMaster ThiefMaster commented Jan 9, 2018

While this is nice, there is still one problem: If you use SSL, you'd need to use flower's unix socket with SSL too (which doesn't make much sense) because otherwise the redirect_uri will use http.

Maybe there is some way to get the current URL that was used to access the page? This would always work, regardless of what type of socket or protocol is used...

redirect_uri = self.request.full_url() seems to be a good starting point, but the URL built like this lacks the URL prefix - tbh, using flower on anything but the url root (e.g. on a subdomain) feels a bit hacky. Searching for url_prefix in the list of flower issues also yields lots of results of various issues with it.

@bpedersen2
Copy link
Contributor Author

@bpedersen2 bpedersen2 commented Jan 11, 2018

Yes the http/https problem is not fully solved ( In my install it works, as there is a redirect from http to https configured in nginx as well. I think the probably the easiest solution would be adding further settings in indico to override the computed redirect uri ( then passed as enviroment vars in the flower execution)

@ThiefMaster
Copy link
Member

@ThiefMaster ThiefMaster commented Jan 11, 2018

Doesn't even need a new setting - there is already FLOWER_URL which is currently only used to show a link in the admin area but could be used to build the redirect_uri as well

@bpedersen2 bpedersen2 force-pushed the bpedersen2:flowerfix branch from 6d2dd62 to 9576d14 Jan 11, 2018
@ThiefMaster ThiefMaster changed the base branch from v2.1-dev to master Jan 12, 2018
@bpedersen2 bpedersen2 force-pushed the bpedersen2:flowerfix branch from 9576d14 to 74a3366 Jan 29, 2018
@ThiefMaster ThiefMaster force-pushed the bpedersen2:flowerfix branch from 74a3366 to 53c62db Feb 12, 2018
Use the configured flower url as the base for the login redirect uri if
set, else at least take the unix-socket optin into account.
@ThiefMaster ThiefMaster force-pushed the bpedersen2:flowerfix branch from 53c62db to aa5a4a7 Feb 12, 2018
@ThiefMaster ThiefMaster merged commit aa5a4a7 into indico:master Feb 12, 2018
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bpedersen2 bpedersen2 deleted the bpedersen2:flowerfix branch Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.