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

Admin helpers that reference the login_page setting should be aware of the app's mounted path #1493

Closed
phobetron opened this Issue Nov 20, 2013 · 10 comments

Comments

Projects
None yet
5 participants
@phobetron

phobetron commented Nov 20, 2013

It seems that the path applied to login_page is being treated as absolute. The Admin's authentication helpers may redirect to invalid paths when an Admin app (or an app which makes use of Admin helpers) is mounted to a different path than expected. This is more of a problem when the app is loaded as a gem, because it negates the gem user's flexibility in choosing a path appropriate to their app. It would be nice if the path were treated similarly to url_for, which seems to be aware of the app's mounted path.

I've been looking for decent work-arounds to keep me going until this is fixed, so any suggestions would be appreciated.

@phobetron

This comment has been minimized.

Show comment
Hide comment
@phobetron

phobetron Nov 20, 2013

My current workaround is to set login_page within the controller that manages login, and use url_for to generate it. This placement is due to the router not yet having processed controller paths when app.rb is loaded.

Example:

Admin.controllers :login do
  get :new do
    ...
  end
  ...
  Admin.set :login_page, url_for(:login, :new)
end

Kludgy, but it seems to work, for now.

phobetron commented Nov 20, 2013

My current workaround is to set login_page within the controller that manages login, and use url_for to generate it. This placement is due to the router not yet having processed controller paths when app.rb is loaded.

Example:

Admin.controllers :login do
  get :new do
    ...
  end
  ...
  Admin.set :login_page, url_for(:login, :new)
end

Kludgy, but it seems to work, for now.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Nov 20, 2013

Contributor

Hey @phobetron, yeah, I'm aware of that issue and I know that there's some patch needed to fix that too. I need to get back to it soon. @angelbotto would you know when (~) we had that IRC chat on a similar topic? Thanks :)

Contributor

dariocravero commented Nov 20, 2013

Hey @phobetron, yeah, I'm aware of that issue and I know that there's some patch needed to fix that too. I need to get back to it soon. @angelbotto would you know when (~) we had that IRC chat on a similar topic? Thanks :)

@ghost ghost assigned dariocravero Nov 20, 2013

@angelbotto

This comment has been minimized.

Show comment
Hide comment
@angelbotto

angelbotto Nov 20, 2013

hi @phobetron check this. in admin folder the app.rb change this line.

set :login_page, '/admin/sessions/new'

for

set :login_page, '/sessions/new'

and add this

set :public_folder, Padrino.root('public', 'admin')

this example working for this url

admin.MYDOMAIN.com

👍

sorry my english :)

angelbotto commented Nov 20, 2013

hi @phobetron check this. in admin folder the app.rb change this line.

set :login_page, '/admin/sessions/new'

for

set :login_page, '/sessions/new'

and add this

set :public_folder, Padrino.root('public', 'admin')

this example working for this url

admin.MYDOMAIN.com

👍

sorry my english :)

@phobetron

This comment has been minimized.

Show comment
Hide comment
@phobetron

phobetron Nov 20, 2013

@angelbotto that doesn't seem to have anything to do with the issue I've opened.

phobetron commented Nov 20, 2013

@angelbotto that doesn't seem to have anything to do with the issue I've opened.

@angelbotto

This comment has been minimized.

Show comment
Hide comment
@angelbotto

angelbotto Nov 20, 2013

If your problem relates to the routes the administrator when you change your place of normal location.

test as indicated, may be solved :)

angelbotto commented Nov 20, 2013

If your problem relates to the routes the administrator when you change your place of normal location.

test as indicated, may be solved :)

@phobetron

This comment has been minimized.

Show comment
Hide comment
@phobetron

phobetron Nov 20, 2013

No, @angelbotto, that doesn't solve it. Referencing the the mounted path anywhere in the app.rb is not acceptable. That is the core of this issue. This is an actual bug in the padrino-admin code that needs to be addressed.

Besides, plugging it into the public folder path seems hackish. The workaround I posted works under the aforementioned requirement, and it doesn't exploit any padrino quirks.

phobetron commented Nov 20, 2013

No, @angelbotto, that doesn't solve it. Referencing the the mounted path anywhere in the app.rb is not acceptable. That is the core of this issue. This is an actual bug in the padrino-admin code that needs to be addressed.

Besides, plugging it into the public folder path seems hackish. The workaround I posted works under the aforementioned requirement, and it doesn't exploit any padrino quirks.

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Nov 20, 2013

Contributor

Thanks @angelbotto for your comments, would you remember at around when we has that chat? What I'm after is the IRC log :).

@phobetron you're right, that is a hack and it's a temporary solution only, that's why I mentioned above that we need to fix that on the framework itself.

However, @angelbotto's recommendation should get you going for now as your workaround does.

I'll look at this in more depth as soon as I can but I don't think I'll have time before mid December if anything.
Thanks for understanding and for bringing up this issue :)

Contributor

dariocravero commented Nov 20, 2013

Thanks @angelbotto for your comments, would you remember at around when we has that chat? What I'm after is the IRC log :).

@phobetron you're right, that is a hack and it's a temporary solution only, that's why I mentioned above that we need to fix that on the framework itself.

However, @angelbotto's recommendation should get you going for now as your workaround does.

I'll look at this in more depth as soon as I can but I don't think I'll have time before mid December if anything.
Thanks for understanding and for bringing up this issue :)

@ujifgc ujifgc closed this in 6d51246 Jan 3, 2014

@ghost ghost assigned ujifgc Jan 3, 2014

Ortuna added a commit to Ortuna/padrino-framework that referenced this issue Jan 17, 2014

@leucos

This comment has been minimized.

Show comment
Hide comment
@leucos

leucos Apr 5, 2014

Is there any way to "get back" the old behaviour ?
Use case : you have multiples apps (let's say the admin one and your main app) and want an unified login page in the main app (e.g. http://host/login/).

leucos commented Apr 5, 2014

Is there any way to "get back" the old behaviour ?
Use case : you have multiples apps (let's say the admin one and your main app) and want an unified login page in the main app (e.g. http://host/login/).

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Apr 6, 2014

Member

@leucos you can define this method

def access_denied
if login_page.present?
redirect url(login_page)
else
halt 401, "You don't have permission for this resource"
end
end
in your helpers:

Your::Admin.helpers do
  def access_denied
    if login_page.present?
      #redirect url(login_page)
      redirect login_page
    else
      halt 401, "You don't have permission for this resource"
    end
  end
end

Or you can monkey-patch it.

Member

ujifgc commented Apr 6, 2014

@leucos you can define this method

def access_denied
if login_page.present?
redirect url(login_page)
else
halt 401, "You don't have permission for this resource"
end
end
in your helpers:

Your::Admin.helpers do
  def access_denied
    if login_page.present?
      #redirect url(login_page)
      redirect login_page
    else
      halt 401, "You don't have permission for this resource"
    end
  end
end

Or you can monkey-patch it.

@leucos

This comment has been minimized.

Show comment
Hide comment
@leucos

leucos Apr 6, 2014

Works like a charm. Thanks Igor @ujifgc !

leucos commented Apr 6, 2014

Works like a charm. Thanks Igor @ujifgc !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment