BioplatformsAustralia / bpaotu

OTU database access for the Australian Microbiome
GNU Affero General Public License v3.0
5 stars 1 forks source link

Authentication bug #111

Closed grahame closed 5 years ago

grahame commented 5 years ago

We don't properly handle the case of a user who has access to the Bioplatforms Data Portal, but does not have access to the Australian MIcrobiome.

The user doesn't receive a "log in" notification, instead they see "unhandled server-side error" messages.

I'm not sure of the fix here; given that we grant access to the Australian Microbiome automatically on application, perhaps we should just tell CKAN to add them to the group, and continue through?

What do you both think @sztamas and @ccgdev004?

sztamas commented 5 years ago

I am a bit confused by the possible users of the system.

We currently have a flag for CKAN_AUTH_INTEGRATION that's coming from settings.py. We check that the user is logged in to CKAN only if that flag is True. However, most of our views are wrapped with checks for user being logged in to CKAN and having access to Australian Microbiome, so I don't see how the app is currently usable if CKAN_AUTH_INTEGRATION is False.

The first question is whether we have to support installations without CKAN_AUTH_INTEGRATION? If we do then we have to look at how the UI would change without it?

I've just added a new page for the case when the user logged in to CKAN but they don't have access to Australian Microbiome data. I'm holding back a bit on pushing it until we decide whether we have to support the app without CKAN_AUTH_INTEGRATION.

"

Australian Microbiome Data Access Required

You do not have access to the Australian Microbiome data.

Please contact support. "

grahame commented 5 years ago

Hey Tamas

We don’t use the auth integration in local dev - we wouldn’t want to break that would we?

Grahame

On Thu, 13 Dec 2018 at 10:40 am, Tamas Szabo notifications@github.com wrote:

I am a bit confused by the possible users of the system.

We currently have a flag for CKAN_AUTH_INTEGRATION that's coming from settings.py. We check that the user is logged in to CKAN only if that flag is True. However, most of our views are wrapped with checks for user being logged in to CKAN and having access to Australian Microbiome, so I don't see how the app is currently usable if CKAN_AUTH_INTEGRATION is False.

The first question is whether we have to support installations without CKAN_AUTH_INTEGRATION? If we do then we have to look at how the UI would change without it?

I've just added a new page for the case when the user logged in to CKAN but they don't have access to Australian Microbiome data. I'm holding back a bit on pushing it until we decide whether we have to support the app without CKAN_AUTH_INTEGRATION.

" Australian Microbiome Data Access Required

You do not have access to the Australian Microbiome data.

Please contact support. "

— You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub https://github.com/BioplatformsAustralia/bpaotu/issues/111#issuecomment-446822081, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUMNTpmAbhCp1_3qLCwu2H4oMNCSMp6ks5u4b4tgaJpZM4ZMsRX .

-- Grahame Bowland Centre for Comparative Genomics Murdoch University 0433 317 142

sztamas commented 5 years ago

The way dev currently works is that we have a "dev only" fake view that acts as the real ckan auth view.

https://github.com/BioplatformsAustralia/bpaotu/blob/6ec949b216e62532a1565374aec802a50d915ed8/bpaotu/bpaotu/views.py#L611

So dev runs with CKAN_AUTH_INTEGRATION just as prod does. The only difference is that it doesn't call CKAN for auth it just uses that view with pre-baked values.

BTW, I used that view to set organisations to [] to reproduce the bug you've reported, without having to set up CKAN, set up a user that isn't in the australian-microbiome group etc.

sztamas commented 5 years ago

If the only reason for the CKAN_AUTH_INTEGRATION was to use it in dev I vote for dropping it altogether. We would have less branching in the code and less special conditions in dev versus prod.

grahame commented 5 years ago

Sounds good - let’s drop it.

On Thu, 13 Dec 2018 at 11:13 am, Tamas Szabo notifications@github.com wrote:

If the only reason for the CKAN_AUTH_INTEGRATION was to use it in dev I vote for dropping it altogether. We would have less branching in the code and less special conditions in dev versus prod.

— You are receiving this because you authored the thread.

Reply to this email directly, view it on GitHub https://github.com/BioplatformsAustralia/bpaotu/issues/111#issuecomment-446827961, or mute the thread https://github.com/notifications/unsubscribe-auth/AAUMNXnwoJg0YvGrzfXqsix4HUB-u1g_ks5u4cXtgaJpZM4ZMsRX .

-- Grahame Bowland Centre for Comparative Genomics Murdoch University 0433 317 142