Avoid tokens of trusted web clients after a password reset

[OmarIthawi]

Description

Filip found out that the missing AMC tokens is an Open edX feature after a password reset.

This fixes the issue by avoid removing the tokens for TrustedClient like the AMC.

See: https://tahoe.appsembler.com/admin/edx_oauth2_provider/trustedclient/

And yes, it has tests 😉

Questions

• Should I put this behind a feature or Wafl flag? Just in case we found out it's dangerous? @melvinsoft says yes! Done.
• How do we prevent marking mobile clients as trusted? Fixed
• Do you think the tests are solid? Or should test more intensively? Done

[OmarIthawi]

(4) Tahoe: LMS password reset process is destroying OAuth tokens and producing the spinning box

[thraxil]

Would this be useful to push upstream? Or is the token removal something that only affects us?

[OmarIthawi]

Thanks @thraxil! I think it would be useful, I'll check with the Open edX folks.

[melvinsoft]

@OmarIthawi Thanks for fixing this, it looks good to me!

I second @thraxil comment, we should push it upstream.

Also, regarding your questions, I think put behind a feature flag would be great, we're trying to get our enterprise and tahoe branches together, and this could be an undesired behaviour for enterprise customers.

[OmarIthawi]

Thanks @melvinsoft! I've already pushed it upstream, but it's going to be a long time before we can get a response from the Open edX folks.

So I'll put it behind a feature flag, disabled by default.

[OmarIthawi]

@melvinsoft what about the other questions. I'd love to get what you think on them!

[iamjazzar]

Thanks for the tests, @OmarIthawi! Some is always better than nothing.

I think this change will create a confusion in edx-oauth2-provider library as its documentation clearly stated the purpose behind the TrustedClient:

   """ 
   By default `django-oauth2-provider` shows a consent form to the
   user after his credentials has been validated. Trusted clients
   bypass the user consent and redirect to the OAuth2 client
   directly.
   """ 

One change here we can make is to add a flag to by passing the consent and/or another one to allow destroying OAuth tokens. What do you think?

Regarding your questions:

Should I put this behind a feature or Wafl flag? Just in case we found out it's dangerous? @melvinsoft says yes!?

I recommend this! It'll reduce the maintenance effort in the future, which is going to accelerate the development time by then 😉

How do we prevent marking mobile clients as trusted?

See my suggestion above ↑

Do you think the tests are solid? Or should test more intensively?

Those are good for now, more might be needed later. I still don't know if we can run all edx-platform tests in this repo?

[OmarIthawi]

Thanks @AhmedAljazzar!

Thanks for the tests, @OmarIthawi! Some is always better than nothing.

😄

I think this change will create a confusion in edx-oauth2-provider library as its documentation clearly stated the purpose behind the TrustedClient:

   """ 
   By default `django-oauth2-provider` shows a consent form to the
   user after his credentials has been validated. Trusted clients
   bypass the user consent and redirect to the OAuth2 client
   directly.
   """ 

Great catch! Thanks a lot for checking this.

One change here we can make is to add a flag to by passing the consent and/or another one to allow destroying OAuth tokens. What do you think?

I don't think it makes sense to disable the original behavior. So adding one more feature flag to toggle the new behavior sounds like a good option to me.

Regarding your questions:

Should I put this behind a feature or Wafl flag? Just in case we found out it's dangerous? @melvinsoft says yes!?

I recommend this! It'll reduce the maintenance effort in the future, which is going to accelerate the development time by then

How do we prevent marking mobile clients as trusted?

See my suggestion above ↑

Not sure I understand what you mean. Anyway, I think it's a minor point that we can address later.

Do you think the tests are solid? Or should test more intensively?

Those are good for now, more might be needed later. I still don't know if we can run all edx-platform tests in this repo?

No we don't have that. We used to have it for Ficus but we didn't get to it in Hawthorn yet.

[iamjazzar]

Not sure I understand what you mean. Anyway, I think it's a minor point that we can address later.

@OmarIthawi What I meant is: if we decided to go with implementing a flag for the new behavior, then we will be able to disable this flag for mobile clients (Or any other client technically). I don't know if edX creates mobile clients upon deployment, but if so, we can store those mobile clients with this flag set as disabled to skip the behavior.

I wonder what edX will say about this though.

[OmarIthawi]

@OmarIthawi What I meant is: if we decided to go with implementing a flag for the new behavior, then we will be able to disable this flag for mobile clients (Or any other client technically). I don't know if edX creates mobile clients upon deployment, but if so, we can store those mobile clients with this flag set as disabled to skip the behavior.

Ah, got you! I think this is something that I didn't think about! Mobile clients are indeed marked trusted sometimes. I have no idea how to separate mobile client from others.

What do you suggest?

I wonder what edX will say about this though.

That'll take sometime. But I think it's worth the wait.

[melvinsoft]

@OmarIthawi I agreed with @AhmedAljazzar here, those test look enough for now.

Regarding the mobile clients, I don't fully understand the question, I think mobile clients are always trusted.

Outdated.

[OmarIthawi]

@AhmedAljazzar @melvinsoft @thraxil I've added a feature flag and now the function removes the tokens for mobile applications even if they're trusted.

Now all the known issues of the original code are resolved. Please take another look and let me know what do you think.

[OmarIthawi]

Thanks @thraxil for the review!

@AhmedAljazzar one more look please!

[melvinsoft]

@OmarIthawi Thanks for implementing the changes! Let's try it on Staging!