Enable external auth support and disabling of user/group mgmt #809

chambridge · 2021-08-16T14:14:09Z

Enable development setup for external authentication
Disable user/group management

himdel · 2021-08-16T15:10:41Z

Question, isUserMgmtDisabled seems to be used everywhere where there's a check for user.model_permissions.{add_group,change_group,delete_group,add_user,change_user,delete_user}

Would it make sense to just have the backend remove these from the list of permissions, instead of adding extra logic?

chambridge · 2021-08-16T15:35:24Z

Question, isUserMgmtDisabled seems to be used everywhere where there's a check for user.model_permissions.{add_group,change_group,delete_group,add_user,change_user,delete_user}

Would it make sense to just have the backend remove these from the list of permissions, instead of adding extra logic?

Thanks for the feedback! I will take a look to see if this can be updated in the backend.

chambridge · 2021-08-16T21:57:33Z

@himdel I made the following update that reduced some of the usage you pointed out:
https://github.com/ansible/galaxy_ng/pull/889/files#diff-416aea42d5d99e187f7340543709cb3b126924b4fe30e6c36be869388c997845R132-R159

It was necessary to utilize the pattern for the user profile, to manage the human readable permission set, and for cases where permissions can be applied to groups (change_group permission); but you still need to hide adding/removing users from the group detail.

himdel · 2021-08-18T04:27:21Z

Makes sense 👍 :)

One last thing I'm confused about...

src/loaders/standalone/routes.tsx

config/standalone.dev.webpack.config.js

config/standalone.prod.webpack.config.js

config/webpack.base.config.js

chambridge · 2021-08-18T15:02:49Z

@himdel Thanks for the above. I set the default to match what was defined in Paths here: https://github.com/ansible/ansible-hub-ui/pull/809/files#diff-ee5c055de3e53b22e883d37e2b0ef701779dfbd11691ca5a6504df271b53e008L29

I'm not sure updating it to /ui/login is actually the right fix. My concern now is that in a non-development build (i.e. static content in django server) the login path will still be /ui/login and not trigger the Keycloak login.

Perhaps you have a thought here?

himdel · 2021-08-18T15:27:57Z

hm, I see what you mean .. we can't pass the same UI_LOGIN_URI both to replace and to places which previously used Paths.login.

Sounds like we need to distinguish between external login uris for replace and internal login urls for Redirect.
Maybe a simple solution is to replace(UI_BASE + UI_LOGIN_URI) and keep UI_LOGIN_URI='/login'.

Though I wonder if we could make it simpler by leaving Paths.login as is, and adding UI_EXTERNAL_LOGIN_URI instead, which defaults to null, and whose presence always triggers a full location.replace redirect (regardless of dev vs prod).

chambridge · 2021-08-18T17:29:39Z

@himdel I like the simplicity of the UI_EXTERNAL_LOGIN_URI option. I've reverted those previous changes. I also updated the server-side code so feature flags can be obtained if not authenticated so the login flow can be dynamically determined via that API.

https://github.com/ansible/galaxy_ng/pull/889/files#diff-85ab1792a81296d83b329359f5e9d8262535dad7cfda367e28ae6df427f2c2a6R9

* Enable development setup for external authentication * Disable user/group management Related to ansible/galaxy_ng#889

src/containers/group-management/group-detail.tsx

config/standalone.dev.webpack.config.js

himdel · 2021-08-23T13:24:48Z

@chambridge I'm on PTO this week, removing the review request for now, feel free to re-add :)

If you do end up going the route of "UI_EXTERNAL_LOGIN_URI can either be an absolute url, or a relative one absolute to UI root", this is currently misconfigured as it's missing the UI_BASE_PATH prefix, at least for standalone.

(Unless /login is considered the external url as everything else is /ui/something.)
Also, can you write down the intended defaults please? I'm pretty sure standalone.dev should have that null, not /login. (Or /ui/login instead of /login.) (I'm guessing standalone devel mode is never using external auth, as for the others, I can't tell the intent.)

chambridge · 2021-08-23T13:32:43Z

@chambridge I'm on PTO this week, removing the review request for now, feel free to re-add :)

If you do end up going the route of "UI_EXTERNAL_LOGIN_URI can either be an absolute url, or a relative one absolute to UI root", this is currently misconfigured as it's missing the UI_BASE_PATH prefix, at least for standalone.

(Unless /login is considered the external url as everything else is /ui/something.)
Also, can you write down the intended defaults please? I'm pretty sure standalone.dev should have that null, not /login. (Or /ui/login instead of /login.) (I'm guessing standalone devel mode is never using external auth, as for the others, I can't tell the intent.)

/login is considered the external url as there is a redirect that will handle the flow to keycloak in the backend (https://github.com/ansible/galaxy_ng/pull/889/files#diff-f00335e89bdbb1cc98ff1161298369ec25e0fcaf7baccc1cfff44b3e733bef71R54).

chambridge · 2021-08-23T13:42:12Z

@newswangerd based on my responses above I'm not sure what you would like me to update. Feel free to take a look at the responses and give an update; I'll re-request a review.

newswangerd · 2021-08-24T13:52:52Z

@newswangerd based on my responses above I'm not sure what you would like me to update. Feel free to take a look at the responses and give an update; I'll re-request a review.

Looks like you addressed all my questions.

newswangerd · 2021-08-27T14:17:32Z

config/standalone.dev.webpack.config.js

@@ -4,6 +4,7 @@ const webpackBase = require('./webpack.base.config');
 const proxyHost = process.env.API_PROXY_HOST || 'localhost';
 const proxyPort = process.env.API_PROXY_PORT || '5001';
 const apiBasePath = process.env.API_BASE_PATH || '/api/automation-hub/';
+const uiExternalLoginURI = process.env.UI_EXTERNAL_LOGIN_URI || '/login';


It just occurred to me that this won't work. This data needs to be passed to the UI by the API.

These configurations only affect the UI at build time, not at run time. For this to work, the UI would have to be recompiled for each environment that it's deployed in since the SSO login URL will presumably be different for every deployment.

In production deployment the value will be the default /login. The login flow does a window.location.replace() which will hit the login redirect in the API https://github.com/ansible/galaxy_ng/pull/889/files#diff-f00335e89bdbb1cc98ff1161298369ec25e0fcaf7baccc1cfff44b3e733bef71R54 it doesn't put it under the UI base path. Which should then trigger the SSO login calls.

ah, so the redirrect happens on the api side. That makes sense.

ZitaNemeckova · 2021-08-30T12:21:40Z

In dev. env. it breaks with UI_EXTERNAL_LOGIN_URI.

Edit: nevermind. Backend and UI not in sync.

chambridge · 2021-08-30T13:21:32Z

@ZitaNemeckova Do you have some logs or a link? I'm not encountering an error so I'm having trouble reproducing to try and resolve.

…ansible#809)" This reverts commit 503910e.

ZitaNemeckova · 2021-08-30T14:57:50Z

@chambridge It was my env. not having UI in sync with backend. Sorry for false alarm.

chambridge force-pushed the 863-ext-auth branch from 5eb0cf8 to 04f7b33 Compare August 16, 2021 14:40

chambridge force-pushed the 863-ext-auth branch from 04f7b33 to 8b55ee5 Compare August 16, 2021 21:52

chambridge marked this pull request as ready for review August 16, 2021 22:04

himdel reviewed Aug 18, 2021

View reviewed changes

src/loaders/standalone/routes.tsx Outdated Show resolved Hide resolved

himdel reviewed Aug 18, 2021

View reviewed changes

config/standalone.dev.webpack.config.js Outdated Show resolved Hide resolved

himdel reviewed Aug 18, 2021

View reviewed changes

config/standalone.prod.webpack.config.js Outdated Show resolved Hide resolved

himdel reviewed Aug 18, 2021

View reviewed changes

config/webpack.base.config.js Outdated Show resolved Hide resolved

chambridge force-pushed the 863-ext-auth branch 2 times, most recently from e413d69 to 81fc7c2 Compare August 18, 2021 17:23

chambridge force-pushed the 863-ext-auth branch from 81fc7c2 to a58b72e Compare August 18, 2021 17:55

Enable external auth support and disabling of user/group mgmt

748909a

* Enable development setup for external authentication * Disable user/group management Related to ansible/galaxy_ng#889

chambridge force-pushed the 863-ext-auth branch from 8bf6f17 to 748909a Compare August 18, 2021 19:52

chambridge requested a review from himdel August 19, 2021 13:39

newswangerd requested changes Aug 20, 2021

View reviewed changes

src/containers/group-management/group-detail.tsx Show resolved Hide resolved

config/standalone.dev.webpack.config.js Show resolved Hide resolved

himdel removed their request for review August 23, 2021 13:25

chambridge requested a review from newswangerd August 23, 2021 13:42

newswangerd approved these changes Aug 24, 2021

View reviewed changes

newswangerd requested changes Aug 27, 2021

View reviewed changes

newswangerd approved these changes Aug 27, 2021

View reviewed changes

newswangerd merged commit 503910e into ansible:master Aug 27, 2021

ZitaNemeckova added a commit to ZitaNemeckova/ansible-hub-ui that referenced this pull request Aug 30, 2021

Revert "Enable external auth support and disabling of user/group mgmt (…

e30cb15

…ansible#809)" This reverts commit 503910e.

himdel mentioned this pull request Aug 31, 2021

Fix Users Redirect #859

Merged

himdel mentioned this pull request Oct 7, 2021

standalone-loader, routes: fix keycloak redirect by setting featureFlags even if loading user/settings fails #1053

Merged

patchback bot mentioned this pull request Oct 11, 2021

[PR #1053/b59a60c0 backport][stable-4.4] standalone-loader, routes: fix keycloak redirect by setting featureFlags even if loading user/settings fails #1081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable external auth support and disabling of user/group mgmt #809

Enable external auth support and disabling of user/group mgmt #809

chambridge commented Aug 16, 2021

himdel commented Aug 16, 2021

chambridge commented Aug 16, 2021

chambridge commented Aug 16, 2021

himdel commented Aug 18, 2021

chambridge commented Aug 18, 2021 •

edited

Loading

himdel commented Aug 18, 2021

chambridge commented Aug 18, 2021

himdel commented Aug 23, 2021

chambridge commented Aug 23, 2021 •

edited

Loading

chambridge commented Aug 23, 2021

newswangerd commented Aug 24, 2021

newswangerd Aug 27, 2021

chambridge Aug 27, 2021

newswangerd Aug 27, 2021

ZitaNemeckova commented Aug 30, 2021 •

edited

Loading

chambridge commented Aug 30, 2021

ZitaNemeckova commented Aug 30, 2021

Enable external auth support and disabling of user/group mgmt #809

Enable external auth support and disabling of user/group mgmt #809

Conversation

chambridge commented Aug 16, 2021

himdel commented Aug 16, 2021

chambridge commented Aug 16, 2021

chambridge commented Aug 16, 2021

himdel commented Aug 18, 2021

chambridge commented Aug 18, 2021 • edited Loading

himdel commented Aug 18, 2021

chambridge commented Aug 18, 2021

himdel commented Aug 23, 2021

chambridge commented Aug 23, 2021 • edited Loading

chambridge commented Aug 23, 2021

newswangerd commented Aug 24, 2021

newswangerd Aug 27, 2021

Choose a reason for hiding this comment

chambridge Aug 27, 2021

Choose a reason for hiding this comment

newswangerd Aug 27, 2021

Choose a reason for hiding this comment

ZitaNemeckova commented Aug 30, 2021 • edited Loading

chambridge commented Aug 30, 2021

ZitaNemeckova commented Aug 30, 2021

chambridge commented Aug 18, 2021 •

edited

Loading

chambridge commented Aug 23, 2021 •

edited

Loading

ZitaNemeckova commented Aug 30, 2021 •

edited

Loading