-
-
Notifications
You must be signed in to change notification settings - Fork 764
convert use of URI to string in models #620
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,14 +22,14 @@ namespace Thinktecture.IdentityServer.Core.Services.Default | |
{ | ||
public class DefaultRedirectUriValidator : IRedirectUriValidator | ||
{ | ||
public Task<bool> IsRedirecUriValidAsync(Uri requestedUri, Client client) | ||
public Task<bool> IsRedirecUriValidAsync(string requestedUri, Client client) | ||
{ | ||
var result = client.RedirectUris.Contains(requestedUri); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think thats good enough to compare URIs - case sensitivity in host name etc. For comparison we probably need to convert to URI first (since we don't want to rebuild all that logic). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep. Need unit tests for that. But with all the strings exact the same, it's working. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should still use the Uri class for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want to get into the business of comparing URIs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep -- I'll add those stronger checks. Is this the only place where we need those comparisons? |
||
|
||
return Task.FromResult(result); | ||
} | ||
|
||
public Task<bool> IsPostLogoutRedirecUriValidAsync(Uri requestedUri, Client client) | ||
public Task<bool> IsPostLogoutRedirecUriValidAsync(string requestedUri, Client client) | ||
{ | ||
var result = client.PostLogoutRedirectUris.Contains(requestedUri); | ||
|
||
|
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like ICollection - since this Model is used to define clients in code manually -
List<T>
plays much nicer with intellisense.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lern2linq :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is not about linq - it is about being able to press tab in VS (and not getting #if)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow (or agree), but that's ok -- I reverted it back.