Added customUrlValidation #331

vytautas-pranskunas- · 2018-05-22T10:00:32Z

Added custom url validation because we have an issue where we are calling optile requests for payment providers and there were only allowedUrls which we also have different like on local we have full url and on other environemtns calls are local and starts from 'api'. So with current allowedUrls there were not possibility to put allowed urls because it looks for startsWith. So i thought it is better to introduce custom url validation rather then add notAllowedUrls.

vytautas-pranskunas- · 2018-05-24T15:31:24Z

I have also changed access level of storeAccessTokenResponse to public because there are situations where you have to do silent login that means that you receiving tokens from other sources and it would be really not good to add it manually to session storage because it should use same storage.

vytautas-pranskunas- · 2018-05-27T09:27:11Z

I have synced masters so it is good to merge now. Is there a chance to get it merged sometime soon?

to indicate if login was a success

poke

Can you rebase your changes against the current master instead of merging it? That will make the history cleaner and will likely make this PR easier to accept.

poke · 2018-06-04T09:39:43Z

projects/lib/src/interceptors/default-oauth.interceptor.ts

+        const found = !this.moduleConfig.resourceServer.customUrlValidation
+            ? this.moduleConfig.resourceServer.allowedUrls.find(u => url.startsWith(u))
+            : this.moduleConfig.resourceServer.customUrlValidation(url);
+


I think you could simplify this a bit to avoid having to check the customUrlValidation multiple times:

if (this.moduleConfig.resourceServer.customUrlValidation) { return this.moduleConfig.resourceServer.customUrlValidation(url); } if (this.moduleConfig.resourceServer.allowedUrls) { return !!this.moduleConfig.resourceServer.allowedUrls.find(u => url.startsWith(u)); } return true;

poke · 2018-06-04T09:40:31Z

projects/lib/src/interceptors/default-oauth.interceptor.ts

@@ -39,7 +47,7 @@ export class DefaultOAuthInterceptor implements HttpInterceptor {
        if (!this.moduleConfig.resourceServer) {
            return next.handle(req);
        }


Maybe move the moduleConfig.resourceServer check into the checkUrl function as well then?

do not think this is good idea because there can be other configuration in resourceServer that are used not only for checking urls in the future. Dont you think so/ So i propose to leave it where it is

Yeah, you’re right. I missed that there were some other things used from resourceServer right after that line.

poke · 2018-06-04T09:41:03Z

projects/lib/src/oauth-service.ts

@@ -1216,7 +1216,7 @@ export class OAuthService extends AuthConfig {
        }
    }

-    private storeAccessTokenResponse(
+    public storeAccessTokenResponse(


I feel like this should be a separate PR

In ideal world it might be. But i have really lack of time in playing with git now tring to split it into 2 merge requests + it is not breaking change it is just open one essential method to public. Maybe we can merge it like it is this time? :)

…iodc after tryLogin()

vytautas-pranskunas- · 2018-06-05T21:36:24Z

Fixed comments

poke · 2018-06-05T22:35:27Z

I’m not in any way affiliated with this project, but the amount of commits in this pull request is really intimidating, especially considering the few actual changes this is introducing.

To clean that up, you could do git reset --soft origin/master and then make a new fresh commit (which will then include all the changes here), and finally push that using git push --force to update this pull request with that squashed commit.

And before you actually commit that, you might want to remove the changes to the package.json since that only includes whitespace changes.

vytautas-pranskunas- · 2018-06-06T07:04:58Z

I have tried soft reset but there is nothing happening. Maybe t his is because i have rebased it i am not sure (not big git guru). Anyway - ammount of commits is big but changes are obivous. Decission is yours but i think functionality that is added is more important than commits history :)

poke · 2018-06-06T07:41:43Z

Oh, my bad, origin/master is your own master. Try git reset --soft b4c87312f5 instead.

And sure, the changes are obvious but if I was a maintainer of this (again: I am not involved with this project, I am just trying to contribute just like you), I would reject this because it is quite a mess. So I just want to give you the chance and offer help to fix it first before that happens :)

…r authentication flows like Silent Login within same software

vytautas-pranskunas- · 2018-06-06T07:48:56Z

@poke thanks. Thats ofcourse is better. I did it.

poke · 2018-06-06T07:52:12Z

Cool, that looks much better, good job! 👍

vytautas-pranskunas- · 2018-06-11T19:39:04Z

Any chance to merge it? :)

jeroenheijmans · 2018-06-11T19:50:05Z

Disclaimer: I'm no maintainer or expert, but followed this PR with interest.

Some additional feedback I found while looking through the code.

I suggest changing the name for the custom validator property to allowedUrlValidator because then it shows up right next to allowedUrls in intellisense, making it easier for users to spot that they should use one or the other. Essentially, these settings are mutually exclusive. (In addition, this name conveys the typing a bit better IMHO)
It might be good to update docs along this change as well?

vytautas-pranskunas- · 2018-06-12T19:31:43Z

That makes sense and I will do it when maintainer will tell that he is going to merge it.

- Fixed some spelling errors - Fixed some newlines (add/remove) based on other code around it - Made capitalization and interpunction usage more consistent - Fixed some jsdoc comment alignment issues - Made small textual clarifications in some places

Fixes manfredsteyer#368

Fixes manfredsteyer#365

After further discussion and research in manfredsteyer#368 I noticed that even though RFC 7617 seems to state that: > Note that both scheme and parameter names are matched > case-insensitively. All the examples use "Basic" instead of "BASIC". There is one very notable framework that checks case *sensitively* for "Basic". Given that the header was incorrect anyways, we can be sure that changing this casing won't further break anything, and work around issues for server side frameworks like Spring. See: https://tools.ietf.org/html/rfc7617 See: https://github.com/spring-projects/spring-security/blob/5.0.6.RELEASE/web/src/main/java/org/springframework/security/web/authentication/www/BasicAuthenticationFilter.java#L157 Fixes manfredsteyer#368

…th-header Change 'Authentication' to 'Authorization'

…or-comment-issues Docs/fix minor comment issues

…ed-callback-for-no-iodc Issue manfredsteyer#343 onTokenReceived callback is not called if no iodc after tr…

Make all of the login functions return Promise<boolean>

WIP: feat: remove jsrsasign dependancy

…resh is not triggered with every interval.

Session check changed to run outside of Angular zone

Set accessibility for properties and methods to protected

Update oauth-service.ts

Resolves manfredsteyer#439

Update to Angular 7

@manfredsteyer

In manfredsteyer#30 the maintainer @manfredsteyer mentions adding an MIT license, but in 5b8cbbe it was accidentally removed. In manfredsteyer#385 it was mentioned that the license should be added back. So that's what we do here! Fixes manfredsteyer#370 See also manfredsteyer#385 See also manfredsteyer#30

…nse-back Add LICENSE back to repository

…th2-oidc

Added option to set up silent refresh only for certain type of token.

react-state · 2019-03-29T14:41:30Z

Is there a chance to merge this someday because because of this we need to keep fix in our local npm and cannot migrate to version 5 :(

…r authentication flows like Silent Login within same software

…lar-oauth2-oidc # Conflicts: # projects/lib/src/oauth-service.ts

vytautas-pranskunas- · 2019-04-18T13:33:40Z

Hello again. I made this pull requests without conflicts again. Maybe you can accept it?
I will explain problem again:
I added customUrlValidation function that validates url in addition to allowedUrls. Because when you have 20 countries with 4 environments each where api urls can be changed by devops without from time to time allowedUrls is not enough and can be painful.

p.s.
We are waiting for this for half a year now. ;)

adrianbenjuya · 2019-07-19T13:56:42Z

projects/lib/src/interceptors/default-oauth.interceptor.ts

@@ -39,7 +47,7 @@ export class DefaultOAuthInterceptor implements HttpInterceptor {
        if (!this.moduleConfig.resourceServer) {
            return next.handle(req);
        }
-        if (this.moduleConfig.resourceServer.allowedUrls && !this.checkUrl(url)) {
+        if (!this.checkUrl(url)) {


I don't see this change after the merge of the PR, the final code still has
if (this.moduleConfig.resourceServer.allowedUrls && !this.checkUrl(url)) {
I need to set both allowedUrls and customUrlValidation to make it work

PR created #584

ajpierson added 3 commits May 30, 2018 09:33

Updated all trylogin functions to return Promise<Boolean>

700a468

to indicate if login was a success

reverted to previous .gitignore

04f8a39

added cpr as a dev dependency since it's used in the build

4d2e48a

poke reviewed Jun 4, 2018

View reviewed changes

Issue manfredsteyer#343 onTokenReceived callback is not called if no …

15bff0c

…iodc after tryLogin()

Improved url check in intercveptor by adding customUrlValidation

38feedf

vytautas-pranskunas- force-pushed the master branch from 1f59de7 to 38feedf Compare June 6, 2018 07:47

Opened storeAccessTokenResponse to public to for users that uses othe…

6d471ee

…r authentication flows like Silent Login within same software

Fabian Wiles and others added 11 commits June 16, 2018 11:53

feat: remove jsrsasign dependancy

77cb37a

fixup! feat: remove jsrsasign dependancy

290c801

fixup! feat: remove jsrsasign dependancy

43ecad5

fixup! feat: remove jsrsasign dependancy

c2cce4e

Remove obsolete/commented code

c1e63de

Rename property to non-obsolete version in docs

946c5ef

Fix various minor issues

08210ca

- Fixed some spelling errors - Fixed some newlines (add/remove) based on other code around it - Made capitalization and interpunction usage more consistent - Fixed some jsdoc comment alignment issues - Made small textual clarifications in some places

Add documentation for loadDisco....And...() functions

bc6c4e1

Change 'Authentication' to 'Authorization'

3da25e1

Fixes manfredsteyer#368

Clarify imports in README example

bf5ef56

Fixes manfredsteyer#365

manfredsteyer added 5 commits September 19, 2018 19:38

Merge pull request manfredsteyer#371 from jeroenheijmans/fix/basic-au…

0cac039

…th-header Change 'Authentication' to 'Authorization'

Merge branch 'master' into docs/fix-minor-comment-issues

8fb836d

Merge pull request manfredsteyer#357 from jeroenheijmans/docs/fix-min…

b99db0f

…or-comment-issues Docs/fix minor comment issues

Merge pull request manfredsteyer#345 from denisbeklarov/onTokenReceiv…

bf279c6

…ed-callback-for-no-iodc Issue manfredsteyer#343 onTokenReceived callback is not called if no iodc after tr…

Merge branch 'master' into master

1266843

manfredsteyer added the investigation-needed Indication that the maintainer or involved community members may need to investigate more. label Sep 19, 2018

manfredsteyer and others added 17 commits September 19, 2018 19:58

Merge pull request manfredsteyer#339 from NwBankTech/master

7c752e5

Make all of the login functions return Promise<boolean>

Merge branch 'master' into master

846415f

Merge pull request manfredsteyer#356 from Toxicable/master

f49190e

WIP: feat: remove jsrsasign dependancy

Session check changed to run outside of Angular zone so that view ref…

874873a

…resh is not triggered with every interval.

Merge pull request manfredsteyer#441 from martin1cerny/master

a8df704

Session check changed to run outside of Angular zone

Update oauth-service.ts

aecf660

Set accessibility for properties and methods to protected

Merge pull request manfredsteyer#1 from jdgeier/issue/439

1610395

Update oauth-service.ts

Merge pull request manfredsteyer#448 from jdgeier/master

3e4b705

Resolves manfredsteyer#439

Update to Angular 7

1f9664f

Merge pull request manfredsteyer#456 from akehir/feature/angular-7

2ac4501

Update to Angular 7

v5 for Angular 7

1eec310

Merge pull request manfredsteyer#459 from jeroenheijmans/fix/add-lice…

3a01fe9

…nse-back Add LICENSE back to repository

downgrade to jsrsasign

c2162d0

Merge branch 'master' of https://github.com/manfredsteyer/angular-oau…

2863669

…th2-oidc

Added option to set up silent refresh only for certain type of token.

940f315

Merge pull request manfredsteyer#462 from martin1cerny/master

0274cf5

Added option to set up silent refresh only for certain type of token.

KALLY\vytautas.pranskunas added 3 commits April 18, 2019 15:21

Improved url check in intercveptor by adding customUrlValidation

2e37425

Opened storeAccessTokenResponse to public to for users that uses othe…

b24b4be

…r authentication flows like Silent Login within same software

Merge branch 'master' of https://github.com/vytautas-pranskunas-/angu…

44532af

…lar-oauth2-oidc # Conflicts: # projects/lib/src/oauth-service.ts

manfredsteyer merged commit d66c82a into manfredsteyer:master Jul 17, 2019

adrianbenjuya reviewed Jul 19, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added customUrlValidation #331

Added customUrlValidation #331

vytautas-pranskunas- commented May 22, 2018

vytautas-pranskunas- commented May 24, 2018

vytautas-pranskunas- commented May 27, 2018

poke left a comment

poke Jun 4, 2018

vytautas-pranskunas- Jun 5, 2018

poke Jun 4, 2018

vytautas-pranskunas- Jun 5, 2018

poke Jun 5, 2018

poke Jun 4, 2018

vytautas-pranskunas- Jun 5, 2018

vytautas-pranskunas- commented Jun 5, 2018

poke commented Jun 5, 2018

vytautas-pranskunas- commented Jun 6, 2018

poke commented Jun 6, 2018

vytautas-pranskunas- commented Jun 6, 2018

poke commented Jun 6, 2018

vytautas-pranskunas- commented Jun 11, 2018

jeroenheijmans commented Jun 11, 2018

vytautas-pranskunas- commented Jun 12, 2018

react-state commented Mar 29, 2019

vytautas-pranskunas- commented Apr 18, 2019

adrianbenjuya Jul 19, 2019

adrianbenjuya Jul 19, 2019

Added customUrlValidation #331

Added customUrlValidation #331

Conversation

vytautas-pranskunas- commented May 22, 2018

vytautas-pranskunas- commented May 24, 2018

vytautas-pranskunas- commented May 27, 2018

poke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vytautas-pranskunas- commented Jun 5, 2018

poke commented Jun 5, 2018

vytautas-pranskunas- commented Jun 6, 2018

poke commented Jun 6, 2018

vytautas-pranskunas- commented Jun 6, 2018

poke commented Jun 6, 2018

vytautas-pranskunas- commented Jun 11, 2018

jeroenheijmans commented Jun 11, 2018

vytautas-pranskunas- commented Jun 12, 2018

react-state commented Mar 29, 2019

vytautas-pranskunas- commented Apr 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment