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

Get session duration from SAML assertion #278

Closed
wants to merge 1 commit into from

Conversation

stavros-wb
Copy link
Contributor

No description provided.

@markstos
Copy link
Contributor

@stavros-wb Could you say more about this change?

As I read it, it's taking the session duration detected in a SAML response and saving it to a profile.

I checked out the README, it appears that currently there is no where that we define what is returned in a the profile object. Would you also be willing to update the README to document what is potentially returned in profile?

Maintaining this module is now a community effort. I'm fellow user like yourself that's helping as a I can, but help review and merge completed PRs. Thanks.

@cjbarth
Copy link
Collaborator

cjbarth commented Sep 11, 2018

There are actually a whole bunch of things like this that could be added to the generated profile object. For example there are Assertion.Conditions and Assertion.AuthnStatement that have very useful properties. I'd be willing to add them, but I'm not sure how we want to handle the idea that there are multiple Assertion.Conditions and Assertion.AuthnStatement.

Having said that, anyone can get any property from the XML they want by doing parsing profile.getAssertionXml(). What do you think an appropriate threshold for data is @markstos ?

@markstos
Copy link
Contributor

markstos commented Sep 11, 2018

@cjbarth Hmm, good question. We can either:

  1. Provide a stable set of profile fields, with the option to "get everything" through profile.getAssertionXml() or
  2. Go ahead and always call profile.getAssertionXml() and dump everything into the profile.

I'm not that familiar with what's typically returned and what the range of edge cases are. I think from a user's perspective, it's likely more convenient to have the fields already present, and let this module completely abstract away the XML and SAML protocol. So without having looked what some examples of 1 and 2 end up looking like, I lean slightly more towards 2.

I think it's work mocking up some examples of both approaches and comparing, though.

@cjbarth What do you think is the best approach here?

@cjbarth
Copy link
Collaborator

cjbarth commented Sep 12, 2018

@markstos The problem with option 2 is that it wouldn't be much different from calling profile.getAssertionXml() because that returns a JavaScript object anyway. What is more, we already have option 1.

I feel like there is some research that could be done to figure out what the common, or expected, SAML return values are and then build a more complete stable profile object around that. @stavros-wb, Do you know of a source for that kind of information? I'd love to see a sample or proposal of what an ideal profile object would look like.

I found the schema, which seems a little hard to grok: http://docs.oasis-open.org/security/saml/v2.0/saml-schema-assertion-2.0.xsd
There is also Wikipedia: https://en.wikipedia.org/wiki/SAML_2.0

Sadly the schema is filled with minOccurs, but not maxOccurs, so we can't really reliably determine how many of many valuable properties there are. We might just include everything that will occur only once according to the spec, so we can depend on that for a profile object.

@markstos
Copy link
Contributor

I work with dozens of SAML IdPs and typically all that is passed around is the first name, last name and email. An "employee Id" is the next most popular field in my anecdotal experience, but far less popular. There lack of consistency in what the "employee ID" field is named, but the SAML standard term is "NameID". Commonly the unique ID identifier is the email address. Someone who works at Okta, OneLogin, or similar could provide valuable prospective here.

@stavros-wb
Copy link
Contributor Author

I'm in favour of option 2, but add everything in a property rawJSON in the profile. I've tried using profile.getAssertionXml() but it returns the XML in a String

> profile.getAssertionXml().constructor
[Function: String]

@markstos I agree we should document this. I would use it, had I known it existed

@markstos
Copy link
Contributor

@stavros-wb When you say rawJSON, do you really mean JSON-- which would be a single JSON string, or do you really want the data parsed into a JavaScript object instead?

@stavros-wb
Copy link
Contributor Author

@markstos JavaScript object

@cjbarth
Copy link
Collaborator

cjbarth commented Sep 14, 2018

@markstos @stavros-wb I've created a PR (#301) that follows the spirit of this discussion. Please have a look.

@markstos
Copy link
Contributor

@stavros-wb Does #301 address your interest here? It's a more a general solution to the same issue.

The one difference I see is that this PR adds profile.sessionDuration to the profile object, while in #301, I think the same data would have to accessed as:

profile.getAssertion().SessionNotOnOrAfter

@stavros-wb
Copy link
Contributor Author

yes #301 will do the trick. thank you both @cjbarth @markstos . closing

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.

None yet

3 participants