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

Use issuer._ if it exists #187

Closed
wants to merge 1 commit into from
Closed

Use issuer._ if it exists #187

wants to merge 1 commit into from

Conversation

idris
Copy link

@idris idris commented Jan 27, 2017

For whatever reason, issuer was coming through as:

{
  _: 'http://theissuer',
  '$': { Format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:entity'
}

This PR fixes it, so it will use the value in _, similar to what is done elsewhere in saml.js

@idris
Copy link
Author

idris commented Jan 27, 2017

@bergie are you still maintaining this repo?

@bergie
Copy link
Contributor

bergie commented Jan 27, 2017

@ploer is the current maintainer, but see also #162

@pdspicer
Copy link
Contributor

@idris I'm picking up a maintainer role, one ask to add on to this PR: can you make issuerFormat available on the profile as well (same as is done for nameIDFormat) if issuer is present?

@cjbarth
Copy link
Collaborator

cjbarth commented Oct 2, 2018

@idris If there is still interest in this, can you please clean this up. @markstos What refinements are you looking for, especially now that we've updated the profile object a little?

@markstos
Copy link
Contributor

markstos commented Oct 2, 2018

For this, I think we should return the spec. I can't find any mention of sending an Issuer in a _ field, so did this case come up because bad data needed to be parsed from a third party, or because this module did a bad job parsing good data? Before patching this, I'd like have an example of a valid SAML document which triggers this case.

@jnardone
Copy link

I think this project has to decide if they are going to follow the spec or work to integrate with major vendors. If Okta is doing something weird, yeah that sucks, but they have a ton of marketshare and not supporting them is a disservice to anyone using this library.

@markstos
Copy link
Contributor

markstos commented Jan 2, 2019

@jnardone No one in this thread has mentioned Okta or any other vendor by name but you.

@jnardone
Copy link

jnardone commented Jan 2, 2019

The linked PR mentions it specifically: #283

@markstos
Copy link
Contributor

markstos commented Jan 2, 2019

@jnardone I've attempted to loop in an Okta rep to weigh in, via #283 (reference)

@andkrist
Copy link
Contributor

I think this project has to decide if they are going to follow the spec or work to integrate with major vendors. If Okta is doing something weird, yeah that sucks, but they have a ton of marketshare and not supporting them is a disservice to anyone using this library.

I'm having this issue with OAM , so it doesn't seem to be purely related to Okta.

@markstos
Copy link
Contributor

@andkrist Okta is trying to work with us to resolve the issue. Please see #283 (comment) and contribute example SAML payloads to contribute. Which OAM are you referring to?

@andkrist
Copy link
Contributor

Oracle Access Manager. I'm doing some debugging, but right now I have to catch the bus. Will provide example in #288.

@cjbarth
Copy link
Collaborator

cjbarth commented Mar 12, 2019

@idris #361 has been merged, which should address this issue. Is your concern still valid now that #361 has landed?

@markstos
Copy link
Contributor

I'm closing this PR, assuming #361 supercedes it, but leave a comment if you think there's another improvement not covered by that.

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.

7 participants