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

Fix legacy/new permissions #153

Open
wants to merge 3 commits into
base: stretch-unstable
from

Conversation

@kay0u
Copy link
Member

kay0u commented Jan 20, 2020

Needs this PR

Close: #152
Resolve: YunoHost/issues#1507
And should resolve all our problems with legacy/new permissions system

I've tested it myself, but someone familiar with this project should indeed do a double triple check

To test it, apply YunoHost/yunohost#871 and this PR, reload nginx (systemctl reload nginx) and run yunohost app ssowatconf
Finally, play with unprotected/protected/skipped_url

@kay0u kay0u mentioned this pull request Jan 20, 2020
0 of 4 tasks complete
@kay0u kay0u requested a review from YunoHost/core-dev Jan 20, 2020
@kay0u kay0u added this to the 3.7.x milestone Jan 22, 2020
@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Jan 22, 2020

Ping @maniackcrudelis

This PR should fix the backwards compatibility with legacy permission. If you have time to test it.. I just don't want to create a big vulnerability by omitting something

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Jan 27, 2020

Well... I just made a simple test about the thing I was worried about. Results aren't great...

I installed Jirafeau and Lutim, both have some specific SSO rules.
See https://github.com/YunoHost-Apps/lutim_ynh/blob/master/scripts/install#L195-L205 and https://github.com/YunoHost-Apps/jirafeau_ynh/blob/testing/scripts/install#L150-L156

As expected, these url are private:
https://domain.tld/jirafeau/
https://domain.tld/jirafeau/admin.php
https://domain.tld/lutim/
https://domain.tld/lutim/stats/
https://domain.tld/lutim/manifest.webapp/
These ones are public:
https://domain.tld/jirafeau/f.php?h=1iK2k-Nl
https://domain.tld/lutim/fd34obVg/s2KOOupx.jpg
(I uploaded some files for the test)

Then I moved to testing 3.7
All private url for Lutim are now public.
https://domain.tld/lutim/
https://domain.tld/lutim/stats/
https://domain.tld/lutim/manifest.webapp/
Into the sso conf file, a line has been added for unprotected_urls as following:

"domain.tld/lutim"

Removing that new line does not fix the issue.

Then, I applied this fix but it didn't change anything. Neither the new line in sso config or the app now publicly accessible.
So, lutim became publicly accessible without any notice by upgrading to testing.

Also, as I just checked the doc (https://github.com/YunoHost/doc/blob/group-and-permissions/groups_and_permissions.md#migrating-away-from-the-legacy-permission-management) except saying that it won't work anymore, I have no clues about how to fix that.
And, as explained on the forum (If I remember well) to move from public to private an user should now play with 5 different permissions if a script is not doing it for him. Which is clearly not acceptable...

@zamentur

This comment has been minimized.

Copy link
Contributor

zamentur commented Jan 27, 2020

So if i understand well the issue with lutim:

The migration add "lutim main permission" to visitors. It's seem to be translated as "unprotected" rules (not 100% sure).

And the bug would be protected_regex are not considered, may be because unprotected on / is defined ??? I remember discussions about ordering permissions url rules with length of url prefix... But in the regex case it's not possible.

I am reading the ssowat code to try to understand

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Jan 27, 2020

I did remove the unprotected_urls added for lutim but it didn't solve anything.
It's not removed in the conf.json I sent you though

@zamentur

This comment has been minimized.

Copy link
Contributor

zamentur commented Jan 27, 2020

i think you have not regenerate the ssowat conf with the fix YunoHost/yunohost#871
or this fix desn't work ?
because the url in "permission" is still "/" in your ssowat conf: https://paste.yunohost.org/loletucuji

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Jan 27, 2020

That ssowat conf https://paste.yunohost.org/loletucuji is without the fix. Just the conf after the upgrade to testing.
That other one https://paste.yunohost.org/apikomuhiy is after applying the fix.

@zamentur

This comment has been minimized.

Copy link
Contributor

zamentur commented Jan 27, 2020

Even if it worked i think use skipped_uri on / and protected_uri or regex on a subpath is very strange.

skipped_uris means "SSOWat don't manage this path.". So subpath should be concerned too...

@zamentur

This comment has been minimized.

Copy link
Contributor

zamentur commented Jan 27, 2020

Found it! Indeed, in stable we do "3 redirect, 4 protected, 5 skipped" and in unstable we do "3 redirect, 4 skipped, 5 protected". That's the reason why you have this issue with lutim.

It's a regression, but as explained, it's abnormal to consider to examine protected uris before skipped...

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Jan 27, 2020

You mean instead of an unprotected_urls ?
I don't remember exactly why as it's an old part of the code. But I think it was because we had an issue with some part of the code around here in the sso.
And since it works that way, I guess I never changed it back.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Jan 27, 2020

Using unprotected_uris does work indeed before and after the upgrade.

@maniackcrudelis

This comment has been minimized.

Copy link
Contributor

maniackcrudelis commented Jan 27, 2020

By the way, trying the permission from the new panel, I notice that I can remove the visitors permission, which make the images private.
But there's no way to make the app public, since that the protected_regex that make the app private.

Anyway, from this fix it seems to work after an upgrade. Even if we noticed a possible regression.
But still, this feature needs to be more clear about some other points, like the doc, a proper way to migrate and the usage of script to move from public to private.

So as I asked on the chat, a meeting would be welcome to address those questions.

Copy link
Contributor

zamentur left a comment

This PR seems to kill all the backward compatibility. For example an old app package using protected_uris instruction install with a 3.7 and this change will not do the correct thing (i think).

Alternatively, i suggest to fix YunoHost/issues#1507 by modifying the migration and the app_addaccess.

@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Jan 28, 2020

Found it! Indeed, in stable we do "3 redirect, 4 protected, 5 skipped" and in unstable we do "3 redirect, 4 skipped, 5 protected". That's the reason why you have this issue with lutim.

I'm not sure about that:
Stable: https://github.com/YunoHost/SSOwat/blob/stretch-stable/access.lua#L256

Unstable: https://github.com/YunoHost/SSOwat/blob/stretch-unstable/access.lua#L281
in both case, it's a function, and this function is called in the Unprotected URLs, during the point 7

@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Jan 28, 2020

I think I found the "bug":

In the stable version, skipped_urls/regex check if this path is protected... It looks super weird since the skip should be used to... skip --'
In the unstable version, we don't check if this path is protected.

If I re add the check in the skip url, it fix the "bug", but I'm not sure if it's a bug or not...

@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Jan 28, 2020

I tried to change skipped_uris into unprotected_uris in lutim scripts, and it looks good to me. (don't forget to remove skipped_uris from the settings.yml of this app and run yunohost app ssowatconf` if you want to try on your side)

@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Jan 28, 2020

By the way, trying the permission from the new panel, I notice that I can remove the visitors permission, which make the images private.
But there's no way to make the app public, since that the protected_regex that make the app private.

Anyway, from this fix it seems to work after an upgrade. Even if we noticed a possible regression.
But still, this feature needs to be more clear about some other points, like the doc, a proper way to migrate and the usage of script to move from public to private.

So as I asked on the chat, a meeting would be welcome to address those questions.

The current limitation of permissions is indeed regex (we just can't for now). It can evolve ofc, and we can implement it if we need it.

A meeting would be great to fix what we want from permission system.

The documentation will come after the permissions system is fully implemented and when everyone is in agreement with how it works.

@zamentur

This comment has been minimized.

Copy link
Contributor

zamentur commented Jan 28, 2020

I think I found the "bug":

"It's not a bug, it's a feature"
247847a

So the question is do we want this feature now. I think yes.
I add this topic for the meeting https://pad.lqdn.fr/p/yunohost-28-01-2020

Copy link
Contributor

zamentur left a comment

To finish this work i think we should move this condityion https://github.com/YunoHost/SSOwat/blob/stretch-unstable/access.lua#L348 after the unprotected section.

Indeed if a user has no access to an app because his name is not in the list of allowed user in a permission, he should be able for example to answer to poll send by other users like a visitor could do (unprotected uris/regex).

I have read the other change and i am finally agree with its.

@zamentur zamentur mentioned this pull request Jan 28, 2020
1 of 3 tasks complete
@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Jan 29, 2020

Indeed if a user has no access to an app because his name is not in the list of allowed user in a permission, he should be able for example to answer to poll send by other users like a visitor could do (unprotected uris/regex).

You're right! I'll change that

I have read the other change and i am finally agree with its.

In fact, I found a bug with these lines.
When I try to go on the sso, I browse https://domain.tld/, in my PR there is no permission for that, so the user is by default allowed . And, I don't reach the sso but the default nginx index.html.
Same, when I try to access an url that doesn't exist, I reach the default nginx index.html, not the sso.

kay0u added 2 commits Jan 29, 2020
@kay0u kay0u requested a review from YunoHost/core-dev Jan 31, 2020
@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Feb 3, 2020

Bump

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Feb 4, 2020

Hello, sorry for the delay, I'll try to review all PR (on the group/permission) theses next days.

@Josue-T
Josue-T approved these changes Feb 7, 2020
Copy link
Contributor

Josue-T left a comment

Look like OK for me.

Have you tested this new code with the yunohost unit test ?

@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Feb 8, 2020

yup, but we don't test any edge cases during unit tests.

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Feb 8, 2020

Hello,

After thinking about the skipped_urls/regex and protected priority I was thinking about that and for me:

  • the difference between skipped_urls and protected_urls is just about the user authentication management (In the HEADER). So for me there are no reason to drop the possiblitly to protect a sub-url of a skipped_urls.
  • with this new change we are less consistant. Why we have different priority of the permission if we change the skipped_urls by unprotected_urls.

By example I'll working to implement a new SSO management for synapse. Actually we have / set as skipped_urls because it's just a server which provide a server API and client API, so I really don't want set this as unprotected. When I'll implement the SSO I will add small php code which will authenticate the user from the HEADER REMOTE_USER. To have this to work I'll need to set this as protected. So we will have this configuration by example:
domain.tld/_matrix/ -> skipped_urls.
domain.tld/_matrix/my_sso/auth.php -> protected.

So for all of theses reason I think that it's better to keep the old way to mange this.

@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Feb 8, 2020

I don't have any strong opinion on it, I can add it again if we feel the need.

@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Feb 12, 2020

The reason why this change have been done: #149

So, I don't know what to do now >.<

I propose to fix this bug on another PR, since I didn't change this part, and as I would like to see this one merged (if it works as intended)

@alexAubin alexAubin mentioned this pull request Feb 12, 2020
1 of 2 tasks complete
@kay0u kay0u requested review from zamentur and YunoHost/core-dev Feb 21, 2020
@kay0u

This comment has been minimized.

Copy link
Member Author

kay0u commented Feb 25, 2020

This PR should fix most of remains issue with legacy permissions:
kay0u@af89299

(@Josue-T use it here: YunoHost-Apps/synapse_ynh#175)

I use a lot of that PR, but it's not completely related to this one. Should I merge it here or create a new one that contains the fix and this PR?

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Feb 25, 2020

Should I merge it here or create a new one that contains the fix and this PR?

For me it's equals. I would say that it's better to create an other PR, to make more easy to review even if it's based on this one.

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.

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