-
Notifications
You must be signed in to change notification settings - Fork 506
Discuss / design the values for ldap and sso fields of manifest beyond just true/false/not_relevant
#2639
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
base: main
Are you sure you want to change the base?
Conversation
fflorent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the hope to help
Co-authored-by: Florent <[email protected]>
Co-authored-by: Florent <[email protected]>
|
Tried to improve the doc about this and to be more explicit, not sure if it's the best solution, any feedback is welcome. |
docs/packaging/10.manifest/index.mdx
Outdated
| - `http_headers`: for the account provisioned by the HTTP Header: `Ynh-User`, `Ynh-User-Email`, `Ynh-User-Fullname` headers. For PHP apps it could be also provided by the standard `REMOTE_USER` parameter. | ||
| - `auth_basic`: for apps which use the auth basic authentication to handle the authentication. | ||
| - `client_sso`: for the client apps which support a SSO but the usage depends if the server support it. This is the case for instance with [Element](https://github.com/YunoHost-Apps/element_ynh) or [Agendav](https://github.com/YunoHost-Apps/agendav_ynh). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - `http_headers`: for the account provisioned by the HTTP Header: `Ynh-User`, `Ynh-User-Email`, `Ynh-User-Fullname` headers. For PHP apps it could be also provided by the standard `REMOTE_USER` parameter. | |
| - `auth_basic`: for apps which use the auth basic authentication to handle the authentication. | |
| - `client_sso`: for the client apps which support a SSO but the usage depends if the server support it. This is the case for instance with [Element](https://github.com/YunoHost-Apps/element_ynh) or [Agendav](https://github.com/YunoHost-Apps/agendav_ynh). | |
| - `"http_headers"`: for the account provisioned by the HTTP Header: `Ynh-User`, `Ynh-User-Email`, `Ynh-User-Fullname` headers. For PHP apps it could be also provided by the standard `REMOTE_USER` parameter. | |
| - `"auth_basic"`: for apps which use the auth basic authentication to handle the authentication. | |
| - `"client_sso"`: for the client apps for which the usage depends on whether the server supports it or not. This is the case for instance with [Element](https://github.com/YunoHost-Apps/element_ynh) or [Agendav](https://github.com/YunoHost-Apps/agendav_ynh). |
Also I find client_sso confusing due to the existence of OIDC clients.
Maybe it could be renamed server_side to tell that the SSO is supported by server side apps.
Or even it could be set to not_relevant and we could give an explanation that it fits this case too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I find
client_ssoconfusing due to the existence of OIDC clients.
Maybe it could be renamedserver_sideto tell that the SSO is supported by server side apps.
Yes I agree that it could be confusing, I don't mind to change it. But we need to keep in mind that the SSO usage in cases like this depends of the support client side and server side. So the flag here is to mention that the client App have some implementation to support the SSO but to use it you will need to target a server which also support the SSO. server_side seem to me not really explicit about the fact that the client also need to implement it, but I don't mind to change if everybody have a preference for this.
Co-authored-by: Florent <[email protected]>
fflorent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-authored-by: Florent <[email protected]>
docs/packaging/10.manifest/index.mdx
Outdated
| - `"http_headers"`: for the account provisioned by the HTTP Header: `Ynh-User`, `Ynh-User-Email`, `Ynh-User-Fullname` headers. For PHP apps it could be also provided by the standard `REMOTE_USER` parameter. | ||
| - `"auth_basic"`: for apps which takes advantage of the basic authentication ([RFC 7617](https://datatracker.ietf.org/doc/html/rfc7617)). | ||
| - `"client_sso"`: for the client apps for which the usage depends on whether the server supports it or not. This is the case for instance with [Element](https://github.com/YunoHost-Apps/element_ynh) or [Agendav](https://github.com/YunoHost-Apps/agendav_ynh). Note this only intended for app fully written in Javascript. If your app have some server side code, you should not use this value. | ||
| - `"oidc"`: for apps which use OIDC for authentication. Note for now code don't provide OIDC (until [#676](https://github.com/YunoHost/issues/issues/676) is fixed). For now the apps which use OIDC generally use [the dex_ynh app](https://github.com/YunoHost-Apps/dex_ynh). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eeeeeh i'm a bit confused because currently, supposedly the only supported values are true, false and "not_relevant" ? x_X I suppose it's good to brainstorm other values for the future of but uhhh as long as it's not implemented we don't really know what we'll really know the full picture ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
Yes I agree that we can't just merge this in this state. The idea was firstly to define the new value. And then since everybody agree with the new value, I'll make a PR on the different repos to support the news values. From my quick search it seem not too much used, just in the manifest schema, in the webadmin.
So to define this I tried mainly to think about the type of SSO we use in the app that I know or it could exist. I agree that for oidc is a bit special currently as it's not provided by the core now. But it seem that some apps use dex_ynh for this so we can already have some app which will use this value.
I think we can discuss about this in the next PR-Lanta to clarify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I edit the PR post in order to indicate that is a Documentation-Driven Development and this PR should not be merged until dev are made
ldap and sso fields of manifestldap and sso fields of manifest beyond just true/false/not_relevant
Problem
Solution
Status
Documentation-Driven Development: do not merge until the PR on code repo are merged and published...
PR checklist