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

Development: Course access authorization alert sometimes appears twice #6382

Open
pal03377 opened this issue Mar 24, 2023 · 6 comments
Open
Assignees
Labels
bug client Pull requests that update TypeScript code. (Added Automatically!) good first issue

Comments

@pal03377
Copy link
Contributor

pal03377 commented Mar 24, 2023

Describe the bug

When you visit a course that you don't have access to and you also cannot register for, the access authorization alert on the top right often (but not always) appears twice.

To Reproduce

  1. Make sure you are logged in to Artemis. Otherwise you will be redirected to the login page.
  2. Go to <artemisUrl>/courses/<some course id you don't have access for>. Example: https://artemis.ase.in.tum.de/courses/55
  3. Observe that you get two authorization alerts on the top right.

Expected behavior

Only one authorization error alert appears.

Screenshots

screenshot-2023-03-24_001166

Which version of Artemis are you seeing the problem on?

6.1.1

What browsers are you seeing the problem on?

Chrome, Safari, Microsoft Edge, Firefox

Additional context

I think the problem is related to the course-overview.component.ts not waiting for the initial course load and already trying to access different routes / the same route again while the actual course is still loading.

I had it happen once that the alert only appeared once, but I don't know the reason for this.

Relevant log output

Failed to load resource: the server responded with a status of 403 () - /api/courses/55/for-dashboard:1
Failed to load resource: the server responded with a status of 403 () - /api/courses/55/for-dashboard:1
@pal03377 pal03377 added bug good first issue client Pull requests that update TypeScript code. (Added Automatically!) labels Mar 24, 2023
@quarz12
Copy link

quarz12 commented May 30, 2023

I have spent some time looking into this and it seems like the ngOnInit is being called twice. As far as I can tell, this is being done only by Angular. Therefore my best guess would be that the class CourseOverviewComponent is being instantiated twice. However I can't find where that is done. I would appreciate if someone could point me in the right direction.

@pal03377
Copy link
Contributor Author

pal03377 commented Jun 9, 2023

I have spent some time looking into this and it seems like the ngOnInit is being called twice. As far as I can tell, this is being done only by Angular. Therefore my best guess would be that the class CourseOverviewComponent is being instantiated twice. However I can't find where that is done. I would appreciate if someone could point me in the right direction.

Sorry that I haven't answered in so long. Your message got lost, but I stumbled over it now again.

I believe that the issue is probably not the component being initialized twice, but instead the /for-dashboard endpoint just being called twice by the same component. You can see this by the two separate line numbers that the 403 error originates from:

screenshot-2023-06-09_001423

I think that the problem is caused by the requests not waiting for each other, so when they both fail, two errors are shown.

@quarz12
Copy link

quarz12 commented Jun 11, 2023

I think the issue you are describing is only on the test server. However there does appear to be another issue on both normal and test server.
When I try to access course 55 on the normal server, I get these 2 errors:
image
This has probably something to do with error handling.
When I try the same on the test server, I get 4 error messages:
image

@pal03377
Copy link
Contributor Author

Oh, that's interesting! I think there probable is a race condition somewhere, which changes which error is thrown first and maybe also already stops existing requests (?). I think that waiting for the initial request to finish should resolve the issue in any cases, but I'm not even sure if the slight performance hit from doing that would be worth it 🤔

@aplr aplr self-assigned this Aug 11, 2023
@aplr
Copy link
Contributor

aplr commented Sep 15, 2023

Ok, I've investigated a bit. Each of the url is called just once as far as I can tell, however, both requests are sent, each triggering a 403 alert. IMHO, these requests should not be sent to begin with, but rather it should be checked if the user has access to the course w/ some kind of route guard. If the user does not have access, we could show a prettier 403 screen rather than an alert. WDYT?

@pal03377
Copy link
Contributor Author

Ok, I've investigated a bit. Each of the url is called just once as far as I can tell, however, both requests are sent, each triggering a 403 alert. IMHO, these requests should not be sent to begin with, but rather it should be checked if the user has access to the course w/ some kind of route guard. If the user does not have access, we could show a prettier 403 screen rather than an alert. WDYT?

I think this makes sense! However, I feel like it's a bit difficult to achieve. One solution I see is to first to one of the requests and then only after the response comes back without a 403 message start the second request. However, this would slightly affect performance, so you should probably ask @krusche about his opinion on this. I don't see how we could realistically determine access rights to the course any faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug client Pull requests that update TypeScript code. (Added Automatically!) good first issue
Projects
None yet
Development

No branches or pull requests

3 participants