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

ScopeAttribute and ScopeFromAttribute treat source with multiple @ differently #236

Open
thijskh opened this Issue Aug 12, 2015 · 11 comments

Comments

Projects
None yet
2 participants
@thijskh
Member

thijskh commented Aug 12, 2015

When presenting with an attribute value that has multiple @-signs, like john@doe@example.com, ScopeAttribute will consider everything after the first @ the scope, while ScopeFromAttribute considers only everything from the lastmost @ the scope.

This should probably be unified. I'm quite indifferent in which direction because I don't think it's a common case. Opinions?

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Sep 1, 2015

I agree, both should be unified.

Regarding which approach to follow, I believe there was some guidelines somewhere regarding what to consider the realm in a case like this. I'll have to do some research.

@thijskh

This comment has been minimized.

Member

thijskh commented Sep 1, 2015

AttributeRealm has a third approach: if the source attribute has multiple @'s, the authprocfilter just does nothing.

@jaimeperez jaimeperez added this to the v2.0 milestone Sep 1, 2015

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Sep 1, 2015

I found something, though not very promising. The eduPerson specification refers to this problem. The recommendation there is to use the first @ sign, considering the realm the rest of the value starting there.

In Feide, we are doing the opposite, actually. We are splitting eduPersonPrincipalName by the last @ sign when obtaining the realm from there, as that allows us to detect the case where a user is typing his or her email address as the username.

I don't have any strong feelings towards one or the other. However, taking into account the recommendation in the eduPerson specification, I'm inclined to just follow that and unify all the filters to split in the first @ sign. Does it sound reasonable? Objections?

I'm also wondering about core:ScopeFromAttribute and core:AttributeRealm. Both look for a realm in an existing attribute, and create a new one with it. Do you see any difference between them, apart from the fact that the latter uses the userid.attribute configuration option to determine where to fetch the realm from? I'm deprecating this option right now as per #3, so I'm wondering why would we want to maintain both.

@thijskh

This comment has been minimized.

Member

thijskh commented Sep 1, 2015

If their behaviour is unified, I believe that ScopeFromAttribute can be seen as a generalised version of AttributeRealm.

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Sep 2, 2015

Yes, and that's why I'm quite eager to deprecate core:AttributeRealm and get rid of it later.

What about the two options to parse realms? Come on, take a risk! 😛

@thijskh

This comment has been minimized.

Member

thijskh commented Sep 2, 2015

So in Feide you actually create ePPN based on the user's own input into the login form?

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Sep 2, 2015

Not exactly. We use that user input to try to log the user in. Then we fetch all his/her attributes from the directory. Remember we have one IdP connected via LDAP to all our institutions. The IdP lets you also choose your organization, and sometimes the one pre-selected for you could be wrong. This small trick we are doing allows us to disregard the pre-selected organization and select another one based on what the user entered, then authenticate, then fetch the attributes. So if I previously logged in with my UNINETT account and reach Feide login again, UNINETT will be pre-selected for me. But then I want to use my NTNU account, so I just need to append a @ntnu.no to my NTNU credentials, and all will work.

No other side effects in the attributes, anyway.

@thijskh

This comment has been minimized.

Member

thijskh commented Sep 2, 2015

I personally do not have any preference for a specific interpretation, although the no-op route seems a strange choice to me so I'd discard that as an option. Making it configurable is obviously the most flexible. I'd set the default to the eduPerson one and (that is missing in all three now) document the behaviour explicitly.

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Sep 2, 2015

Having it configurable with a common default behaviour sounds like the way to go. I also agree about no-op, I wasn't even considering that as an option.

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 14, 2016

Rereading this item I conclude that we'll:

  • Follow the eduPerson recommendation to use the first @ sign, considering the realm the rest of the value starting there, consistently in all places;
  • Deprecate AttributeRealm in favour of ScopeFromAttribute.
@thijskh

This comment has been minimized.

Member

thijskh commented Nov 14, 2016

I have filed #512 for the deprecation since it's essentially a different issue than the treatment of @.

thijskh added a commit that referenced this issue Nov 14, 2016

When attribute has multiple @, split on the first one.
eduPerson recommends: 'Multiple "@" signs are not recommended, but in any case,
the first occurrence of the "@" sign starting from the left is to be taken as
the delimiter between components. Thus, user identifier is to the left,
security domain to the right of the first "@".'

Closes #236
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment