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

Allow public apps with no sso tile #894

Merged
merged 2 commits into from Mar 26, 2020
Merged

Allow public apps with no sso tile #894

merged 2 commits into from Mar 26, 2020

Conversation

@kay0u
Copy link
Member

kay0u commented Mar 22, 2020

The problem

https://forum.yunohost.org/t/yunohost-3-7-spooky-testing-call-for-feedback/9385/49?u=kayou
https://forum.yunohost.org/t/yunohost-3-7-spooky-testing-call-for-feedback/9385/50?u=kayou

Solution

...

PR Status

Not fully tested, any though about it?

How to test

...

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 : Josué
  • Simple test 0/1 :
  • Deep review 0/1 :
@kay0u kay0u requested review from alexAubin, maniackcrudelis, Josue-T and YunoHost/core-dev Mar 22, 2020
@kay0u kay0u added this to the 3.7.x milestone Mar 22, 2020
Copy link
Contributor

Josue-T left a comment

Look like good for me, but haven't tested yet.

Thanks for the quick fix.

Copy link
Member

alexAubin left a comment

I suppose it does sort of cover the use case discussed on the forum. On the other hand, as for everything with the permission system, it's touchy because there are plenty of different scenario and use case to consider and everything has to be tested and thought about with precautions ...

I thought about it a bit and it seems okay, but I really can't say for sure that there aren't any piece of code (maybe in app_ssowatconf or related) doing some assumptions about this ... Also gotta be careful about what happens in app_install (and in particular the _migrate_legacy_permission) and in the migration procedure.

We probably want to update the group permission doc here to advise using

ynh_permission_update --permission "main" --add visitors all_users

instead of just adding visitors, otherwise we're going to have many users finding it weird that the app they just installed does not appear in their sso ...

@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Mar 22, 2020

I've tried every case I can think of, it works for me...

We were supposed to release version 3.7 this week, so I guess it's delayed, again?

kay0u added a commit to YunoHost/doc that referenced this pull request Mar 22, 2020
@kay0u kay0u mentioned this pull request Mar 22, 2020
@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Mar 23, 2020

I thought about it a bit and it seems okay, but I really can't say for sure that there aren't any piece of code (maybe in app_ssowatconf or related) doing some assumptions about this ... Also gotta be careful about what happens in app_install (and in particular the _migrate_legacy_permission) and in the migration procedure.

I'll try this week to update all of my apps with group-permission support. So it will be a good occasion to see if we still have some bug or not.

@kay0u kay0u merged commit adbd0a2 into stretch-testing Mar 26, 2020
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@kay0u kay0u deleted the public-app-not-in-sso branch Mar 26, 2020
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

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