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

config parameter disabling forcing sso iframe #463

Merged

Conversation

jonathanmmm
Copy link
Contributor

@jonathanmmm jonathanmmm commented Mar 29, 2022

haven't checked if it works

@jonathanmmm
Copy link
Contributor Author

would fix #462

@jonathanmmm jonathanmmm force-pushed the fix/462/sso-logins-disable-forced-iframe branch 3 times, most recently from 005b24b to 5d62585 Compare May 2, 2022 22:28
@jonathanmmm
Copy link
Contributor Author

@gary-kim it should build, I only changed one file and it complains about missing package:

[1/2] Removing module @nextcloud/browserslist-config...
error This module isn't specified in a package.json file.
info Visit https://yarnpkg.com/en/docs/cli/remove for documentation about this command.
make: *** [Makefile:18: 3rdparty/riot] Error 1

could you have a look at it?

@jonathanmmm jonathanmmm marked this pull request as ready for review May 2, 2022 22:34
@jonathanmmm
Copy link
Contributor Author

Now it passes all checks, have only changed false to true in the comment in line 59.

@gary-kim
Copy link
Owner

gary-kim commented May 4, 2022

Yeah, my bad. I think the build fixed itself because I updated one of the container images used in the build process.

@gary-kim
Copy link
Owner

gary-kim commented May 4, 2022

So, to add a new config, you will need to add it here with the default value:

public const AvailableSettings = [

then make it available to the frontend where you want to use it by adding another one of these:
$this->initialStateService->provideInitialState(Application::APP_ID, 'disable_custom_urls',

then add it to the admin settings by putting it here (should be similar to the disable custom URLs setting):
<input
id="disable_custom_urls"
v-model="disable_custom_urls"
type="checkbox"
class="checkbox"
@change="updateSetting('disable_custom_urls')"
>
<label
ref="disable_custom_urls"
for="disable_custom_urls"
>{{ t('riotchat', 'Disable custom URLs') }}</label>

and here
"sso_immediate_redirect": loadState('riotchat', 'sso_immediate_redirect') === 'true',

@jonathanmmm
Copy link
Contributor Author

jonathanmmm commented May 4, 2022

Have done it

then make it available to the frontend where you want to use it by adding another one of these:

$this->initialStateService->provideInitialState(Application::APP_ID, 'disable_custom_urls',

I am not sure if I implemented this part correctly.

@jonathanmmm
Copy link
Contributor Author

Maybe need documentary somewhere, if someone is using custom config.

src/components/AdminSettings.vue Outdated Show resolved Hide resolved
@jonathanmmm jonathanmmm force-pushed the fix/462/sso-logins-disable-forced-iframe branch from 7704807 to 997c579 Compare May 10, 2022 18:22
src/main.js Outdated Show resolved Hide resolved
@jonathanmmm
Copy link
Contributor Author

Have done it

then make it available to the frontend where you want to use it by adding another one of these:

$this->initialStateService->provideInitialState(Application::APP_ID, 'disable_custom_urls',

I am not sure if I implemented this part correctly.

@gary-kim did you check if implemented this part correctly?

Also where would be a good place to document this?

@jonathanmmm jonathanmmm requested a review from gary-kim May 20, 2022 14:30
@gary-kim gary-kim added this to the v0.12.0 milestone May 21, 2022
Copy link
Owner

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, tested it myself. It does work though the text of the setting and the effect of the setting seem reversed at the moment.

I think we can just fix that and make the setting name a bit clearer and that should be done!

As far as documentation goes, I think just being in the settings will be good enough for now. If questions start being asked, we can make some system for documentation. For now I'd prefer to have it be self-documenting if possible.

src/main.js Outdated
window.localStorage.setItem.apply(this, params);
};
// Setting sso_force_iframe (in config) to true forces iframe even if using SSO or CAS login
if (loadState('riotchat', 'sso_force_iframe') !== 'false') {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (loadState('riotchat', 'sso_force_iframe') !== 'false') {
if (loadState('riotchat', 'sso_force_iframe') === 'false') {

Copy link
Contributor Author

@jonathanmmm jonathanmmm May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think more setting it to !== 'true' would be better, if the value is anything else it will then be treated as false, but your suggestion is easier to read.

src/components/AdminSettings.vue Outdated Show resolved Hide resolved
src/main.js Outdated Show resolved Hide resolved
jonathanmmm and others added 6 commits May 24, 2022 13:33
Signed-off-by: jonathanmmm <32403139+jonathanmmm@users.noreply.github.com>
Signed-off-by: jonathanmmm <32403139+jonathanmmm@users.noreply.github.com>
Signed-off-by: jonathanmmm <32403139+jonathanmmm@users.noreply.github.com>
Update src/components/AdminSettings.vue

Co-authored-by: Gary Kim <gary@garykim.dev>
Signed-off-by: jonathanmmm <32403139+jonathanmmm@users.noreply.github.com>
Signed-off-by: jonathanmmm <32403139+jonathanmmm@users.noreply.github.com>

Signed-off-by: jonathanmmm <32403139+jonathanmmm@users.noreply.github.com>
Co-authored-by: Gary Kim <gary@garykim.dev>
Signed-off-by: jonathanmmm <32403139+jonathanmmm@users.noreply.github.com>
@jonathanmmm jonathanmmm force-pushed the fix/462/sso-logins-disable-forced-iframe branch from 204e6d6 to 8d14841 Compare May 24, 2022 11:37
Copy link
Owner

@gary-kim gary-kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, tested and it seems to work great! Thank you!

@gary-kim gary-kim modified the milestones: v0.12.0, v0.13.0 May 24, 2022
@gary-kim gary-kim merged commit e98717b into gary-kim:master May 24, 2022
@jonathanmmm jonathanmmm deleted the fix/462/sso-logins-disable-forced-iframe branch June 7, 2022 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants