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

Noauth #883

Closed
wants to merge 4 commits into from
Closed

Noauth #883

wants to merge 4 commits into from

Conversation

@yalh76
Copy link
Contributor

yalh76 commented Feb 22, 2020

The problem

Fix YunoHost/issues#1420

Solution

Adding a noauth parameter.

Can be used during package install: ynh_app_setting_set --app=$app --key=noauth_uris --value="/"

It will add in /etc/ssowat/conf.json:

    "noauth_urls": [
        "url with auth disabled"
    ], 

PR Status

Done and tested.
To be used with YunoHost/SSOwat#155

How to test

You can test it installing one of the apps as private app:
yunohost app install https://github.com/YunoHost-Apps/mastodon_ynh/tree/noauth --debug

yunohost app install https://github.com/YunoHost-Apps/peertube_ynh/tree/noauth --debug

yunohost app install https://github.com/YunoHost-Apps/bitwarden_ynh/tree/noauth --debug

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
yalh76 added 2 commits Feb 21, 2020
data/helpers.d/setting Outdated Show resolved Hide resolved
yalh76 added 2 commits Mar 24, 2020
@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Mar 25, 2020

Hello,

I'm adapting my apps for group and permission and now I was thinking about an other way to fix this issue.

As we have at least one permission by application, an other way is to define in the permission if we enable the noauth feature. By example we could say something like that:

permissionA:
 - url: domainA.tld/peertube
 - auth_header: false # Noauth enabled
permissionB:
 - url: domainB.tld/appB
 - auth_header:  true # Noauth enabled
permissionC:
 - url: None

And for me I think in long term we should depreciate the skipped_uris and just keep unprotected_uris with noauth enabled or not.

For the implementation we probably could take some idea from #861 which also update the LDAP schema for a new feature in the permission.

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Mar 26, 2020

Following own discussion, I give you here my idea about how will be the next ssowat configuration file:

{
    "additional_headers": {
        "Auth-User": "uid", 
        "Email": "mail", 
        "Name": "cn", 
        "Remote-User": "uid"
    }, 
    "domains": [
        "domainA.tld", 
        "domainB.tld"
    ], 
    "permissions": {
        "domainA.tld/SOGo": { // Equals to actual protected_uris
            "users": [
                "alice",
                "bob"
            ],
            "tile": "SOGo", // If 'tile' key don't exit, just don't show a tile in the SSO
            "auth_header": true,
           "protected": true
        },
        "domainB.tld/gitea": { // Equals to actual skipped_uris
            "users": [
                "alice",
                "jack"
            ],
            "tile": "Gitea",
            "auth_header": false,
            "protected": false,
        },
        "domainB.tld/gitea/admin": { // Equals to actual unprotected_uris
            "users": [
                "alice"
            ],
            "auth_header": true,
            "protected": false,
        },
        "re:domainB.tld/test[a-z]+/example": { // Equals to actual unprotected_regex
            "users": [
                "alice"
            ],
            "auth_header": true,
           "protected": false,
        },
        "re:domainB.tld/test[1-9]+/other-example": { // Equals to actual protected but with no auth header
            "users": [
                "alice"
            ],
            "auth_header": false,
           "protected": true,
        },
    }, 
    "portal_domain": "domain.tld", 
    "portal_path": "/yunohost/sso/", 
    "redirected_regex": {
        "domain.tld/yunohost[\\/]?$": "https://domain.tld/yunohost/sso/"
    }, 
    "redirected_urls": {}, 
}

It sill a first draft, we still can discuss about this, and improve it :-)

@kay0u

This comment has been minimized.

Copy link
Member

kay0u commented Mar 27, 2020

I think it would be clearer if we us the name of the permission as the key of the dictionnary:

"permissions": {
        "sogo.main": { // Equals to actual protected_uris
            "users": [
                "alice",
                "bob"
            ],
            "tile": "SOGo", // If 'tile' key don't exit, just don't show a tile in the SSO
            "auth_header": true,
           "protected": true,
           "url": "domainA.tld/SOGo"
        },
        "gitea.main": { // Equals to actual skipped_uris
            "users": [
                "alice",
                "jack"
            ],
            "tile": "Gitea",
            "auth_header": false,
            "protected": false,
            "url": "domainB.tld/gitea"
        },
}

Plus we can have "url" which is an array of urls, allowed by the permission, and "tile" could be a dictionary too with "title" and "url" (of the tile)

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Mar 27, 2020

I think it would be clearer if we us the name of the permission as the key of the dictionnary

Yes good idea.

Plus we can have "url" which is an array of urls, allowed by the permission, and "tile" could be a dictionary too with "title" and "url" (of the tile)

Well, it might make the helper really complicated with this idea. From my point of I would purpose this:

"permissions": {
        "sogo.main": { // Equals to actual protected_uris
            "users": [
                "alice",
                "bob"
            ],
            "tile": "SOGo", // If 'tile' key don't exit, just don't show a tile in the SSO
            "auth_header": true,
           "protected": true,
           "url": "domainA.tld/SOGo",
           "additionnal_url": [   // Not mandatory
                "domainB.tld/something",
                "domainA.tld/someotherthing"
        },
        "gitea.main": { // Equals to actual skipped_uris
            "users": [
                "alice",
                "jack"
            ],
            "tile": "Gitea",
            "auth_header": false,
            "protected": false,
            "url": "domainB.tld/gitea"
        },
}
@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Mar 27, 2020

Also an other thing. I would say we should als think about the solution which is the easiest way to manage on the SSOwat side. If we can find a structure of data which is more efficient and less complicated to implement in ssowat it could be good (as we know that the ssowat code is executed at each request).

Thinking about this maybe my first solution is more efficient, we don't need to iterate of each permission to see all url contained in the the permission (but @kay0u you probably know better than me about that).

@kay0u

This comment has been minimized.

Copy link
Member

kay0u commented Mar 27, 2020

If we follow what we said yesterday, each permission can have several urls, so we will have to iterate over them:-)

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Mar 27, 2020

If we follow what we said yesterday, each permission can have several urls, so we will have to iterate over them:-)

Ok, so ok for the last purpose ?

@kay0u

This comment has been minimized.

Copy link
Member

kay0u commented Mar 27, 2020

It looks good to me, another point of view from @YunoHost/core-dev ?

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Mar 28, 2020

Hello,

I'll continue work for this here : #861
Note made some minor change in the SSOwat conf, since some new reflexion.

@alexAubin alexAubin added this to the 3.8.x milestone Apr 1, 2020
Copy link
Member

alexAubin left a comment

@Josue-T @kay0u : wat do with dis ? Shall we go forward and merge this and YunoHost/SSOwat#155 ?

Edit: oh, or did we conclude that we should do YunoHost/SSOwat#155 differently ? I don't remember @.@

@Josue-T

This comment has been minimized.

Copy link
Contributor

Josue-T commented Apr 1, 2020

@yalh76

This comment has been minimized.

Copy link
Contributor Author

yalh76 commented Apr 1, 2020

I agree with @Josue-T / as he is working to integrate noauth in #861, I will close that PR :)

@yalh76 yalh76 closed this Apr 1, 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.

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