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

Issues when adding several redirect_uri for an OIDC client's entry #1257

Closed
aliaksander-samuseu opened this Issue Sep 24, 2018 · 5 comments

Comments

Projects
None yet
4 participants
@aliaksander-samuseu

aliaksander-samuseu commented Sep 24, 2018

Environment:
gluu-server-3.1.4-RC6, CentOS7

Steps to reproduce;

  1. Log in to web UI
  2. Move to "OpenID Connect -> Clients"
  3. Create a new client entry with minimal set of required properties; use "pairwise" for "Subject Type" and specify only one redirect login uri
  4. Try to add an additional redirect uri with the same hostname part as the 1st one has (that step should fail, see below for details)
  5. Supply a valid "Sector Identifier URI" which returns a JSON file containing both of the redirect uris used in this test (use a uri pointing to a remote web server), and click the "Update" button
  6. Try to add 2nd redirect uri again

Results:

  1. At step 4) an error message is displayed reading "A sector identifier must be defined first" and Java error trace is registered in oxtrust.log (full text is here):
2018-09-24 16:44:07,520 ERROR [qtp1094834071-14] [org.gluu.oxtrust.ldap.service.SectorIdentifierService] (SectorIdentifierService.java:90) - Failed to find sector identifier by oxId sector-uris.json
org.gluu.site.ldap.persistence.exception.EntryPersistenceException: Failed to find entry: oxId=sector-uris.json,ou=sector_identifiers,o=@!E8DE.7456.204B.BE5E!0001!25AB.4068,o=gluu
	at org.gluu.site.ldap.persistence.LdapEntryManager.find(LdapEntryManager.java:306) ~[oxcore-ldap-3.1.4-SNAPSHOT.jar:?]
	at org.gluu.site.ldap.persistence.AbstractEntryManager.find(AbstractEntryManager.java:444) ~[oxcore-ldap-3.1.4-SNAPSHOT.jar:?]
	at org.gluu.site.ldap.persistence.AbstractEntryManager.find(AbstractEntryManager.java:381) ~[oxcore-ldap-3.1.4-SNAPSHOT.jar:?]
  1. At step 6) the same message and same log record are seen again, despite of the fact "Sector Identifier URI" is defined now

Expected results:

  1. According to the spec, sector identifier uri only becomes mandatory when several redirect uris are used and their hostname parts are different. Thus the fact a 2nd uri couldn't been added at step 4) doesn't seem right
  2. Even when a correct sector identifier uri is added as web UI requests, it's still not possible to add a 2nd redirect uri, thus it doesn't seem this web UI control functions properly at all
@aliaksander-samuseu

This comment has been minimized.

aliaksander-samuseu commented Sep 24, 2018

@yurem @shekhar16
I've tested it using the "OpenID Connect -> Manage Sector Identifiers" feature and it seems to work in this case (but the issue about it preventing you from having 2 redirect uris with the same hostname part still stands). So it's only when the JSON file is hosted at a remote resource the 2nd part of the issue occurs.

@syntrydy

This comment has been minimized.

Contributor

syntrydy commented Sep 25, 2018

@aliaksander-samuseu i'm the one who implement that restriction.

sector identifier uri only becomes mandatory when several redirect uris are used and their hostname parts are different: This wasn't not mentioned in that task.

Here is the link #966.

@aliaksander-samuseu

This comment has been minimized.

aliaksander-samuseu commented Sep 25, 2018

@syntrydy
Sure, assigning it to you then.

@aliaksander-samuseu

This comment has been minimized.

aliaksander-samuseu commented Sep 25, 2018

@syntrydy
FYI, here is the relevant part of the spec:

If the Client has not provided a value for sector_identifier_uri in Dynamic Client Registration, the Sector Identifier used for pairwise identifier calculation is the host component of the registered redirect_uri. If there are multiple hostnames in the registered redirect_uris, the Client MUST register a sector_identifier_uri.

@qbert2k: could you also perhaps comment on this one?

syntrydy pushed a commit that referenced this issue Sep 26, 2018

syntrydy added a commit that referenced this issue Sep 26, 2018

Merge pull request #1263 from /issues/1257
Fix loginUris selection #1257

syntrydy pushed a commit that referenced this issue Sep 26, 2018

@syntrydy

This comment has been minimized.

Contributor

syntrydy commented Sep 26, 2018

@aliaksander-samuseu i think it's okay now.

@syntrydy syntrydy closed this Sep 26, 2018

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