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

Use token roles in controllers #1211

Merged
merged 9 commits into from
Dec 8, 2023
Merged

Conversation

andchiind
Copy link
Contributor

@andchiind andchiind commented Nov 22, 2023

Makes it so that data models that have an associated installation are only returned or able to be set in requests when the user has the associated role in their bearer token. If the user has a generic role, such as Role.User or Role.Admin, then all data models are available.

Things that are currently missing which need to be added before merging:

  • (potentially for future PR) an extension method for the mocked HttpContextAccessor that allows roles to be added in unit tests. This way we could avoid using admin roles that allow access to all installations, which would then allow us to test that unauthenticated requests fail

@andchiind andchiind added feature New feature or request backend Backend related functionality labels Nov 22, 2023
Copy link

🔔 Changes in database folder detected 🔔
Do these changes require adding new migrations? 🤔 In that case follow these steps.
If you are uncertain, ask a database admin on the team 😄

@andchiind andchiind force-pushed the read-token branch 7 times, most recently from fb90f1f to f1eeb1f Compare November 25, 2023 11:05
@andchiind andchiind marked this pull request as ready for review November 25, 2023 11:07
@andchiind andchiind force-pushed the read-token branch 3 times, most recently from f591391 to 8e791f0 Compare November 28, 2023 09:57
@andchiind andchiind force-pushed the read-token branch 2 times, most recently from 4947cbe to a24c520 Compare November 28, 2023 14:42
Copy link
Contributor

@tsundvoll tsundvoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we take this in now, and test it a bit afterwards to verify desired behaviour :)

Copy link

🔔 Migrations changes detected 🔔
📣 Remember to comment "/UpdateDatabase" after review approval for migrations to take effect!

@github-actions github-actions bot added the database-change Will require migration label Nov 29, 2023
@@ -169,7 +169,7 @@ private IQueryable<MissionRun> GetMissionRunsWithSubModels()
.ThenInclude(robot => robot.Model)
.Include(missionRun => missionRun.Tasks)
.ThenInclude(task => task.Inspections)
.Where((m) => accessibleInstallationCodes.Result.Contains(m.Area.Installation.InstallationCode)); ;
.Where((m) => m.Area == null || accessibleInstallationCodes.Result.Contains(m.Area.Installation.InstallationCode.ToUpper())); ;
Copy link
Contributor

@prasm313 prasm313 Nov 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will installation1 users get all missions from installation2 without an area? (Applies for the change in MissionDefinitionService.cs as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a missionRun does not have an area it also does not have an installation. We can enforce the use of Area in missionRun maybe but I think this will require some refactoring for the localisation code

@@ -133,8 +133,8 @@ public async Task<MissionRun> Update(MissionRun missionRun)
if (missionRun.Area is not null) { context.Entry(missionRun.Area).State = EntityState.Unchanged; }

var entry = context.Update(missionRun);
await context.SaveChangesAsync();
_ = signalRService.SendMessageAsync("Mission run updated", new MissionRunResponse(missionRun));
await ApplyDatabaseUpdate(missionRun.Area?.Installation);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this not updated from the mqtt? Do we need to check this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is good to collect all the updates to one function so that it is easier to debug and protect in the future. It can get messy otherwise. The function should be able to handle not being called from a HTTP request

@andchiind andchiind force-pushed the read-token branch 2 times, most recently from d238431 to b4d91ff Compare December 1, 2023 12:27
@andchiind
Copy link
Contributor Author

I think it is ready to merge, but I'll wait to add new missions to huldra in echo before merging it

@andchiind andchiind force-pushed the read-token branch 2 times, most recently from b6cc9d2 to ad56069 Compare December 8, 2023 08:38
@andchiind
Copy link
Contributor Author

/UpdateDatabase

Copy link

github-actions bot commented Dec 8, 2023

👀 Running migration command... 👀

Copy link

github-actions bot commented Dec 8, 2023

❌ Migration failed, please see action log below for details ❌
https://github.com/equinor/flotilla/actions/runs/7139194977

@andchiind
Copy link
Contributor Author

/UpdateDatabase

Copy link

github-actions bot commented Dec 8, 2023

👀 Running migration command... 👀

Copy link

github-actions bot commented Dec 8, 2023

❌ Migration failed, please see action log below for details ❌
https://github.com/equinor/flotilla/actions/runs/7139824624

@andchiind
Copy link
Contributor Author

/UpdateDatabase

Copy link

github-actions bot commented Dec 8, 2023

👀 Running migration command... 👀

Copy link

github-actions bot commented Dec 8, 2023

✨ Successfully ran migration command! ✨

@andchiind andchiind merged commit 8b0cb86 into equinor:main Dec 8, 2023
12 checks passed
@andchiind andchiind deleted the read-token branch December 8, 2023 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend Backend related functionality database-change Will require migration feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants