Skip to content

Conversation

@LukasReschke
Copy link
Member

@LukasReschke LukasReschke commented Feb 16, 2017

This adds support for mapping attributes for full name and email. Replaces #61.

One can also merge multiple values by using something like urn:oid:2.5.4.42 urn:oid:2.5.4.4 in the display name mapping. The spaces act as a separator here.

⚠️ This requires master of Nextcloud Server as well or at least a cherry-pick of nextcloud/server#3507

Signed-off-by: Lukas Reschke [email protected]

Fixes #54 – at least the email and displayname part. Need to do some more considerations about groups.

This adds support for mapping attributes for full name and email

Signed-off-by: Lukas Reschke <[email protected]>
@LukasReschke LukasReschke added this to the Nextcloud 12.0 milestone Feb 16, 2017
@LukasReschke LukasReschke mentioned this pull request Feb 16, 2017
@codecov-io
Copy link

Codecov Report

Merging #90 into master will decrease coverage by -2.98%.
The diff coverage is 17.72%.

@@             Coverage Diff              @@
##             master      #90      +/-   ##
============================================
- Coverage     36.71%   33.74%   -2.98%     
- Complexity       98      123      +25     
============================================
  Files            10       10              
  Lines           414      489      +75     
============================================
+ Hits            152      165      +13     
- Misses          262      324      +62
Impacted Files Coverage Δ Complexity Δ
appinfo/app.php 0% <ø> (ø) 0 <ø> (ø)
lib/userbackend.php 4.32% <1.58%> (-1.33%) 61 <19> (+24)
lib/Settings/Admin.php 100% <100%> (ø) 5 <ø> (ø)
lib/Controller/SAMLController.php 51.08% <75%> (+0.52%) 26 <ø> (+1)

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 e8e3a51...1a1a11c. Read the comment docs.

@bne86
Copy link
Contributor

bne86 commented Feb 17, 2017

I can confirm that the code from this PR, together with a Nexcloud running at cdc48d301e3d4b8ad77e5e279f240ac1d441f252, works against our SAML IdP.
Email and displayname are successfully stored in the "accounts" database and are used in the WebUI.

@rzmf
Copy link

rzmf commented Feb 17, 2017

i do have a problem with the current version - has nothing to do with the mapping stuff
we are running a frontendproxy and some backends (commented out for now)

we are deploying the nextcloud services from saltstack, wrote all in occ commands, import (generated) json config for user_saml, ....

this exact configuration worked for nextcloud-11 but not for current master with the user_saml:attributes-to-map-for

{"reqId":"EuyTo+zyJkO7e9d3rHBS","remoteAddr":"127.0.0.1","app":"user_saml","message":"The response was received at https:\/\/cloud-t.XXXXX:8008\/apps\/user_saml\/saml\/acs instead of https:\/\/cloud-t.XXXXX\/apps\/user_saml\/saml\/acs","level":3,"time":"2017-02-17T12:26:07+00:00","method":"POST","url":"\/apps\/user_saml\/saml\/acs","user":"--","version":"12.0.0.13"}

nginx.conf:

 upstream nextcloud {
        server 127.0.0.1:8008;
        #server 192.168.XXX.XXX:8008;
        #server 192.168.XXX.XXX:8008;
    }

    server {
        listen       443 ssl http2 default_server;
....
 location / {            
            proxy_pass http://nextcloud;
            proxy_set_header Host            $host:$server_port;
            proxy_set_header X-Forwarded-For $remote_addr;
        }
....
 server {
        listen       localhost:8008 default_server;

....fastcgi and other stuff

@LukasReschke
Copy link
Member Author

@rzmf Can you open a new issue on this and also include your SAML config as well as your system config? I suspect this is something that we can address with https://github.com/onelogin/php-saml#url-guessing-methods 😉

@LukasReschke LukasReschke merged commit dd51c3d into master Feb 20, 2017
@LukasReschke LukasReschke deleted the attributes-to-map-for branch February 20, 2017 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants