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

(External) Authentication layer #1337

Open
wants to merge 10 commits into
base: skosmos-2
Choose a base branch
from
Open

Conversation

YOUR1
Copy link

@YOUR1 YOUR1 commented Jun 8, 2022

Reasons for creating this PR

We are in need of a Skosmos instance that is not available for the public, but only available through an authentication layer (SimpleSamlPHP in our case).

Description of the changes in this PR

Added an authentication layer, with minimal changes in the existing model and controller classes.

@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #1337 (dfb2d29) into master (089b8f5) will decrease coverage by 0.82%.
The diff coverage is 9.80%.

@@             Coverage Diff              @@
##             master    #1337      +/-   ##
============================================
- Coverage     70.68%   69.85%   -0.83%     
- Complexity     1646     1667      +21     
============================================
  Files            32       33       +1     
  Lines          3786     3835      +49     
============================================
+ Hits           2676     2679       +3     
- Misses         1110     1156      +46     
Impacted Files Coverage Δ
controller/Auth/SimpleSamlPHP/SimpleSamlPHP.php 0.00% <0.00%> (ø)
controller/Controller.php 52.44% <7.14%> (-4.92%) ⬇️
model/BaseConfig.php 71.42% <100.00%> (ø)
model/GlobalConfig.php 89.09% <100.00%> (+0.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 089b8f5...dfb2d29. Read the comment docs.

@sonarcloud
Copy link

sonarcloud bot commented Jun 8, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@osma osma self-assigned this Sep 12, 2022
@osma
Copy link
Member

osma commented Sep 12, 2022

Thanks for the PR @YOUR1 and sorry for the somewhat late response.

Can you explain why you decided to create an authentication layer in PHP? The alternative would be to configure Apache with authentication and authorization using one of the many mod_auth* modules. Of course this has its own limitations but at least it should be bullet-proof once configured since it protects a whole directory / URL space.

@YOUR1
Copy link
Author

YOUR1 commented Sep 13, 2022

Thanks for the PR @YOUR1 and sorry for the somewhat late response.

Can you explain why you decided to create an authentication layer in PHP? The alternative would be to configure Apache with authentication and authorization using one of the many mod_auth* modules. Of course this has its own limitations but at least it should be bullet-proof once configured since it protects a whole directory / URL space.

No problem. Using mod_auth implies that you are using apache2 as a webserver. That's obviously not always the case. Also; as you mentioned - mod_auth has its own limitations. We are using SimpleSamlPHP as a authentication backend for some of our clients; and mod_auth couldn't support that the way we wanted. Also; this adds more flexibility to add more/different authentication methods.

@nichtich
Copy link
Contributor

Authentication as part of Skosmos would make more sense if only some vocabularies are non-public. If everything is put behind login, a proxy would be a cleaner solution.

@osma
Copy link
Member

osma commented Mar 23, 2023

Authentication as part of Skosmos would make more sense if only some vocabularies are non-public.

Skosmos (and its REST API) still has global search and other ways to access combinations of vocabularies. Having to hide some vocabularies entirely from unauthorized would take a lot of work. Right now the assumption is that everything is public information.

@nichtich
Copy link
Contributor

Having to hide some vocabularies entirely from unauthorized would take a lot of work.

So then there is no need to add authentication functionality into Skosmos: the question of access is better solved on a different layer (e.g. webserver/proxy) than the PHP code of Skosmos. Just my 2 cents.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants